Re: [pbs-devel] [PATCH proxmox] sys: open process_locker lockfile lazy

[Gabriel Goller <g.goller@proxmox.com>]

Forgot some debug prints :)
Submitted a new patch.

On 11/15/23 12:01, Gabriel Goller wrote:
> When setting a datastore in maintenance mode (offline or read-only) we
> should be able to unmount it. This isn't possible because the
> `ChunkReader` has a `ProcessLocker` instance that holds an open
> file descriptor to (f.e.) `/mnt/datastore/test1/.lock`.
>
> The `ChunkReader` is created at startup, so if the datastore is not set
> to a maintenance mode at startup, we always have the lockfile open.
> Now we create/open the lockfile lazy, when a shared lock or a exclusive
> lock is wanted. Like this, we can set a datastore to 'offline' and
> unmount it without restarting the proxmox-backup service.
>
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>   proxmox-sys/src/process_locker.rs | 79 +++++++++++++++++++++++--------
>   1 file changed, 58 insertions(+), 21 deletions(-)
>
> diff --git a/proxmox-sys/src/process_locker.rs b/proxmox-sys/src/process_locker.rs
> index 4874da8..b612317 100644
> --- a/proxmox-sys/src/process_locker.rs
> +++ b/proxmox-sys/src/process_locker.rs
> @@ -8,7 +8,9 @@
>   //! `oldest_shared_lock()`.
>   
>   use std::collections::HashMap;
> +use std::fs::File;
>   use std::os::unix::io::AsRawFd;
> +use std::path::PathBuf;
>   use std::sync::{Arc, Mutex};
>   
>   use anyhow::{bail, Error};
> @@ -19,7 +21,8 @@ use anyhow::{bail, Error};
>   
>   /// Inter-process reader-writer lock
>   pub struct ProcessLocker {
> -    file: std::fs::File,
> +    path: PathBuf,
> +    file_descriptor: Option<File>,
>       exclusive: bool,
>       writers: usize,
>       next_guard_id: u64,
> @@ -53,11 +56,16 @@ impl Drop for ProcessLockSharedGuard {
>                   l_pid: 0,
>               };
>   
> -            if let Err(err) =
> -                nix::fcntl::fcntl(data.file.as_raw_fd(), nix::fcntl::FcntlArg::F_SETLKW(&op))
> -            {
> -                panic!("unable to drop writer lock - {}", err);
> +            if let Some(file) = &data.file_descriptor {
> +                if let Err(err) =
> +                    nix::fcntl::fcntl(file.as_raw_fd(), nix::fcntl::FcntlArg::F_SETLKW(&op))
> +                {
> +                    panic!("unable to drop writer lock - {}", err);
> +                }
> +            } else {
> +                panic!("file descriptor has been closed while locking");
>               }
> +            data.file_descriptor = None;
>           }
>           if data.writers > 0 {
>               data.writers -= 1;
> @@ -93,29 +101,31 @@ impl Drop for ProcessLockExclusiveGuard {
>               l_pid: 0,
>           };
>   
> -        if let Err(err) =
> -            nix::fcntl::fcntl(data.file.as_raw_fd(), nix::fcntl::FcntlArg::F_SETLKW(&op))
> -        {
> -            panic!("unable to drop exclusive lock - {}", err);
> +        if let Some(file) = &data.file_descriptor {
> +            if let Err(err) =
> +                nix::fcntl::fcntl(file.as_raw_fd(), nix::fcntl::FcntlArg::F_SETLKW(&op))
> +            {
> +                panic!("unable to drop exclusive lock - {}", err);
> +            }
> +        } else {
> +            panic!("file descriptor has been closed while locking");
>           }
>   
> +        println!("closing exclusive file lock");
> +        data.file_descriptor = None;
>           data.exclusive = false;
>       }
>   }
>   
>   impl ProcessLocker {
> -    /// Create a new instance for the specified file.
> +    /// Create a new instance of the ProcessLocker.
>       ///
> -    /// This simply creates the file if it does not exist.
> -    pub fn new<P: AsRef<std::path::Path>>(lockfile: P) -> Result<Arc<Mutex<Self>>, Error> {
> -        let file = std::fs::OpenOptions::new()
> -            .create(true)
> -            .read(true)
> -            .write(true)
> -            .open(lockfile)?;
> -
> +    /// Does not check if the lockfile exists. The lockfile gets opened/created
> +    /// when using [Self::try_shared_lock()] or [Self::try_exclusive_lock()].
> +    pub fn new(lockfile: PathBuf) -> Result<Arc<Mutex<Self>>, Error> {
>           Ok(Arc::new(Mutex::new(Self {
> -            file,
> +            path: lockfile,
> +            file_descriptor: None,
>               exclusive: false,
>               writers: 0,
>               next_guard_id: 0,
> @@ -144,7 +154,20 @@ impl ProcessLocker {
>           let mut data = locker.lock().unwrap();
>   
>           if data.writers == 0 && !data.exclusive {
> -            if let Err(err) = Self::try_lock(&data.file, libc::F_RDLCK) {
> +            if data.file_descriptor.is_none() {
> +                let file = std::fs::OpenOptions::new()
> +                    .create(true)
> +                    .read(true)
> +                    .write(true)
> +                    .open(&data.path)?;
> +                data.file_descriptor = Some(file);
> +            }
> +            if let Err(err) = Self::try_lock(
> +                data.file_descriptor
> +                    .as_ref()
> +                    .expect("unable to open lockfile"),
> +                libc::F_RDLCK,
> +            ) {
>                   bail!("unable to get shared lock - {}", err);
>               }
>           }
> @@ -187,7 +210,21 @@ impl ProcessLocker {
>               bail!("already locked exclusively");
>           }
>   
> -        if let Err(err) = Self::try_lock(&data.file, libc::F_WRLCK) {
> +        if data.file_descriptor.is_none() {
> +            let file = std::fs::OpenOptions::new()
> +                .create(true)
> +                .read(true)
> +                .write(true)
> +                .open(&data.path)?;
> +            data.file_descriptor = Some(file);
> +        }
> +        println!("locking exclusive lock: {:?}", &data.file_descriptor);
> +        if let Err(err) = Self::try_lock(
> +            data.file_descriptor
> +                .as_ref()
> +                .expect("unable to open lockfile"),
> +            libc::F_WRLCK,
> +        ) {
>               bail!("unable to get exclusive lock - {}", err);
>           }
>