From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 4FD249885D for ; Wed, 15 Nov 2023 15:32:58 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 314B4862D for ; Wed, 15 Nov 2023 15:32:28 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 15 Nov 2023 15:32:27 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 61A8A430D5 for ; Wed, 15 Nov 2023 15:32:27 +0100 (CET) Message-ID: Date: Wed, 15 Nov 2023 15:32:26 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Gabriel Goller To: pbs-devel@lists.proxmox.com Reply-To: Proxmox Backup Server development discussion References: <20231115110103.92457-1-g.goller@proxmox.com> Content-Language: en-US In-Reply-To: <20231115110103.92457-1-g.goller@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.249 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox] sys: open process_locker lockfile lazy X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Nov 2023 14:32:58 -0000 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 > --- > 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, > 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>(lockfile: P) -> Result>, 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>, 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); > } >