From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 616771FF0B3 for ; Wed, 11 Nov 2026 14:18:11 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 7F8DD215DA; Wed, 11 Nov 2026 14:18:10 +0100 (CET) Message-ID: <1584ed85-8fcb-455e-be64-758e93775c1f@proxmox.com> Date: Wed, 11 Nov 2026 14:18:03 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [RFC PATCH proxmox-backup 3/4] close #7333: tape: restore: allow using several drives in parallel To: Jakob-Tobias Winter , pbs-devel@lists.proxmox.com References: <20261106133826.1692620-2-tobias@stuttgart.de> <20261106133826.1692620-5-tobias@stuttgart.de> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20261106133826.1692620-5-tobias@stuttgart.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794403084028 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.160 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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: UWCXGOWGCWBT36BKF7RHMRGP4UHEZWGG X-Message-ID-Hash: UWCXGOWGCWBT36BKF7RHMRGP4UHEZWGG X-MailFrom: d.csapak@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: comments inline On 11/6/26 2:39 PM, Jakob-Tobias Winter wrote: > Restoring a media set that spans several tapes from a single drive > takes long, even if other drives of the library are idle. Add the > optional 'additional-drives' parameter to the restore API and the > proxmox-tape CLI, for up to three drives in addition to the given one. > All drives are checked for the Tape.Read privilege, locked and marked > as in use for the whole task. > > A selective restore then reads the media of its first two phases in > parallel. A full restore cannot simply do the same: a tape backup > writes the chunks of a snapshot before its snapshot archive, possibly > to an earlier media, so with several drives a snapshot could be added > before all of its chunks are restored, and an aborted restore would > leave it behind with chunks missing. So with several drives, a full > restore uses the phases of the selective restore for all snapshots, > which only add the snapshots once all chunks are restored. This needs > the media set catalog, the snapshots only show up at the end of the > restore, and each media is loaded twice, once for the small snapshot > archives and once for the chunks. With a single drive, a full restore > works as before. > > Signed-off-by: Jakob-Tobias Winter > --- > src/api2/tape/restore.rs | 65 +++++++++++++++++++++++++++++++--------- > src/bin/proxmox-tape.rs | 10 +++++++ > 2 files changed, 61 insertions(+), 14 deletions(-) > > diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs > index ee8906f85..8040a6c21 100644 > --- a/src/api2/tape/restore.rs > +++ b/src/api2/tape/restore.rs > @@ -286,6 +286,16 @@ pub const ROUTER: Router = Router::new().post(&API_METHOD_RESTORE); > drive: { > schema: DRIVE_NAME_SCHEMA, > }, > + "additional-drives": { > + description: "Use these drives in addition to 'drive', to restore from several \ > + media in parallel.", > + type: Array, > + optional: true, > + max_length: 3, > + items: { > + schema: DRIVE_NAME_SCHEMA, > + }, > + }, as said in my answer to the cover letter, i'd prefer 'drives' as a replacement for drives instead of an additional parameter. > "media-set": { > description: "Media set UUID.", > type: String, > @@ -318,8 +328,9 @@ pub const ROUTER: Router = Router::new().post(&API_METHOD_RESTORE); > access: { > // Note: parameters are no uri parameter, so we need to test inside function body > description: "The user needs Tape.Read privilege on /tape/pool/{pool} and \ > - /tape/device/{drive}, Datastore.Backup privilege on /datastore/{store}/[{namespace}], \ > - Datastore.Modify privileges to create namespaces (if they don't exist).", > + /tape/device/{drive} (and each additional drive), Datastore.Backup privilege on \ > + /datastore/{store}/[{namespace}], Datastore.Modify privileges to create namespaces \ > + (if they don't exist).", > permission: &Permission::Anybody, > }, > )] > @@ -328,6 +339,7 @@ pub const ROUTER: Router = Router::new().post(&API_METHOD_RESTORE); > pub fn restore( > store: String, > drive: String, > + additional_drives: Option>, > namespaces: Option>, > media_set: String, > notify_user: Option, > @@ -374,7 +386,16 @@ pub fn restore( > } > } > } > - user_info.check_privs(&auth_id, &["tape", "device", &drive], PRIV_TAPE_READ, false)?; > + let mut drives = vec![drive]; > + for drive in additional_drives.unwrap_or_default() { > + if drives.contains(&drive) { > + bail!("drive '{drive}' given more than once"); nit: if we want to enforce that here, i think a hashset is a better way, since we have quadratic check cost here (shouldn't matter that much though) > + } > + drives.push(drive); > + } > + for drive in drives.iter() { > + user_info.check_privs(&auth_id, &["tape", "device", drive], PRIV_TAPE_READ, false)?; > + } > > let media_set_uuid = media_set.parse()?; > > @@ -388,7 +409,10 @@ pub fn restore( > let (drive_config, _digest) = pbs_config::drive::config()?; > > // early check/lock before starting worker > - let drive_lock = lock_tape_device(&drive_config, &drive)?; > + let drive_locks = drives > + .iter() > + .map(|drive| lock_tape_device(&drive_config, drive)) > + .collect::, _>>()?; > > let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI; > > @@ -404,23 +428,28 @@ pub fn restore( > auth_id.to_string(), > to_stdout, > move |worker| { > - let _drive_lock = drive_lock; // keep lock guard > + let _drive_locks = drive_locks; // keep lock guards > > - set_tape_device_state(&drive, &worker.upid().to_string())?; > + for drive in drives.iter() { > + set_tape_device_state(drive, &worker.upid().to_string())?; > + } > > let restore_owner = owner.as_ref().unwrap_or(&auth_id); > > info!("Mediaset '{media_set}'"); > info!("Pool: {pool}"); > > - let res = if snapshots.is_some() || namespaces { > + // the chunks of a snapshot can be on an earlier media that another drive did not > + // restore yet, so with several drives a full restore uses the phases of the selective > + // restore, which only add the snapshots after all chunks are restored > + let res = if snapshots.is_some() || namespaces || drives.len() > 1 { > restore_list_worker( > worker, > - snapshots.unwrap_or_default(), > + snapshots, > inventory, > media_set_uuid, > drive_config, > - std::slice::from_ref(&drive), > + &drives, > store_map, > restore_owner, > ¬ification_mode, > @@ -433,7 +462,7 @@ pub fn restore( > inventory, > media_set_uuid, > drive_config, > - &drive, > + &drives[0], > store_map, > restore_owner, > ¬ification_mode, > @@ -443,8 +472,10 @@ pub fn restore( > if res.is_ok() { > info!("Restore mediaset '{media_set}' done"); > } > - if let Err(err) = set_tape_device_state(&drive, "") { > - info!("could not unset drive state for {drive}: {err}"); > + for drive in drives.iter() { > + if let Err(err) = set_tape_device_state(drive, "") { > + info!("could not unset drive state for {drive}: {err}"); > + } > } > > res > @@ -626,7 +657,7 @@ fn log_required_tapes<'a>(inventory: &Inventory, list: impl Iterator #[allow(clippy::too_many_arguments)] > fn restore_list_worker( > worker: Arc, > - snapshots: Vec, > + snapshots: Option>, not sure why you change that to an option here? an empty snapshot list should already be a good enough indicator? i don't think it makes a good API do differentiate between no list or an empty list... > inventory: Inventory, > media_set_uuid: Uuid, > drive_config: SectionConfigData, > @@ -646,11 +677,17 @@ fn restore_list_worker( > > let res = proxmox_lang::try_block!({ > // phase 0 > + // without snapshots and namespace mapping, this is a full restore using several drives > + let restore_all = snapshots.is_none() && store_map.ns_map.is_none(); here we could just do an `is_empty()` check? > + let snapshots = snapshots.unwrap_or_default(); and this would stay where it was, before calling the function > let snapshots = if snapshots.is_empty() { > let mut restorable = Vec::new(); > // restore source namespaces > for (store, snapshot) in catalog.list_snapshots() { > let (ns, dir) = match parse_ns_and_snapshot(snapshot) { > + Ok((ns, dir)) if restore_all && store_map.target_store(store).is_some() => { > + (ns, dir) > + } > Ok((ns, dir)) if store_map.has_full_mapping(store, &ns) => (ns, dir), > Err(err) => { > warn!("couldn't parse snapshot {snapshot} - {err}"); > @@ -665,7 +702,7 @@ fn restore_list_worker( > &snapshot, > &ns, > &dir, > - false, > + restore_all, > &user_info, > auth_id, > restore_owner, > diff --git a/src/bin/proxmox-tape.rs b/src/bin/proxmox-tape.rs > index dcbf2d0fb..860ed38fc 100644 > --- a/src/bin/proxmox-tape.rs > +++ b/src/bin/proxmox-tape.rs > @@ -906,6 +906,16 @@ async fn backup(mut param: Value) -> Result<(), Error> { > schema: DRIVE_NAME_SCHEMA, > optional: true, > }, > + "additional-drives": { > + description: "Use these drives in addition to 'drive', to restore from several \ > + media in parallel.", > + type: Array, > + optional: true, > + max_length: 3, > + items: { > + schema: DRIVE_NAME_SCHEMA, > + }, > + }, > "media-set": { > description: "Media set UUID.", > type: String,