[pbs-devel] [PATCH proxmox{, -backup} 0/2] Move ProcessLocker to tmpfs

[pbs-devel] [PATCH proxmox{, -backup} 0/2] Move ProcessLocker to tmpfs

[Gabriel Goller]

This moves the `ProcessLocker`'s `.lock` file to `/run/proxmox-backup/locks` (tmpfs).

The first patch only converts all the `F_SETLK` flags to `F_OFD_SETLK` flags. This
changes normal locks, which are based on the process, to locks based on an open file
descriptor. This actually doesn't change anything, because we use mutexes, so the
lock is already thread-safe. It would be cleaner though and would safe us from
weird quirks like closing the lock-file which would drop all the locks when using
the POSIX `F_SETLK`. See more here [0].

The second patch changes the location of the `.lock` file to the `/run/proxmox-backup/locks`
tmpfs directory. Like this we don't need to lazy-lock anything and we can keep the lockfile
open all the time. Unmounting datastores is now possible as the lock file is not on the
datastore mount anymore.

Another thing is noticed is that datastores that are not available (e.g. have been unmounted)
don't display an error on the ui. That means the only way to see if a datastore is online is
to either start a gc or verify run. An idea for a future patch would be to check if the
datastore exists and (maybe) even automatically set the maintenance mode to `offline` if it
doesn't exist?

[0]: https://man7.org/linux/man-pages/man2/fcntl.2.html



proxmox:

Gabriel Goller (1):
  process_locker: use ofd locking

 proxmox-sys/src/process_locker.rs | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)


proxmox-backup:

Gabriel Goller (1):
  datastore: store datastore lock on tmpfs

 pbs-datastore/src/chunk_store.rs | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)


Summary over all repositories:
  2 files changed, 17 insertions(+), 14 deletions(-)

-- 
murpp v0.4.0

[pbs-devel] [PATCH proxmox 1/2] process_locker: use ofd locking

[Gabriel Goller]

Instead of using normal fcntl locking, use the OFD (open file
descriptor) flags [0].
This blocks the file on the file-descriptor level, not on the
process-level.

This solves two problems:

-  If a process closes any file descriptor referring to a file,
  then all of the process's locks on that file are released,
  regardless of the file descriptor(s) on which the locks were
  obtained.  This is bad: it means that a process can lose its
  locks on a file such as /etc/passwd or /etc/mtab when for some
  reason a library function decides to open, read, and close the
  same file.

-  The threads in a process share locks.  In other words, a
  multithreaded program can't use record locking to ensure that
  threads don't simultaneously access the same region of a file.

(Copied from [0])

[0]: https://man7.org/linux/man-pages/man2/fcntl.2.html

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-sys/src/process_locker.rs | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/proxmox-sys/src/process_locker.rs b/proxmox-sys/src/process_locker.rs
index 4874da8..ac2a8a2 100644
--- a/proxmox-sys/src/process_locker.rs
+++ b/proxmox-sys/src/process_locker.rs
@@ -1,7 +1,7 @@
 //! Inter-process reader-writer lock builder.
 //!
 //! This implementation uses fcntl record locks with non-blocking
-//! F_SETLK command (never blocks).
+//! F_OFD_SETLK command (never blocks).
 //!
 //! We maintain a map of shared locks with time stamps, so you can get
 //! the timestamp for the oldest open lock with
@@ -13,8 +13,6 @@ use std::sync::{Arc, Mutex};
 
 use anyhow::{bail, Error};
 
-// fixme: use F_OFD_ locks when implemented with nix::fcntl
-
 // Note: flock lock conversion is not atomic, so we need to use fcntl
 
 /// Inter-process reader-writer lock
@@ -53,9 +51,10 @@ 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))
-            {
+            if let Err(err) = nix::fcntl::fcntl(
+                data.file.as_raw_fd(),
+                nix::fcntl::FcntlArg::F_OFD_SETLKW(&op),
+            ) {
                 panic!("unable to drop writer lock - {}", err);
             }
         }
@@ -93,9 +92,10 @@ 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))
-        {
+        if let Err(err) = nix::fcntl::fcntl(
+            data.file.as_raw_fd(),
+            nix::fcntl::FcntlArg::F_OFD_SETLKW(&op),
+        ) {
             panic!("unable to drop exclusive lock - {}", err);
         }
 
@@ -132,7 +132,7 @@ impl ProcessLocker {
             l_pid: 0,
         };
 
-        nix::fcntl::fcntl(file.as_raw_fd(), nix::fcntl::FcntlArg::F_SETLK(&op))?;
+        nix::fcntl::fcntl(file.as_raw_fd(), nix::fcntl::FcntlArg::F_OFD_SETLK(&op))?;
 
         Ok(())
     }
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 2/2] datastore: store datastore lock on tmpfs

[Gabriel Goller]

Store the lockfiles that lock the whole datastore (which get created on
startup) on tmpfs in `/run/proxmox-backup/locks`.
This allows datastores to always be unmounted without having to restart.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index fb282749..41332db3 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -18,6 +18,8 @@ use crate::file_formats::{
 };
 use crate::DataBlob;
 
+pub const DATASTORE_LOCKS_DIR: &str = "/run/proxmox-backup/locks";
+
 /// File system based chunk store
 pub struct ChunkStore {
     name: String, // used for error reporting
@@ -124,7 +126,7 @@ impl ChunkStore {
         }
 
         // create lock file with correct owner/group
-        let lockfile_path = Self::lockfile_path(&base);
+        let lockfile_path = Self::lockfile_path(name);
         proxmox_sys::fs::replace_file(lockfile_path, b"", options.clone(), false)?;
 
         // create 64*1024 subdirs
@@ -153,8 +155,9 @@ impl ChunkStore {
         Self::open(name, base, sync_level)
     }
 
-    fn lockfile_path<P: Into<PathBuf>>(base: P) -> PathBuf {
-        let mut lockfile_path: PathBuf = base.into();
+    fn lockfile_path(base: &str) -> PathBuf {
+        let mut lockfile_path: PathBuf = DATASTORE_LOCKS_DIR.into();
+        lockfile_path.push(base);
         lockfile_path.push(".lock");
         lockfile_path
     }
@@ -181,7 +184,7 @@ impl ChunkStore {
             bail!("unable to open chunk store '{name}' at {chunk_dir:?} - {err}");
         }
 
-        let lockfile_path = Self::lockfile_path(&base);
+        let lockfile_path = Self::lockfile_path(name);
 
         let locker = ProcessLocker::new(lockfile_path)?;
 
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox{, -backup} 0/2] Move ProcessLocker to tmpfs

[Gabriel Goller]

Submitted a new patch!

On 12/4/23 14:18, Gabriel Goller wrote:
> This moves the `ProcessLocker`'s `.lock` file to `/run/proxmox-backup/locks` (tmpfs).
>
> The first patch only converts all the `F_SETLK` flags to `F_OFD_SETLK` flags. This
> changes normal locks, which are based on the process, to locks based on an open file
> descriptor. This actually doesn't change anything, because we use mutexes, so the
> lock is already thread-safe. It would be cleaner though and would safe us from
> weird quirks like closing the lock-file which would drop all the locks when using
> the POSIX `F_SETLK`. See more here [0].
>
> The second patch changes the location of the `.lock` file to the `/run/proxmox-backup/locks`
> tmpfs directory. Like this we don't need to lazy-lock anything and we can keep the lockfile
> open all the time. Unmounting datastores is now possible as the lock file is not on the
> datastore mount anymore.
>
> Another thing is noticed is that datastores that are not available (e.g. have been unmounted)
> don't display an error on the ui. That means the only way to see if a datastore is online is
> to either start a gc or verify run. An idea for a future patch would be to check if the
> datastore exists and (maybe) even automatically set the maintenance mode to `offline` if it
> doesn't exist?
>
> [0]: https://man7.org/linux/man-pages/man2/fcntl.2.html
>
>
>
> proxmox:
>
> Gabriel Goller (1):
>    process_locker: use ofd locking
>
>   proxmox-sys/src/process_locker.rs | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
>
> proxmox-backup:
>
> Gabriel Goller (1):
>    datastore: store datastore lock on tmpfs
>
>   pbs-datastore/src/chunk_store.rs | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
>
> Summary over all repositories:
>    2 files changed, 17 insertions(+), 14 deletions(-)
>