tape: improve locking (lock media-sets)

- new helper: lock_media_set()

- MediaPool: lock media set

- Expose Inventory::new() to avoid double loading

- do not lock pool on restore (only lock media-set)

- change pool lock name to ".pool-{name}"

src/api2/tape/backup.rs

@@ -409,12 +409,10 @@ fn backup_worker(
     let start = std::time::Instant::now();
     let mut summary: TapeBackupJobSummary = Default::default();
 
-    let _lock = MediaPool::lock(status_path, &pool_config.name)?;
-
     task_log!(worker, "update media online status");
     let changer_name = update_media_online_status(&setup.drive)?;
 
-    let pool = MediaPool::with_config(status_path, &pool_config, changer_name)?;
+    let pool = MediaPool::with_config(status_path, &pool_config, changer_name, false)?;
 
     let mut pool_writer = PoolWriter::new(pool, &setup.drive, worker, email)?;
 

src/api2/tape/drive.rs

@@ -53,10 +53,12 @@ use crate::{
     server::WorkerTask,
     tape::{
         TAPE_STATUS_DIR,
-        MediaPool,
         Inventory,
         MediaCatalog,
         MediaId,
+        lock_media_set,
+        lock_media_pool,
+        lock_unassigned_media_pool,
         linux_tape_device_list,
         lookup_device_identification,
         file_formats::{
@@ -373,10 +375,19 @@ pub fn erase_media(
                     );
 
                     let status_path = Path::new(TAPE_STATUS_DIR);
-                    let mut inventory = Inventory::load(status_path)?;
+                    let mut inventory = Inventory::new(status_path);
 
+                    if let Some(MediaSetLabel { ref pool, ref uuid, ..}) =  media_id.media_set_label {
+                        let _pool_lock = lock_media_pool(status_path, pool)?;
+                        let _media_set_lock = lock_media_set(status_path, uuid, None)?;
                         MediaCatalog::destroy(status_path, &media_id.label.uuid)?;
                         inventory.remove_media(&media_id.label.uuid)?;
+                    } else {
+                        let _lock = lock_unassigned_media_pool(status_path)?;
+                        MediaCatalog::destroy(status_path, &media_id.label.uuid)?;
+                        inventory.remove_media(&media_id.label.uuid)?;
+                    };
+
                     handle.erase_media(fast.unwrap_or(true))?;
                 }
             }
@@ -548,29 +559,38 @@ fn write_media_label(
 
     drive.label_tape(&label)?;
 
-    let mut media_set_label = None;
+    let status_path = Path::new(TAPE_STATUS_DIR);
 
-    if let Some(ref pool) = pool {
+    let media_id = if let Some(ref pool) = pool {
         // assign media to pool by writing special media set label
         worker.log(format!("Label media '{}' for pool '{}'", label.label_text, pool));
         let set = MediaSetLabel::with_data(&pool, [0u8; 16].into(), 0, label.ctime, None);
 
         drive.write_media_set_label(&set, None)?;
-        media_set_label = Some(set);
-    } else {
-        worker.log(format!("Label media '{}' (no pool assignment)", label.label_text));
-    }
 
-    let media_id = MediaId { label, media_set_label };
+        let media_id = MediaId { label, media_set_label: Some(set) };
-
-    let status_path = Path::new(TAPE_STATUS_DIR);
 
         // Create the media catalog
         MediaCatalog::overwrite(status_path, &media_id, false)?;
 
-    let mut inventory = Inventory::load(status_path)?;
+        let mut inventory = Inventory::new(status_path);
         inventory.store(media_id.clone(), false)?;
 
+        media_id
+    } else {
+        worker.log(format!("Label media '{}' (no pool assignment)", label.label_text));
+
+        let media_id = MediaId { label, media_set_label: None };
+
+        // Create the media catalog
+        MediaCatalog::overwrite(status_path, &media_id, false)?;
+
+        let mut inventory = Inventory::new(status_path);
+        inventory.store(media_id.clone(), false)?;
+
+        media_id
+    };
+
     drive.rewind()?;
 
     match drive.read_label() {
@@ -705,14 +725,24 @@ pub async fn read_label(
 
                         if let Err(err) = drive.set_encryption(encrypt_fingerprint) {
                             // try, but ignore errors. just log to stderr
-                            eprintln!("uable to load encryption key: {}", err);
+                            eprintln!("unable to load encryption key: {}", err);
                         }
                     }
 
                     if let Some(true) = inventorize {
                         let state_path = Path::new(TAPE_STATUS_DIR);
-                        let mut inventory = Inventory::load(state_path)?;
+                        let mut inventory = Inventory::new(state_path);
+
+                        if let Some(MediaSetLabel { ref pool, ref uuid, ..}) =  media_id.media_set_label {
+                            let _pool_lock = lock_media_pool(state_path, pool)?;
+                            let _lock = lock_media_set(state_path, uuid, None)?;
+                            MediaCatalog::destroy_unrelated_catalog(state_path, &media_id)?;
                             inventory.store(media_id, false)?;
+                        } else {
+                            let _lock = lock_unassigned_media_pool(state_path)?;
+                            MediaCatalog::destroy(state_path, &media_id.label.uuid)?;
+                            inventory.store(media_id, false)?;
+                        };
                     }
 
                     flat
@@ -947,7 +977,17 @@ pub fn update_inventory(
                             continue;
                         }
                         worker.log(format!("inventorize media '{}' with uuid '{}'", label_text, media_id.label.uuid));
+
+                        if let Some(MediaSetLabel { ref pool, ref uuid, ..}) =  media_id.media_set_label {
+                            let _pool_lock = lock_media_pool(state_path, pool)?;
+                            let _lock = lock_media_set(state_path, uuid, None)?;
+                            MediaCatalog::destroy_unrelated_catalog(state_path, &media_id)?;
                             inventory.store(media_id, false)?;
+                        } else {
+                            let _lock = lock_unassigned_media_pool(state_path)?;
+                            MediaCatalog::destroy(state_path, &media_id.label.uuid)?;
+                            inventory.store(media_id, false)?;
+                        };
                     }
                 }
                 changer.unload_media(None)?;
@@ -1237,19 +1277,22 @@ pub fn catalog_media(
 
             let status_path = Path::new(TAPE_STATUS_DIR);
 
-            let mut inventory = Inventory::load(status_path)?;
+            let mut inventory = Inventory::new(status_path);
-            inventory.store(media_id.clone(), false)?;
 
-            let pool = match media_id.media_set_label {
+            let _media_set_lock = match media_id.media_set_label {
                 None => {
                     worker.log("media is empty");
+                    let _lock = lock_unassigned_media_pool(status_path)?;
                     MediaCatalog::destroy(status_path, &media_id.label.uuid)?;
+                    inventory.store(media_id.clone(), false)?;
                     return Ok(());
                 }
                 Some(ref set) => {
                     if set.uuid.as_ref() == [0u8;16] { // media is empty
                         worker.log("media is empty");
+                        let _lock = lock_unassigned_media_pool(status_path)?;
                         MediaCatalog::destroy(status_path, &media_id.label.uuid)?;
+                        inventory.store(media_id.clone(), false)?;
                         return Ok(());
                     }
                     let encrypt_fingerprint = set.encryption_key_fingerprint.clone()
@@ -1257,16 +1300,22 @@ pub fn catalog_media(
 
                     drive.set_encryption(encrypt_fingerprint)?;
 
-                    set.pool.clone()
+                    let _pool_lock = lock_media_pool(status_path, &set.pool)?;
+                    let media_set_lock = lock_media_set(status_path, &set.uuid, None)?;
+
+                    MediaCatalog::destroy_unrelated_catalog(status_path, &media_id)?;
+
+                    inventory.store(media_id.clone(), false)?;
+
+                    media_set_lock
                 }
             };
 
-            let _lock = MediaPool::lock(status_path, &pool)?;
-
             if MediaCatalog::exists(status_path, &media_id.label.uuid) && !force {
                 bail!("media catalog exists (please use --force to overwrite)");
             }
 
+             // fixme: implement fast catalog restore
             restore_media(&worker, &mut drive, &media_id, None, verbose)?;
 
             Ok(())

src/api2/tape/media.rs

@@ -122,7 +122,7 @@ pub async fn list_media(
         let config: MediaPoolConfig = config.lookup("pool", pool_name)?;
 
         let changer_name = None; // assume standalone drive
-        let mut pool = MediaPool::with_config(status_path, &config, changer_name)?;
+        let mut pool = MediaPool::with_config(status_path, &config, changer_name, true)?;
 
         let current_time = proxmox::tools::time::epoch_i64();
 

src/api2/tape/restore.rs

@@ -66,8 +66,8 @@ use crate::{
         TapeRead,
         MediaId,
         MediaCatalog,
-        MediaPool,
         Inventory,
+        lock_media_set,
         file_formats::{
             PROXMOX_BACKUP_MEDIA_LABEL_MAGIC_1_0,
             PROXMOX_BACKUP_SNAPSHOT_ARCHIVE_MAGIC_1_0,
@@ -161,11 +161,14 @@ pub fn restore(
         bail!("no permissions on /tape/drive/{}", drive);
     }
 
-    let status_path = Path::new(TAPE_STATUS_DIR);
-    let inventory = Inventory::load(status_path)?;
-
     let media_set_uuid = media_set.parse()?;
 
+    let status_path = Path::new(TAPE_STATUS_DIR);
+
+    let _lock = lock_media_set(status_path, &media_set_uuid, None)?;
+
+    let inventory = Inventory::load(status_path)?;
+
     let pool = inventory.lookup_media_set_pool(&media_set_uuid)?;
 
     let privs = user_info.lookup_privs(&auth_id, &["tape", "pool", &pool]);
@@ -192,8 +195,6 @@ pub fn restore(
 
             set_tape_device_state(&drive, &worker.upid().to_string())?;
 
-            let _lock = MediaPool::lock(status_path, &pool)?;
-
             let members = inventory.compute_media_set_members(&media_set_uuid)?;
 
             let media_list = members.media_list();

src/tape/inventory.rs

@@ -3,10 +3,30 @@
 //! The Inventory persistently stores the list of known backup
 //! media. A backup media is identified by its 'MediaId', which is the
 //! MediaLabel/MediaSetLabel combination.
+//!
+//! Inventory Locking
+//!
+//! The inventory itself has several methods to update single entries,
+//! but all of them can be considered atomic.
+//!
+//! Pool Locking
+//!
+//! To add/modify media assigned to a pool, we always do
+//! lock_media_pool(). For unassigned media, we call
+//! lock_unassigned_media_pool().
+//!
+//! MediaSet Locking
+//!
+//! To add/remove media from a media set, or to modify catalogs we
+//! always do lock_media_set(). Also, we aquire this lock during
+//! restore, to make sure it is not reused for backups.
+//!
 
 use std::collections::{HashMap, BTreeMap};
 use std::path::{Path, PathBuf};
 use std::os::unix::io::AsRawFd;
+use std::fs::File;
+use std::time::Duration;
 
 use anyhow::{bail, Error};
 use serde::{Serialize, Deserialize};
@@ -78,7 +98,8 @@ impl Inventory {
     pub const MEDIA_INVENTORY_FILENAME: &'static str = "inventory.json";
     pub const MEDIA_INVENTORY_LOCKFILE: &'static str = ".inventory.lck";
 
-    fn new(base_path: &Path) -> Self {
+    /// Create empty instance, no data loaded
+    pub fn new(base_path: &Path) -> Self {
 
         let mut inventory_path = base_path.to_owned();
         inventory_path.push(Self::MEDIA_INVENTORY_FILENAME);
@@ -127,7 +148,7 @@ impl Inventory {
     }
 
     /// Lock the database
-    pub fn lock(&self) -> Result<std::fs::File, Error> {
+    fn lock(&self) -> Result<std::fs::File, Error> {
         let file = open_file_locked(&self.lockfile_path, std::time::Duration::new(10, 0), true)?;
         if cfg!(test) {
             // We cannot use chown inside test environment (no permissions)
@@ -733,6 +754,52 @@ impl Inventory {
 
 }
 
+/// Lock a media pool
+pub fn lock_media_pool(base_path: &Path, name: &str) -> Result<File, Error> {
+    let mut path = base_path.to_owned();
+    path.push(format!(".pool-{}", name));
+    path.set_extension("lck");
+
+    let timeout = std::time::Duration::new(10, 0);
+    let lock = proxmox::tools::fs::open_file_locked(&path, timeout, true)?;
+
+    let backup_user = crate::backup::backup_user()?;
+    fchown(lock.as_raw_fd(), Some(backup_user.uid), Some(backup_user.gid))?;
+
+    Ok(lock)
+}
+
+/// Lock for media not assigned to any pool
+pub fn lock_unassigned_media_pool(base_path: &Path)  -> Result<File, Error> {
+    // lock artificial "__UNASSIGNED__" pool to avoid races
+    lock_media_pool(base_path, "__UNASSIGNED__")
+}
+
+/// Lock a media set
+///
+/// Timeout is 10 seconds by default
+pub fn lock_media_set(
+    base_path: &Path,
+    media_set_uuid: &Uuid,
+    timeout: Option<Duration>,
+) -> Result<File, Error> {
+    let mut path = base_path.to_owned();
+    path.push(format!(".media-set-{}", media_set_uuid));
+    path.set_extension("lck");
+
+    let timeout = timeout.unwrap_or(Duration::new(10, 0));
+    let file = open_file_locked(&path, timeout, true)?;
+    if cfg!(test) {
+        // We cannot use chown inside test environment (no permissions)
+        return Ok(file);
+    }
+
+    let backup_user = crate::backup::backup_user()?;
+    fchown(file.as_raw_fd(), Some(backup_user.uid), Some(backup_user.gid))?;
+
+    Ok(file)
+}
+
 // shell completion helper
 
 /// List of known media uuids

src/tape/media_pool.rs

@@ -8,6 +8,8 @@
 //!
 
 use std::path::{PathBuf, Path};
+use std::fs::File;
+
 use anyhow::{bail, Error};
 use ::serde::{Deserialize, Serialize};
 
@@ -27,6 +29,9 @@ use crate::{
         MediaId,
         MediaSet,
         Inventory,
+        lock_media_set,
+        lock_media_pool,
+        lock_unassigned_media_pool,
         file_formats::{
             MediaLabel,
             MediaSetLabel,
@@ -34,9 +39,6 @@ use crate::{
     }
 };
 
-/// Media Pool lock guard
-pub struct MediaPoolLockGuard(std::fs::File);
-
 /// Media Pool
 pub struct MediaPool {
 
@@ -49,11 +51,16 @@ pub struct MediaPool {
     changer_name: Option<String>,
     force_media_availability: bool,
 
+    // Set this if you do not need to allocate writeable media -  this
+    // is useful for list_media()
+    no_media_set_locking: bool,
+
     encrypt_fingerprint: Option<Fingerprint>,
 
     inventory: Inventory,
 
     current_media_set: MediaSet,
+    current_media_set_lock: Option<File>,
 }
 
 impl MediaPool {
@@ -72,8 +79,15 @@ impl MediaPool {
         retention: RetentionPolicy,
         changer_name: Option<String>,
         encrypt_fingerprint: Option<Fingerprint>,
+        no_media_set_locking: bool, // for list_media()
      ) -> Result<Self, Error> {
 
+        let _pool_lock = if no_media_set_locking {
+            None
+        } else {
+            Some(lock_media_pool(state_path, name)?)
+        };
+
         let inventory = Inventory::load(state_path)?;
 
         let current_media_set = match inventory.latest_media_set(name) {
@@ -81,6 +95,12 @@ impl MediaPool {
             None => MediaSet::new(),
         };
 
+        let current_media_set_lock = if no_media_set_locking {
+            None
+        } else {
+            Some(lock_media_set(state_path, current_media_set.uuid(), None)?)
+        };
+
         Ok(MediaPool {
             name: String::from(name),
             state_path: state_path.to_owned(),
@@ -89,8 +109,10 @@ impl MediaPool {
             changer_name,
             inventory,
             current_media_set,
+            current_media_set_lock,
             encrypt_fingerprint,
             force_media_availability: false,
+            no_media_set_locking,
         })
     }
 
@@ -111,6 +133,7 @@ impl MediaPool {
         state_path: &Path,
         config: &MediaPoolConfig,
         changer_name: Option<String>,
+        no_media_set_locking: bool, // for list_media()
     ) -> Result<Self, Error> {
 
         let allocation = config.allocation.clone().unwrap_or_else(|| String::from("continue")).parse()?;
@@ -129,6 +152,7 @@ impl MediaPool {
             retention,
             changer_name,
             encrypt_fingerprint,
+            no_media_set_locking,
         )
     }
 
@@ -239,7 +263,18 @@ impl MediaPool {
     /// status, so this must not change persistent/saved state.
     ///
     /// Returns the reason why we started a new media set (if we do)
-    pub fn start_write_session(&mut self, current_time: i64) -> Result<Option<String>, Error> {
+    pub fn start_write_session(
+        &mut self,
+        current_time: i64,
+    ) -> Result<Option<String>, Error> {
+
+        let _pool_lock = if self.no_media_set_locking {
+            None
+        } else {
+            Some(lock_media_pool(&self.state_path, &self.name)?)
+        };
+
+        self.inventory.reload()?;
 
         let mut create_new_set = match self.current_set_usable() {
              Err(err) => {
@@ -268,6 +303,14 @@ impl MediaPool {
 
         if create_new_set.is_some() {
             let media_set = MediaSet::new();
+
+            let current_media_set_lock = if self.no_media_set_locking {
+                None
+            } else {
+                Some(lock_media_set(&self.state_path, media_set.uuid(), None)?)
+            };
+
+            self.current_media_set_lock = current_media_set_lock;
             self.current_media_set = media_set;
         }
 
@@ -327,6 +370,10 @@ impl MediaPool {
 
     fn add_media_to_current_set(&mut self, mut media_id: MediaId, current_time: i64) -> Result<(), Error> {
 
+        if self.current_media_set_lock.is_none() {
+            bail!("add_media_to_current_set: media set is not locked - internal error");
+        }
+
         let seq_nr = self.current_media_set.media_list().len() as u64;
 
         let pool = self.name.clone();
@@ -357,6 +404,10 @@ impl MediaPool {
     /// Allocates a writable media to the current media set
     pub fn alloc_writable_media(&mut self, current_time: i64) -> Result<Uuid, Error> {
 
+        if self.current_media_set_lock.is_none() {
+            bail!("alloc_writable_media: media set is not locked - internal error");
+        }
+
         let last_is_writable = self.current_set_usable()?;
 
         if last_is_writable {
@@ -367,6 +418,11 @@ impl MediaPool {
 
         // try to find empty media in pool, add to media set
 
+        { // limit pool lock scope
+            let _pool_lock = lock_media_pool(&self.state_path, &self.name)?;
+
+            self.inventory.reload()?;
+
             let media_list = self.list_media();
 
             let mut empty_media = Vec::new();
@@ -431,17 +487,26 @@ impl MediaPool {
                 res
             });
 
-        if let Some(media) = expired_media.pop() {
+            while let Some(media) = expired_media.pop() {
+                // check if we can modify the media-set (i.e. skip
+                // media used by a restore job)
+                if let Ok(_media_set_lock) = lock_media_set(
+                    &self.state_path,
+                    &media.media_set_label().unwrap().uuid,
+                    Some(std::time::Duration::new(0, 0)), // do not wait
+                ) {
                     println!("reuse expired media '{}'", media.label_text());
                     let uuid = media.uuid().clone();
                     self.add_media_to_current_set(media.into_id(), current_time)?;
                     return Ok(uuid);
                 }
+            }
+        }
 
         println!("no expired media in pool, try to find unassigned/free media");
 
         // try unassigned media
-        let _lock = Self::lock_unassigned_media_pool(&self.state_path)?;
+        let _lock = lock_unassigned_media_pool(&self.state_path)?;
 
         self.inventory.reload()?;
 
@@ -561,23 +626,6 @@ impl MediaPool {
         self.inventory.generate_media_set_name(media_set_uuid, template)
     }
 
-    /// Lock the pool
-    pub fn lock(base_path: &Path, name: &str) -> Result<MediaPoolLockGuard, Error> {
-        let mut path = base_path.to_owned();
-        path.push(format!(".{}", name));
-        path.set_extension("lck");
-
-        let timeout = std::time::Duration::new(10, 0);
-        let lock = proxmox::tools::fs::open_file_locked(&path, timeout, true)?;
-
-        Ok(MediaPoolLockGuard(lock))
-    }
-
-    /// Lock for media not assigned to any pool
-    pub fn lock_unassigned_media_pool(base_path: &Path)  -> Result<MediaPoolLockGuard, Error> {
-        // lock artificial "__UNASSIGNED__" pool to avoid races
-        MediaPool::lock(base_path, "__UNASSIGNED__")
-    }
 }
 
 /// Backup media