From 76328ada0597acd49d5150c6eec5804f22c47b86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arpad=20M=C3=BCller?= Date: Fri, 25 Oct 2024 22:06:27 +0200 Subject: [PATCH] Fix unoffload_timeline races with creation (#9525) This PR does two things: 1. Obtain a `TimelineCreateGuard` object in `unoffload_timeline`. This prevents two unoffload tasks from racing with each other. While they already obtain locks for `timelines` and `offloaded_timelines`, they aren't sufficient, as we have already constructed an entire timeline at that point. We shouldn't ever have two `Timeline` objects in the same process at the same time. 2. don't allow timeline creations for timelines that have been offloaded. Obviously they already exist, so we should not allow creation. the previous logic only looked at the timelines list. Part of #8088 --- pageserver/src/tenant.rs | 61 +++++++++++++++++++++--- pageserver/src/tenant/timeline/uninit.rs | 40 ++++++++++------ 2 files changed, 79 insertions(+), 22 deletions(-) diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index d4f6384d9bfe..f846e145c5f9 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -294,11 +294,11 @@ pub struct Tenant { /// During timeline creation, we first insert the TimelineId to the /// creating map, then `timelines`, then remove it from the creating map. - /// **Lock order**: if acquiring both, acquire`timelines` before `timelines_creating` + /// **Lock order**: if acquiring all (or a subset), acquire them in order `timelines`, `timelines_offloaded`, `timelines_creating` timelines_creating: std::sync::Mutex>, /// Possibly offloaded and archived timelines - /// **Lock order**: if acquiring both, acquire`timelines` before `timelines_offloaded` + /// **Lock order**: if acquiring all (or a subset), acquire them in order `timelines`, `timelines_offloaded`, `timelines_creating` timelines_offloaded: Mutex>>, // This mutex prevents creation of new timelines during GC. @@ -584,13 +584,19 @@ impl OffloadedTimeline { } } +impl fmt::Debug for OffloadedTimeline { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "OffloadedTimeline<{}>", self.timeline_id) + } +} + #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)] pub enum MaybeOffloaded { Yes, No, } -#[derive(Clone)] +#[derive(Clone, Debug)] pub enum TimelineOrOffloaded { Timeline(Arc), Offloaded(Arc), @@ -1815,6 +1821,8 @@ impl Tenant { } /// Loads the specified (offloaded) timeline from S3 and attaches it as a loaded timeline + /// + /// Counterpart to [`offload_timeline`]. async fn unoffload_timeline( self: &Arc, timeline_id: TimelineId, @@ -1823,6 +1831,24 @@ impl Tenant { ) -> Result, TimelineArchivalError> { info!("unoffloading timeline"); let cancel = self.cancel.clone(); + + // Protect against concurrent attempts to use this TimelineId + // We don't care much about idempotency, as it's ensured a layer above. + let allow_offloaded = true; + let _create_guard = self + .create_timeline_create_guard( + timeline_id, + CreateTimelineIdempotency::FailWithConflict, + allow_offloaded, + ) + .map_err(|err| match err { + TimelineExclusionError::AlreadyCreating => TimelineArchivalError::AlreadyInProgress, + TimelineExclusionError::AlreadyExists { .. } => { + TimelineArchivalError::Other(anyhow::anyhow!("Timeline already exists")) + } + TimelineExclusionError::Other(e) => TimelineArchivalError::Other(e), + })?; + let timeline_preload = self .load_timeline_metadata(timeline_id, self.remote_storage.clone(), cancel.clone()) .await; @@ -4129,7 +4155,8 @@ impl Tenant { new_timeline_id: TimelineId, idempotency: CreateTimelineIdempotency, ) -> Result, CreateTimelineError> { - match self.create_timeline_create_guard(new_timeline_id, idempotency) { + let allow_offloaded = false; + match self.create_timeline_create_guard(new_timeline_id, idempotency, allow_offloaded) { Ok(create_guard) => { pausable_failpoint!("timeline-creation-after-uninit"); Ok(StartCreatingTimelineResult::CreateGuard(create_guard)) @@ -4141,10 +4168,21 @@ impl Tenant { Err(CreateTimelineError::AlreadyCreating) } Err(TimelineExclusionError::Other(e)) => Err(CreateTimelineError::Other(e)), - Err(TimelineExclusionError::AlreadyExists { existing, arg }) => { + Err(TimelineExclusionError::AlreadyExists { + existing: TimelineOrOffloaded::Offloaded(_existing), + .. + }) => { + info!("timeline already exists but is offloaded"); + Err(CreateTimelineError::Conflict) + } + Err(TimelineExclusionError::AlreadyExists { + existing: TimelineOrOffloaded::Timeline(existing), + arg, + }) => { { let existing = &existing.create_idempotency; let _span = info_span!("idempotency_check", ?existing, ?arg).entered(); + debug!("timeline already exists"); match (existing, &arg) { // FailWithConflict => no idempotency check @@ -4467,17 +4505,26 @@ impl Tenant { /// Get a guard that provides exclusive access to the timeline directory, preventing /// concurrent attempts to create the same timeline. + /// + /// The `allow_offloaded` parameter controls whether to tolerate the existence of + /// offloaded timelines or not. fn create_timeline_create_guard( &self, timeline_id: TimelineId, idempotency: CreateTimelineIdempotency, + allow_offloaded: bool, ) -> Result { let tenant_shard_id = self.tenant_shard_id; let timeline_path = self.conf.timeline_path(&tenant_shard_id, &timeline_id); - let create_guard = - TimelineCreateGuard::new(self, timeline_id, timeline_path.clone(), idempotency)?; + let create_guard = TimelineCreateGuard::new( + self, + timeline_id, + timeline_path.clone(), + idempotency, + allow_offloaded, + )?; // At this stage, we have got exclusive access to in-memory state for this timeline ID // for creation. diff --git a/pageserver/src/tenant/timeline/uninit.rs b/pageserver/src/tenant/timeline/uninit.rs index 7d66c5aec805..c398289a5c8d 100644 --- a/pageserver/src/tenant/timeline/uninit.rs +++ b/pageserver/src/tenant/timeline/uninit.rs @@ -8,7 +8,7 @@ use utils::{fs_ext, id::TimelineId, lsn::Lsn}; use crate::{ context::RequestContext, import_datadir, - tenant::{CreateTimelineIdempotency, Tenant}, + tenant::{CreateTimelineIdempotency, Tenant, TimelineOrOffloaded}, }; use super::Timeline; @@ -177,7 +177,7 @@ pub(crate) struct TimelineCreateGuard<'t> { pub(crate) enum TimelineExclusionError { #[error("Already exists")] AlreadyExists { - existing: Arc, + existing: TimelineOrOffloaded, arg: CreateTimelineIdempotency, }, #[error("Already creating")] @@ -194,31 +194,41 @@ impl<'t> TimelineCreateGuard<'t> { timeline_id: TimelineId, timeline_path: Utf8PathBuf, idempotency: CreateTimelineIdempotency, + allow_offloaded: bool, ) -> Result { // Lock order: this is the only place we take both locks. During drop() we only // lock creating_timelines let timelines = owning_tenant.timelines.lock().unwrap(); + let timelines_offloaded = owning_tenant.timelines_offloaded.lock().unwrap(); let mut creating_timelines: std::sync::MutexGuard< '_, std::collections::HashSet, > = owning_tenant.timelines_creating.lock().unwrap(); if let Some(existing) = timelines.get(&timeline_id) { - Err(TimelineExclusionError::AlreadyExists { - existing: existing.clone(), + return Err(TimelineExclusionError::AlreadyExists { + existing: TimelineOrOffloaded::Timeline(existing.clone()), arg: idempotency, - }) - } else if creating_timelines.contains(&timeline_id) { - Err(TimelineExclusionError::AlreadyCreating) - } else { - creating_timelines.insert(timeline_id); - Ok(Self { - owning_tenant, - timeline_id, - timeline_path, - idempotency, - }) + }); } + if !allow_offloaded { + if let Some(existing) = timelines_offloaded.get(&timeline_id) { + return Err(TimelineExclusionError::AlreadyExists { + existing: TimelineOrOffloaded::Offloaded(existing.clone()), + arg: idempotency, + }); + } + } + if creating_timelines.contains(&timeline_id) { + return Err(TimelineExclusionError::AlreadyCreating); + } + creating_timelines.insert(timeline_id); + Ok(Self { + owning_tenant, + timeline_id, + timeline_path, + idempotency, + }) } }