From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 112FB1FF136 for ; Mon, 20 Apr 2026 10:33:22 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E235C21763; Mon, 20 Apr 2026 10:33:21 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 20 Apr 2026 10:32:46 +0200 Message-Id: Subject: Re: [PATCH proxmox-backup v2 2/3] api: datastore: add option to run garbage collection before unmount To: "Hannes Laimer" , X-Mailer: aerc 0.20.0 References: <20260420074257.9492-1-h.laimer@proxmox.com> <20260420074257.9492-3-h.laimer@proxmox.com> In-Reply-To: <20260420074257.9492-3-h.laimer@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776673883053 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.123 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 4QFOKY2RKWKLMFHKSRG5CLA6BRG73S55 X-Message-ID-Hash: 4QFOKY2RKWKLMFHKSRG5CLA6BRG73S55 X-MailFrom: s.sterz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon Apr 20, 2026 at 9:42 AM CEST, Hannes Laimer wrote: > Removable datastores set up for auto-unmount have no natural point at > which to run garbage collection, since the drive is unmounted right > after jobs finish. Expose a gc-on-unmount option so GC can be triggered > as part of the unmount for those setups. > > Relies on the active write operation by garbage collection to block > the unmount task until GC completes (among possible other active > operations). No other active operation can occur in the meantime > since the datastore remains in 'unmount' maintenance mode. > > Signed-off-by: Hannes Laimer > --- > src/api2/admin/datastore.rs | 23 ++++++++++++++++++++--- > src/api2/config/datastore.rs | 9 +++++++++ > 2 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs > index 757b3114..212a48da 100644 > --- a/src/api2/admin/datastore.rs > +++ b/src/api2/admin/datastore.rs > @@ -2641,9 +2641,8 @@ fn do_unmount_device( > } > > async fn do_unmount(store: String, auth_id: Authid, to_stdout: bool) -> = Result { > - let _lock =3D pbs_config::datastore::lock_config()?; > - let (mut section_config, _digest) =3D pbs_config::datastore::config(= )?; > - let mut datastore: DataStoreConfig =3D section_config.lookup("datast= ore", &store)?; > + let (section_config, _digest) =3D pbs_config::datastore::config()?; > + let datastore: DataStoreConfig =3D section_config.lookup("datastore"= , &store)?; > > if datastore.backing_device.is_none() { > bail!("datastore '{store}' is not removable"); > @@ -2651,6 +2650,24 @@ async fn do_unmount(store: String, auth_id: Authid= , to_stdout: bool) -> Result > ensure_datastore_is_mounted(&datastore)?; > > + // Setting gc-on-unmount requires Datastore.Modify (or Datastore.All= ocate at creation), the > + // same level needed to start GC directly, so no privilege escalatio= n from triggering it here. > + if datastore.gc_on_unmount.unwrap_or(false) { > + let client =3D crate::client_helpers::connect_to_localhost() > + .context("failed to connect to localhost for starting GC")?; > + match client > + .post(&format!("api2/json/admin/datastore/{store}/gc"), None= ) > + .await > + { > + Ok(_) =3D> info!("started garbage collection, unmount will w= ait for it to finish"), > + Err(err) =3D> warn!("unable to start garbage collection befo= re unmount: {err}"), small question, any reason to do a round trip across the api here instead of factoring out the logic needed here from the `start_garbage_collection` function below and calling that directly? something like this should to the trick: ``` fn init_garbage_collection_job( store: String, auth_id: &Authid, to_stdout: bool, ) -> Result { let datastore =3D DataStore::lookup_datastore(lookup_with(&store, Opera= tion::Write))?; let job =3D Job::new("garbage_collection", &store) .map_err(|_| format_err!("garbage collection already running"))?; let upid_str =3D crate::server::do_garbage_collection_job(job, datastore, &auth_id, = None, to_stdout) .map_err(|err| { format_err!("unable to start garbage collection job on data= store {store} - {err:#}") })?; Ok(json!(upid_str)) } you can then call that in `do_unmount` and `start_garbage_collection`. or am i missing something? would also associate the gc task with user starting the unmount operation instead of root@pam if im not mistaken? > + } > + } > + > + let _lock =3D pbs_config::datastore::lock_config()?; > + let (mut section_config, _digest) =3D pbs_config::datastore::config(= )?; > + let mut datastore: DataStoreConfig =3D section_config.lookup("datast= ore", &store)?; > + > datastore.set_maintenance_mode(Some(MaintenanceMode { > ty: MaintenanceType::Unmount, > message: None, > diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs > index c44d50d1..c0be0296 100644 > --- a/src/api2/config/datastore.rs > +++ b/src/api2/config/datastore.rs > @@ -409,6 +409,8 @@ pub enum DeletableProperty { > Comment, > /// Delete the garbage collection schedule. > GcSchedule, > + /// Delete the gc-on-unmount property. > + GcOnUnmount, > /// Delete the prune job schedule. > PruneSchedule, > /// Delete the keep-last property > @@ -495,6 +497,9 @@ pub fn update_datastore( > DeletableProperty::GcSchedule =3D> { > data.gc_schedule =3D None; > } > + DeletableProperty::GcOnUnmount =3D> { > + data.gc_on_unmount =3D None; > + } > DeletableProperty::PruneSchedule =3D> { > data.prune_schedule =3D None; > } > @@ -560,6 +565,10 @@ pub fn update_datastore( > data.gc_schedule =3D update.gc_schedule; > } > > + if update.gc_on_unmount.is_some() { > + data.gc_on_unmount =3D update.gc_on_unmount; > + } > + > macro_rules! prune_disabled { > ($(($param:literal, $($member:tt)+)),+) =3D> { > $(