* [pbs-devel] [PATCH proxmox 1/2] process_locker: use ofd locking
2023-12-04 13:18 [pbs-devel] [PATCH proxmox{, -backup} 0/2] Move ProcessLocker to tmpfs Gabriel Goller
@ 2023-12-04 13:18 ` Gabriel Goller
2023-12-04 13:18 ` [pbs-devel] [PATCH proxmox-backup 2/2] datastore: store datastore lock on tmpfs Gabriel Goller
2023-12-06 13:29 ` [pbs-devel] [PATCH proxmox{, -backup} 0/2] Move ProcessLocker to tmpfs Gabriel Goller
2 siblings, 0 replies; 4+ messages in thread
From: Gabriel Goller @ 2023-12-04 13:18 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/2] datastore: store datastore lock on tmpfs
2023-12-04 13:18 [pbs-devel] [PATCH proxmox{, -backup} 0/2] Move ProcessLocker to tmpfs Gabriel Goller
2023-12-04 13:18 ` [pbs-devel] [PATCH proxmox 1/2] process_locker: use ofd locking Gabriel Goller
@ 2023-12-04 13:18 ` Gabriel Goller
2023-12-06 13:29 ` [pbs-devel] [PATCH proxmox{, -backup} 0/2] Move ProcessLocker to tmpfs Gabriel Goller
2 siblings, 0 replies; 4+ messages in thread
From: Gabriel Goller @ 2023-12-04 13:18 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [PATCH proxmox{, -backup} 0/2] Move ProcessLocker to tmpfs
2023-12-04 13:18 [pbs-devel] [PATCH proxmox{, -backup} 0/2] Move ProcessLocker to tmpfs Gabriel Goller
2023-12-04 13:18 ` [pbs-devel] [PATCH proxmox 1/2] process_locker: use ofd locking Gabriel Goller
2023-12-04 13:18 ` [pbs-devel] [PATCH proxmox-backup 2/2] datastore: store datastore lock on tmpfs Gabriel Goller
@ 2023-12-06 13:29 ` Gabriel Goller
2 siblings, 0 replies; 4+ messages in thread
From: Gabriel Goller @ 2023-12-06 13:29 UTC (permalink / raw)
To: pbs-devel
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(-)
>
^ permalink raw reply [flat|nested] 4+ messages in thread