From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 5F14B1FF136 for ; Mon, 20 Apr 2026 10:46:03 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3186021C73; Mon, 20 Apr 2026 10:46:03 +0200 (CEST) Message-ID: <41889dc0-da67-4316-86dd-8facc5e23405@proxmox.com> Date: Mon, 20 Apr 2026 10:45:57 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup v2 2/3] api: datastore: add option to run garbage collection before unmount To: Shannon Sterz , pbs-devel@lists.proxmox.com References: <20260420074257.9492-1-h.laimer@proxmox.com> <20260420074257.9492-3-h.laimer@proxmox.com> Content-Language: en-US From: Hannes Laimer In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776674674414 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.082 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: RY677OVJ2QL4DOVC3V7QZLRXR5S7USYR X-Message-ID-Hash: RY677OVJ2QL4DOVC3V7QZLRXR5S7USYR X-MailFrom: h.laimer@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 2026-04-20 10:31, Shannon Sterz wrote: > 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 = pbs_config::datastore::lock_config()?; >> - let (mut section_config, _digest) = pbs_config::datastore::config()?; >> - let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?; >> + let (section_config, _digest) = pbs_config::datastore::config()?; >> + let datastore: DataStoreConfig = 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.Allocate at creation), the >> + // same level needed to start GC directly, so no privilege escalation from triggering it here. >> + if datastore.gc_on_unmount.unwrap_or(false) { >> + let client = 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(_) => info!("started garbage collection, unmount will wait for it to finish"), >> + Err(err) => warn!("unable to start garbage collection before 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 = DataStore::lookup_datastore(lookup_with(&store, Operation::Write))?; > > let job = Job::new("garbage_collection", &store) > .map_err(|_| format_err!("garbage collection already running"))?; > > let upid_str = > 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 datastore {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? > the reason is that the unmounting is running in the api process, so as root. if we don't go through the api we would have the gc also running in the privileged process. general datastore operations, like gc, do assume they run as the `backup` user hitting the (proxy) api endpoint is the simplest way to have the gc run with the correct permissions >> + } >> + } >> + >> + let _lock = pbs_config::datastore::lock_config()?; >> + let (mut section_config, _digest) = pbs_config::datastore::config()?; >> + let mut datastore: DataStoreConfig = section_config.lookup("datastore", &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 => { >> data.gc_schedule = None; >> } >> + DeletableProperty::GcOnUnmount => { >> + data.gc_on_unmount = None; >> + } >> DeletableProperty::PruneSchedule => { >> data.prune_schedule = None; >> } >> @@ -560,6 +565,10 @@ pub fn update_datastore( >> data.gc_schedule = update.gc_schedule; >> } >> >> + if update.gc_on_unmount.is_some() { >> + data.gc_on_unmount = update.gc_on_unmount; >> + } >> + >> macro_rules! prune_disabled { >> ($(($param:literal, $($member:tt)+)),+) => { >> $( >