From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 5395F94FF1 for ; Tue, 28 Feb 2023 10:56:43 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2FEBD55F0 for ; Tue, 28 Feb 2023 10:56:13 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 28 Feb 2023 10:56:11 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 2082748A20 for ; Tue, 28 Feb 2023 10:56:11 +0100 (CET) Message-ID: Date: Tue, 28 Feb 2023 10:56:08 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Content-Language: en-US To: Proxmox Backup Server development discussion , Dominik Csapak References: <20230223115902.1180695-1-d.csapak@proxmox.com> <20230223115902.1180695-2-d.csapak@proxmox.com> From: Dietmar Maurer In-Reply-To: <20230223115902.1180695-2-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.480 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.092 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [drive.rs, media.rs, self.map, inventory.rs, self.name, mod.rs, pool.id] Subject: Re: [pbs-devel] [PATCH proxmox-backup 2/3] tape: initial WORM support X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Feb 2023 09:56:43 -0000 comments inline On 2/23/23 12:59, Dominik Csapak wrote: > the only thing preventing us from using WORM tapes was that we relied > on being able to rewrite the media set label when first using a tape > that was pre-allocated in a media-pool. > > so instead of needing to write a meida set label with a special uuid, > just save the pool in the media label itself. This has currently no > downsides, as we're not able to move tapes from one pool to another > anyway. > > this makes some checks a bit trickier, as we now have to get the pool > out of the media set label and as a fallback look into the media label. > > such new tapes can still be read and restored by older proxmox-bacukp-server > versions. The only thing missing is when a tape labeled with the new > format that has an assigned pool, that pool won't show up when the tape > is inventoried in an old version (but can still be used otherwise). > > Signed-off-by: Dominik Csapak > --- > src/api2/tape/drive.rs | 101 +++++++++++++++++------------------ > src/api2/tape/media.rs | 40 ++++++++------ > src/tape/file_formats/mod.rs | 3 ++ > src/tape/inventory.rs | 49 ++++++++++------- > src/tape/media_pool.rs | 10 ++-- > 5 files changed, 109 insertions(+), 94 deletions(-) > > diff --git a/src/api2/tape/drive.rs b/src/api2/tape/drive.rs > index a66c999c..cf4d6190 100644 > --- a/src/api2/tape/drive.rs > +++ b/src/api2/tape/drive.rs > @@ -346,19 +346,21 @@ pub fn format_media( > > let mut inventory = Inventory::new(TAPE_STATUS_DIR); > > - if let Some(MediaSetLabel { > - ref pool, ref uuid, .. > - }) = media_id.media_set_label > - { > - let _pool_lock = lock_media_pool(TAPE_STATUS_DIR, pool)?; > + let _pool_lock = if let Some(pool) = media_id.pool() { > + lock_media_pool(TAPE_STATUS_DIR, &pool)? > + } else { > + lock_unassigned_media_pool(TAPE_STATUS_DIR)? > + }; > + > + if let Some(MediaSetLabel { ref uuid, .. }) = media_id.media_set_label { > let _media_set_lock = lock_media_set(TAPE_STATUS_DIR, uuid, None)?; > MediaCatalog::destroy(TAPE_STATUS_DIR, &media_id.label.uuid)?; > - inventory.remove_media(&media_id.label.uuid)?; > } else { > - let _lock = lock_unassigned_media_pool(TAPE_STATUS_DIR)?; > MediaCatalog::destroy(TAPE_STATUS_DIR, &media_id.label.uuid)?; > inventory.remove_media(&media_id.label.uuid)?; > }; > + inventory.remove_media(&media_id.label.uuid)?; above refactoring looks wrong to me! I.e. you call remove_media twice... > + drop(_pool_lock); > > handle.format_media(fast.unwrap_or(true))?; > } > @@ -504,6 +506,7 @@ pub fn label_media( > label_text: label_text.to_string(), > uuid: Uuid::generate(), > ctime, > + pool: pool.clone(), > }; > > write_media_label(worker, &mut drive, label, pool) > @@ -520,50 +523,31 @@ fn write_media_label( > pool: Option, > ) -> Result<(), Error> { > drive.label_tape(&label)?; > - let media_id = if let Some(ref pool) = pool { > - // assign media to pool by writing special media set label > + if let Some(ref pool) = pool { > task_log!( > worker, > "Label media '{}' for pool '{}'", > label.label_text, > pool > ); > - let set = MediaSetLabel::new_unassigned(pool, label.ctime); > - > - drive.write_media_set_label(&set, None)?; > - > - let media_id = MediaId { > - label, > - media_set_label: Some(set), > - }; > - > - // Create the media catalog > - MediaCatalog::overwrite(TAPE_STATUS_DIR, &media_id, false)?; > - > - let mut inventory = Inventory::new(TAPE_STATUS_DIR); > - inventory.store(media_id.clone(), false)?; > - > - media_id > } else { > task_log!( > worker, > "Label media '{}' (no pool assignment)", > label.label_text > ); > + } > > - let media_id = MediaId { > - label, > - media_set_label: None, > - }; > - > - // Create the media catalog > - MediaCatalog::overwrite(TAPE_STATUS_DIR, &media_id, false)?; > + let media_id = MediaId { > + label, > + media_set_label: None, > + }; > > - let mut inventory = Inventory::new(TAPE_STATUS_DIR); > - inventory.store(media_id.clone(), false)?; > + // Create the media catalog > + MediaCatalog::overwrite(TAPE_STATUS_DIR, &media_id, false)?; > > - media_id > - }; > + let mut inventory = Inventory::new(TAPE_STATUS_DIR); > + inventory.store(media_id.clone(), false)?; > > drive.rewind()?; > > @@ -573,8 +557,8 @@ fn write_media_label( > bail!("verify label failed - got wrong label uuid"); > } > if let Some(ref pool) = pool { > - match info.media_set_label { > - Some(set) => { > + match (info.label.pool, info.media_set_label) { > + (None, Some(set)) => { > if !set.unassigned() { > bail!("verify media set label failed - got wrong set uuid"); > } > @@ -582,7 +566,12 @@ fn write_media_label( > bail!("verify media set label failed - got wrong pool"); > } > } > - None => { > + (Some(initial_pool), _) => { > + if initial_pool != *pool { > + bail!("verify media label failed - got wrong pool"); > + } > + } > + (None, None) => { > bail!("verify media set label failed (missing set label)"); > } > } > @@ -686,7 +675,7 @@ pub async fn read_label(drive: String, inventorize: Option) -> Result label_text: media_id.label.label_text.clone(), > media_set_ctime: None, > media_set_uuid: None, > - pool: None, > + pool: media_id.label.pool.clone(), > seq_nr: None, > uuid: media_id.label.uuid.clone(), > } > @@ -695,19 +684,20 @@ pub async fn read_label(drive: String, inventorize: Option) -> Result if let Some(true) = inventorize { > let mut inventory = Inventory::new(TAPE_STATUS_DIR); > > - if let Some(MediaSetLabel { > - ref pool, ref uuid, .. > - }) = media_id.media_set_label > - { > - let _pool_lock = lock_media_pool(TAPE_STATUS_DIR, pool)?; > + let _pool_lock = if let Some(pool) = media_id.pool() { > + lock_media_pool(TAPE_STATUS_DIR, &pool)? > + } else { > + lock_unassigned_media_pool(TAPE_STATUS_DIR)? > + }; > + > + if let Some(MediaSetLabel { ref uuid, .. }) = media_id.media_set_label { > let _lock = lock_media_set(TAPE_STATUS_DIR, uuid, None)?; > MediaCatalog::destroy_unrelated_catalog(TAPE_STATUS_DIR, &media_id)?; > - inventory.store(media_id, false)?; > } else { > - let _lock = lock_unassigned_media_pool(TAPE_STATUS_DIR)?; > MediaCatalog::destroy(TAPE_STATUS_DIR, &media_id.label.uuid)?; > - inventory.store(media_id, false)?; > }; > + > + inventory.store(media_id, false)?; > } > > Ok(label) > @@ -952,8 +942,13 @@ pub fn update_inventory( > media_id.label.uuid > ); > > + let _pool_lock = if let Some(pool) = media_id.pool() { > + lock_media_pool(TAPE_STATUS_DIR, &pool)? > + } else { > + lock_unassigned_media_pool(TAPE_STATUS_DIR)? > + }; > + > if let Some(ref set) = media_id.media_set_label { > - let _pool_lock = lock_media_pool(TAPE_STATUS_DIR, &set.pool)?; > let _lock = lock_media_set(TAPE_STATUS_DIR, &set.uuid, None)?; > MediaCatalog::destroy_unrelated_catalog(TAPE_STATUS_DIR, &media_id)?; > inventory.store(media_id.clone(), false)?; > @@ -977,7 +972,6 @@ pub fn update_inventory( > } > } > } else { > - let _lock = lock_unassigned_media_pool(TAPE_STATUS_DIR)?; > MediaCatalog::destroy(TAPE_STATUS_DIR, &media_id.label.uuid)?; > inventory.store(media_id, false)?; > }; > @@ -1113,6 +1107,7 @@ fn barcode_label_media_worker( > label_text: label_text.to_string(), > uuid: Uuid::generate(), > ctime, > + pool: pool.clone(), > };catzalog > > write_media_label(worker.clone(), &mut drive, label, pool.clone())? > @@ -1296,7 +1291,11 @@ pub fn catalog_media( > let (_media_set_lock, media_set_uuid) = match media_id.media_set_label { > None => { > task_log!(worker, "media is empty"); > - let _lock = lock_unassigned_media_pool(TAPE_STATUS_DIR)?; > + let _pool_lock = if let Some(pool) = media_id.pool() { > + lock_media_pool(TAPE_STATUS_DIR, &pool)? > + } else { > + lock_unassigned_media_pool(TAPE_STATUS_DIR)? > + }; > MediaCatalog::destroy(TAPE_STATUS_DIR, &media_id.label.uuid)?; > inventory.store(media_id.clone(), false)?; > return Ok(()); > diff --git a/src/api2/tape/media.rs b/src/api2/tape/media.rs > index cdeffd5b..3e8e1a9d 100644 > --- a/src/api2/tape/media.rs > +++ b/src/api2/tape/media.rs > @@ -240,37 +240,45 @@ pub async fn list_media( > // set status to MediaStatus::Unknown > for uuid in inventory.media_list() { > let media_id = inventory.lookup_media(uuid).unwrap(); > - let media_set_label = match media_id.media_set_label { > - Some(ref set) => set, > - None => continue, > - }; > > - if config.sections.get(&media_set_label.pool).is_some() { > - continue; > - } > + if let Some(pool) = media_id.pool() { > + if config.sections.get(&pool).is_some() { > + continue; > + } > > - let privs = user_info.lookup_privs(&auth_id, &["tape", "pool", &media_set_label.pool]); > - if (privs & PRIV_TAPE_AUDIT) == 0 { > + let privs = user_info.lookup_privs(&auth_id, &["tape", "pool", &pool]); > + if (privs & PRIV_TAPE_AUDIT) == 0 { > + continue; > + } > + } else { > continue; > } > > let (_status, location) = inventory.status_and_location(uuid); > - > - let media_set_name = inventory.generate_media_set_name(&media_set_label.uuid, None)?; > + let (media_set_name, media_set_ctime, media_set_uuid, seq_nr) = > + match media_id.media_set_label { > + Some(ref set) => ( > + Some(inventory.generate_media_set_name(&set.uuid, None)?), > + Some(set.ctime), > + Some(set.uuid.clone()), > + Some(set.seq_nr), > + ), > + None => (None, None, None, None), > + }; > > list.push(MediaListEntry { > uuid: media_id.label.uuid.clone(), > label_text: media_id.label.label_text.clone(), > ctime: media_id.label.ctime, > - pool: Some(media_set_label.pool.clone()), > + pool: media_id.pool(), > location, > status: MediaStatus::Unknown, > catalog: catalogs.contains(uuid), > expired: false, > - media_set_ctime: Some(media_set_label.ctime), > - media_set_uuid: Some(media_set_label.uuid.clone()), > - media_set_name: Some(media_set_name), > - seq_nr: Some(media_set_label.seq_nr), > + media_set_ctime, > + media_set_uuid, > + media_set_name, > + seq_nr, > }); > } > > diff --git a/src/tape/file_formats/mod.rs b/src/tape/file_formats/mod.rs > index f80e2d90..0f983059 100644 > --- a/src/tape/file_formats/mod.rs > +++ b/src/tape/file_formats/mod.rs > @@ -131,6 +131,9 @@ pub struct MediaLabel { > pub label_text: String, > /// Creation time stamp > pub ctime: i64, > + /// The initial pool the media is reserved for > + #[serde(skip_serializing_if = "Option::is_none")] > + pub pool: Option, > } > > #[derive(Serialize, Deserialize, Clone, Debug)] > diff --git a/src/tape/inventory.rs b/src/tape/inventory.rs > index e01a7f91..b84bd346 100644 > --- a/src/tape/inventory.rs > +++ b/src/tape/inventory.rs > @@ -64,6 +64,15 @@ pub struct MediaId { > pub media_set_label: Option, > } > > +impl MediaId { > + pub fn pool(&self) -> Option { > + if let Some(set) = &self.media_set_label { > + return Some(set.pool.to_owned()); > + } > + self.label.pool.to_owned() > + } > +} > + > #[derive(Serialize, Deserialize)] > struct MediaStateEntry { > id: MediaId, > @@ -249,11 +258,12 @@ impl Inventory { > /// > /// Returns (pool, is_empty) > pub fn lookup_media_pool(&self, uuid: &Uuid) -> Option<(&str, bool)> { > - let pool = self.map.get(uuid)?; > - pool.id > - .media_set_label > - .as_ref() > - .map(|media_set| (media_set.pool.as_str(), media_set.unassigned())) > + let media_id = &self.map.get(uuid)?.id; > + match (&media_id.label.pool, &media_id.media_set_label) { > + (_, Some(media_set)) => Some((media_set.pool.as_str(), media_set.unassigned())), > + (Some(pool), None) => Some((pool.as_str(), true)), > + (None, None) => None, > + } > } > > /// List all media assigned to the pool > @@ -261,18 +271,16 @@ impl Inventory { > let mut list = Vec::new(); > > for entry in self.map.values() { > - match entry.id.media_set_label { > - Some(ref set) if set.pool == pool => { > - let id = match set.unassigned() { > - true => MediaId { > - label: entry.id.label.clone(), > - media_set_label: None, > - }, > - false => entry.id.clone(), > - }; > - list.push(id); > + if entry.id.pool().as_deref() == Some(pool) { > + match entry.id.media_set_label { What if media_set.pool != label.pool?? > + Some(ref set) if set.unassigned() => list.push(MediaId { > + label: entry.id.label.clone(), > + media_set_label: None, > + }), > + _ => { > + list.push(entry.id.clone()); > + } > } > - _ => continue, // not assigned to any pool or belongs to another pool > } > } > > @@ -294,7 +302,7 @@ impl Inventory { > pub fn list_unassigned_media(&self) -> Vec { > self.map > .values() > - .filter_map(|entry| match entry.id.media_set_label { > + .filter_map(|entry| match entry.id.pool() { > None => Some(entry.id.clone()), > _ => None, > }) > @@ -553,6 +561,7 @@ impl Inventory { > label_text: label_text.to_string(), > uuid: Uuid::generate(), > ctime, > + pool: None, > }; > let uuid = label.uuid.clone(); > > @@ -575,16 +584,15 @@ impl Inventory { > label_text: label_text.to_string(), > uuid: Uuid::generate(), > ctime, > + pool: Some(pool.to_string()), > }; > > let uuid = label.uuid.clone(); > > - let set = MediaSetLabel::new_unassigned(pool, ctime); > - > self.store( > MediaId { > label, > - media_set_label: Some(set), > + media_set_label: None, > }, > false, > ) > @@ -599,6 +607,7 @@ impl Inventory { > label_text: label_text.to_string(), > uuid: Uuid::generate(), > ctime, > + pool: Some(set.pool.clone()), > }; > let uuid = label.uuid.clone(); > > diff --git a/src/tape/media_pool.rs b/src/tape/media_pool.rs > index e1bba9ab..afb27cab 100644 > --- a/src/tape/media_pool.rs > +++ b/src/tape/media_pool.rs > @@ -205,13 +205,9 @@ impl MediaPool { > Some(media_id) => media_id.clone(), > }; > > - if let Some(ref set) = media_id.media_set_label { > - if set.pool != self.name { > - bail!( > - "media does not belong to pool ({} != {})", > - set.pool, > - self.name > - ); > + if let Some(pool) = media_id.pool() { > + if pool != self.name { > + bail!("media does not belong to pool ({} != {})", pool, self.name); > } > } >