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

[pbs-devel] [PATCH v2 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

Changes since version 1:
 - create directory path if it doesn't exist. (f.e. on my instance the `locks` directory 
   did not exist yet)



proxmox:

Gabriel Goller (1):
  process_locker: use ofd locking

 proxmox-sys/src/process_locker.rs | 26 ++++++++++++++++----------
 1 file changed, 16 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, 23 insertions(+), 14 deletions(-)

-- 
murpp v0.4.0

[pbs-devel] [PATCH v2 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])

Added a `create_dir_all()` call to create the folders if they are not
existing.

[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 | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/proxmox-sys/src/process_locker.rs b/proxmox-sys/src/process_locker.rs
index 4874da8..58deacb 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);
         }
 
@@ -108,6 +108,12 @@ impl 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 parent = lockfile
+            .as_ref()
+            .parent()
+            .ok_or(anyhow::anyhow!("Failed getting parent dir of locking file"))?;
+        std::fs::create_dir_all(parent)?;
+
         let file = std::fs::OpenOptions::new()
             .create(true)
             .read(true)
@@ -132,7 +138,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 v2 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 v2 proxmox{, -backup} 0/2] Move ProcessLocker to tmpfs

[Fabian Grünbichler]

> Gabriel Goller <g.goller@proxmox.com> hat am 06.12.2023 14:28 CET geschrieben:
> 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].

this might be moot, since most likely both patches go in at the same time, is this change reload/upgrade-compatible? i.e., if an old proxmox-backup(-proxy) process is (still) running that has the lock open with F_SETLK, and the new one obtains it using F_OFD_SETLK, is the behaviour still correct? (the other direction might be interesting too, but can only happen on an unsupported downgrade)

> 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.

the same question applies here with the changed path and reloads. if not (and this seems rather likely if the path changes), we might need an explicit hand-over and compat code that obtains both locks at least as long as any old processes are still running?

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

[Gabriel Goller]

On 12/6/23 14:41, Fabian Grünbichler wrote:
>>
>> Gabriel Goller <g.goller@proxmox.com> hat am 06.12.2023 14:28 CET 
>> geschrieben: 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].
>>
> this might be moot, since most likely both patches go in at the same 
> time, is this change reload/upgrade-compatible? i.e., if an old 
> proxmox-backup(-proxy) process is (still) running that has the lock 
> open with F_SETLK, and the new one obtains it using F_OFD_SETLK, is 
> the behaviour still correct? (the other direction might be interesting 
> too, but can only happen on an unsupported downgrade)
>
Just spoke with Stefan Sterz about this and we will probably 
apply/release this with a major version bump (kernel update), so that 
the user
is forced to reboot the system (same as with his tmpfs locking series).
I don't think there is another way, because the lockfiles get moved to 
another dir. Although F_SETLK and F_OFD_SETLK should be compatible,
so having one process use F_SETLK and another F_OFD_SETLK *should* still 
work (don't take my word for it though).

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

[Fabian Grünbichler]

> Gabriel Goller <g.goller@proxmox.com> hat am 06.12.2023 14:56 CET geschrieben:
> On 12/6/23 14:41, Fabian Grünbichler wrote:
> >>
> >> Gabriel Goller <g.goller@proxmox.com> hat am 06.12.2023 14:28 CET 
> >> geschrieben: 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].
> >>
> > this might be moot, since most likely both patches go in at the same 
> > time, is this change reload/upgrade-compatible? i.e., if an old 
> > proxmox-backup(-proxy) process is (still) running that has the lock 
> > open with F_SETLK, and the new one obtains it using F_OFD_SETLK, is 
> > the behaviour still correct? (the other direction might be interesting 
> > too, but can only happen on an unsupported downgrade)
> >
> Just spoke with Stefan Sterz about this and we will probably 
> apply/release this with a major version bump (kernel update), so that 
> the user
> is forced to reboot the system (same as with his tmpfs locking series).
> I don't think there is another way, because the lockfiles get moved to 
> another dir. Although F_SETLK and F_OFD_SETLK should be compatible,
> so having one process use F_SETLK and another F_OFD_SETLK *should* still 
> work (don't take my word for it though).

that doesn't really help though, unless we also add machinery to detect the missing reboot and block any process-locker-requiring stuff in the new process until it has happened? or we make "set all datastores to read-only or offline" a requirement for upgrading from 3 to 4, instead of optional like for 2 to 3[0]. otherwise even just the time between "postinst of PBS package is called" to "upgrade of whole system is fully done" can be big enough to cause a problem..

0: https://pbs.proxmox.com/wiki/index.php/Upgrade_from_2_to_3#Optional:_Enable_Maintenance_Mode

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

[Gabriel Goller]

On 12/6/23 15:14, Fabian Grünbichler wrote:
>> Gabriel Goller <g.goller@proxmox.com> hat am 06.12.2023 14:56 CET geschrieben:
>> On 12/6/23 14:41, Fabian Grünbichler wrote:
>>> [..]
>> Just spoke with Stefan Sterz about this and we will probably
>> apply/release this with a major version bump (kernel update), so that
>> the user
>> is forced to reboot the system (same as with his tmpfs locking series).
>> I don't think there is another way, because the lockfiles get moved to
>> another dir. Although F_SETLK and F_OFD_SETLK should be compatible,
>> so having one process use F_SETLK and another F_OFD_SETLK *should* still
>> work (don't take my word for it though).
> that doesn't really help though, unless we also add machinery to detect the missing reboot and block any process-locker-requiring stuff in the new process until it has happened? or we make "set all datastores to read-only or offline" a requirement for upgrading from 3 to 4, instead of optional like for 2 to 3[0]. otherwise even just the time between "postinst of PBS package is called" to "upgrade of whole system is fully done" can be big enough to cause a problem..
>
> 0: https://pbs.proxmox.com/wiki/index.php/Upgrade_from_2_to_3#Optional:_Enable_Maintenance_Mode
That's a good idea.
Optionally we could also somehow remove the `.lock` file in the 
datastore and remove the `.create(true)`,
so that creating the 'old' `.lock` file will fail?
Although not sure how we would do this...

But can we also somehow force the user to have the datastore in a 
maintenance mode? I guess not...

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

[Fabian Grünbichler]

> Gabriel Goller <g.goller@proxmox.com> hat am 06.12.2023 15:21 CET geschrieben:
> 
>  
> On 12/6/23 15:14, Fabian Grünbichler wrote:
> >> Gabriel Goller <g.goller@proxmox.com> hat am 06.12.2023 14:56 CET geschrieben:
> >> On 12/6/23 14:41, Fabian Grünbichler wrote:
> >>> [..]
> >> Just spoke with Stefan Sterz about this and we will probably
> >> apply/release this with a major version bump (kernel update), so that
> >> the user
> >> is forced to reboot the system (same as with his tmpfs locking series).
> >> I don't think there is another way, because the lockfiles get moved to
> >> another dir. Although F_SETLK and F_OFD_SETLK should be compatible,
> >> so having one process use F_SETLK and another F_OFD_SETLK *should* still
> >> work (don't take my word for it though).
> > that doesn't really help though, unless we also add machinery to detect the missing reboot and block any process-locker-requiring stuff in the new process until it has happened? or we make "set all datastores to read-only or offline" a requirement for upgrading from 3 to 4, instead of optional like for 2 to 3[0]. otherwise even just the time between "postinst of PBS package is called" to "upgrade of whole system is fully done" can be big enough to cause a problem..
> >
> > 0: https://pbs.proxmox.com/wiki/index.php/Upgrade_from_2_to_3#Optional:_Enable_Maintenance_Mode
> That's a good idea.
> Optionally we could also somehow remove the `.lock` file in the 
> datastore and remove the `.create(true)`,
> so that creating the 'old' `.lock` file will fail?
> Although not sure how we would do this...

I don't see that working with the old code still running? and if the old code is not running (anymore), we don't have the problem anyway ;)

> But can we also somehow force the user to have the datastore in a 
> maintenance mode? I guess not...

forcing is hard, but we could both
- make it a required step in the upgrade guide (it's not our fault then if the user didn't follow it ;))
- check in post-inst, print a big fat warning, and *not reload* but just keep the old process running

that way, the user will only get an actual 4.x process running if they manually reload or restart the service(s), or reboot the machine:
- reload could be handled by touching a flag file in tmpfs in postinst if the maintenance-mode pre-requisites are not met, and refusing to reload if it is found (that part could already be added to 3.x if needed)
- restart and reboot are okay, since in both cases the old process is killed/stopped, and no lock path mismatch can happen

still, the other variant with passing a long the "need to double-lock" flag would also not be too complex I think if we don't want to wait that long - postinst touches a flag file in tmpfs before reloading (on the first upgrade from a pre-change version), as long as that file exists the new code uses a compat mode that obtains both old and new lock paths. once the flag file is gone (reboot, or process detects no more old processes are around), the compat code path becomes dead code at runtime, and can be removed altogether with the next major release.

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

[Thomas Lamprecht]

Am 06/12/2023 um 14:41 schrieb Fabian Grünbichler:
> 
>> Gabriel Goller <g.goller@proxmox.com> hat am 06.12.2023 14:28 CET geschrieben:
>> 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].
> 
> this might be moot, since most likely both patches go in at the same time, is this change reload/upgrade-compatible? i.e., if an old proxmox-backup(-proxy) process is (still) running that has the lock open with F_SETLK, and the new one obtains it using F_OFD_SETLK, is the behaviour still correct? (the other direction might be interesting too, but can only happen on an unsupported downgrade)
> 
>> 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.
> 
> the same question applies here with the changed path and reloads. if not (and this seems rather likely if the path changes), we might need an explicit hand-over and compat code that obtains both locks at least as long as any old processes are still running?
> 

Wolfgang and I discussed this last week or so, and we'd use a flag living
in `/run/proxmox-backup` touched via proxmox-backup-server.postinst on
upgrade to signal that the new locking should not be used yet, after reboot
all old daemons are gone and so is the flag, so the new locking can be used.

Manual downgrades we don't care for, as those need special attention anyway.
The duplicate code can then be ripped out in the next major release.

Wolfgang has a PoC about this on his staff repo IIRC.

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

[Gabriel Goller]

On 12/6/23 15:36, Thomas Lamprecht wrote:
> Am 06/12/2023 um 14:41 schrieb Fabian Grünbichler:
>> [..]
> Wolfgang and I discussed this last week or so, and we'd use a flag living
> in `/run/proxmox-backup` touched via proxmox-backup-server.postinst on
> upgrade to signal that the new locking should not be used yet, after reboot
> all old daemons are gone and so is the flag, so the new locking can be used.
>
> Manual downgrades we don't care for, as those need special attention anyway.
> The duplicate code can then be ripped out in the next major release.
>
> Wolfgang has a PoC about this on his staff repo IIRC.
We should also add a section in the upgrade guide on how to upgrade 
*without*
rebooting, so stopping all old processes, removing the flag manually, 
updating,
then starting the new process. (I'd guess a lot of people don't want to 
reboot)

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

[Thomas Lamprecht]

Am 06/12/2023 um 15:46 schrieb Gabriel Goller:
> On 12/6/23 15:36, Thomas Lamprecht wrote:
>> Am 06/12/2023 um 14:41 schrieb Fabian Grünbichler:
>>> [..]
>> Wolfgang and I discussed this last week or so, and we'd use a flag living
>> in `/run/proxmox-backup` touched via proxmox-backup-server.postinst on
>> upgrade to signal that the new locking should not be used yet, after reboot
>> all old daemons are gone and so is the flag, so the new locking can be used.
>>
>> Manual downgrades we don't care for, as those need special attention anyway.
>> The duplicate code can then be ripped out in the next major release.
>>
>> Wolfgang has a PoC about this on his staff repo IIRC.
> We should also add a section in the upgrade guide on how to upgrade 
> *without*
> rebooting, so stopping all old processes, removing the flag manually, 
> updating,
> then starting the new process. (I'd guess a lot of people don't want to 
> reboot)

I'd not bother too much with that, besides discoverability, these are
actions that can lead to data loss if done slightly wrong (reload
instead of restart)..

If any new big feature is better off with the new locking, like the
removable datastore is, then that feature should actively check and
warn/hint if the old mechanism is still active and the release note
can contain a hint about requiring a reboot.

Long uptimes are most of the time a bad sign anyway (kernel live
patching isn't a 100% replacement to full updates via reboot).