* [pbs-devel] [PATCH proxmox-backup 1/2] backup/datastore: really lock manifest on delete @ 2020-12-02 13:19 Dominik Csapak 2020-12-02 13:19 ` [pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run Dominik Csapak 2020-12-02 13:40 ` [pbs-devel] applied: [PATCH proxmox-backup 1/2] backup/datastore: really lock manifest on delete Wolfgang Bumiller 0 siblings, 2 replies; 6+ messages in thread From: Dominik Csapak @ 2020-12-02 13:19 UTC (permalink / raw) To: pbs-devel 'lock_manifest' returns a Result<File, Error> so we always got the result, even when we did not get the lock, but we acted like we had. bubble the locking error up Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> --- src/backup/datastore.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs index e6be1f67..0f74ac3c 100644 --- a/src/backup/datastore.rs +++ b/src/backup/datastore.rs @@ -244,7 +244,7 @@ impl DataStore { let (_guard, _manifest_guard); if !force { _guard = lock_dir_noblock(&full_path, "snapshot", "possibly running or in use")?; - _manifest_guard = self.lock_manifest(backup_dir); + _manifest_guard = self.lock_manifest(backup_dir)?; } log::info!("removing backup snapshot {:?}", full_path); -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run 2020-12-02 13:19 [pbs-devel] [PATCH proxmox-backup 1/2] backup/datastore: really lock manifest on delete Dominik Csapak @ 2020-12-02 13:19 ` Dominik Csapak 2020-12-02 13:50 ` Wolfgang Bumiller 2020-12-02 13:40 ` [pbs-devel] applied: [PATCH proxmox-backup 1/2] backup/datastore: really lock manifest on delete Wolfgang Bumiller 1 sibling, 1 reply; 6+ messages in thread From: Dominik Csapak @ 2020-12-02 13:19 UTC (permalink / raw) To: pbs-devel this fixes the issue that on some filesystems, you cannot recursively remove a directory when you hold a lock on a file inside (e.g. nfs/cifs) it is not really backwards compatible (so during an upgrade, there could be two daemons have the lock), but since the locking was broken before (see previous patch) it should not really matter (also it seems very unlikely that someone will trigger this) Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> --- src/backup/datastore.rs | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs index 0f74ac3c..9cc88906 100644 --- a/src/backup/datastore.rs +++ b/src/backup/datastore.rs @@ -257,6 +257,12 @@ impl DataStore { ) })?; + // the manifest does not exists anymore, we do not need to keep the lock + if let Ok(path) = self.manifest_lock_path(backup_dir) { + // ignore errors + let _ = std::fs::remove_file(path); + } + Ok(()) } @@ -698,13 +704,27 @@ impl DataStore { )) } + fn manifest_lock_path( + &self, + backup_dir: &BackupDir, + ) -> Result<PathBuf, Error> { + + let mut path = PathBuf::from("/run/proxmox-backup/.locks/"); + path.push(self.name()); + path.push(backup_dir.group().backup_type()); + path.push(backup_dir.group().backup_id()); + std::fs::create_dir_all(&path)?; + + path.push(format!( "{}{}", backup_dir.backup_time_string(), &MANIFEST_LOCK_NAME)); + + Ok(path) + } + fn lock_manifest( &self, backup_dir: &BackupDir, ) -> Result<File, Error> { - let mut path = self.base_path(); - path.push(backup_dir.relative_path()); - path.push(&MANIFEST_LOCK_NAME); + let path = self.manifest_lock_path(backup_dir)?; // update_manifest should never take a long time, so if someone else has // the lock we can simply block a bit and should get it soon -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run 2020-12-02 13:19 ` [pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run Dominik Csapak @ 2020-12-02 13:50 ` Wolfgang Bumiller 2020-12-02 13:58 ` Dominik Csapak 0 siblings, 1 reply; 6+ messages in thread From: Wolfgang Bumiller @ 2020-12-02 13:50 UTC (permalink / raw) To: Dominik Csapak; +Cc: pbs-devel On Wed, Dec 02, 2020 at 02:19:57PM +0100, Dominik Csapak wrote: > this fixes the issue that on some filesystems, you cannot recursively > remove a directory when you hold a lock on a file inside (e.g. nfs/cifs) > > it is not really backwards compatible (so during an upgrade, there > could be two daemons have the lock), but since the locking was > broken before (see previous patch) it should not really matter > (also it seems very unlikely that someone will trigger this) > > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> > --- > src/backup/datastore.rs | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs > index 0f74ac3c..9cc88906 100644 > --- a/src/backup/datastore.rs > +++ b/src/backup/datastore.rs > @@ -257,6 +257,12 @@ impl DataStore { > ) > })?; > > + // the manifest does not exists anymore, we do not need to keep the lock > + if let Ok(path) = self.manifest_lock_path(backup_dir) { > + // ignore errors > + let _ = std::fs::remove_file(path); > + } > + > Ok(()) > } > > @@ -698,13 +704,27 @@ impl DataStore { > )) > } > please describe the path in a doc comment here > + fn manifest_lock_path( > + &self, > + backup_dir: &BackupDir, > + ) -> Result<PathBuf, Error> { > + > + let mut path = PathBuf::from("/run/proxmox-backup/.locks/"); why `.locks` and not just `locks`? I don't see the benefit in "hidden" files in `/run`? > + path.push(self.name()); > + path.push(backup_dir.group().backup_type()); > + path.push(backup_dir.group().backup_id()); > + std::fs::create_dir_all(&path)?; Is there a particular reason you use a `PathBuf` here this way? Looks like you could just `format!()` it all the same? Since none of these types are `Path`s to begin with anyway. Since those components are all strings, IMO you could work with a `String` from the start and only convert to PathBuf at the end. Would save you the extra String allocation below. So if I see this right, the file will then be /run/proxmox-backup/.locks/$store/${type}/${id}/${timestamp}.index.json.lck seems reasonable apart from the dot in `.locks` ;-) However, do we really need the directory structure here? Shouldn't a flat `.../locks/${type}.${id}.${timestamp}.index.json` be fine as well? (I don't really mind, it would just be less code ;-) ) > + > + path.push(format!( "{}{}", backup_dir.backup_time_string(), &MANIFEST_LOCK_NAME)); > + > + Ok(path) > + } > + > fn lock_manifest( > &self, > backup_dir: &BackupDir, > ) -> Result<File, Error> { > - let mut path = self.base_path(); > - path.push(backup_dir.relative_path()); > - path.push(&MANIFEST_LOCK_NAME); > + let path = self.manifest_lock_path(backup_dir)?; > > // update_manifest should never take a long time, so if someone else has > // the lock we can simply block a bit and should get it soon > -- > 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run 2020-12-02 13:50 ` Wolfgang Bumiller @ 2020-12-02 13:58 ` Dominik Csapak 2020-12-02 14:07 ` Wolfgang Bumiller 0 siblings, 1 reply; 6+ messages in thread From: Dominik Csapak @ 2020-12-02 13:58 UTC (permalink / raw) To: Wolfgang Bumiller; +Cc: pbs-devel On 12/2/20 2:50 PM, Wolfgang Bumiller wrote: > On Wed, Dec 02, 2020 at 02:19:57PM +0100, Dominik Csapak wrote: >> this fixes the issue that on some filesystems, you cannot recursively >> remove a directory when you hold a lock on a file inside (e.g. nfs/cifs) >> >> it is not really backwards compatible (so during an upgrade, there >> could be two daemons have the lock), but since the locking was >> broken before (see previous patch) it should not really matter >> (also it seems very unlikely that someone will trigger this) >> >> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> >> --- >> src/backup/datastore.rs | 26 +++++++++++++++++++++++--- >> 1 file changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs >> index 0f74ac3c..9cc88906 100644 >> --- a/src/backup/datastore.rs >> +++ b/src/backup/datastore.rs >> @@ -257,6 +257,12 @@ impl DataStore { >> ) >> })?; >> >> + // the manifest does not exists anymore, we do not need to keep the lock >> + if let Ok(path) = self.manifest_lock_path(backup_dir) { >> + // ignore errors >> + let _ = std::fs::remove_file(path); >> + } >> + >> Ok(()) >> } >> >> @@ -698,13 +704,27 @@ impl DataStore { >> )) >> } >> > > please describe the path in a doc comment here ok, but even in a private api? > >> + fn manifest_lock_path( >> + &self, >> + backup_dir: &BackupDir, >> + ) -> Result<PathBuf, Error> { >> + >> + let mut path = PathBuf::from("/run/proxmox-backup/.locks/"); > > why `.locks` and not just `locks`? I don't see the benefit in "hidden" > files in `/run`? yeah you're right, no sense in making this hidden > >> + path.push(self.name()); >> + path.push(backup_dir.group().backup_type()); >> + path.push(backup_dir.group().backup_id()); >> + std::fs::create_dir_all(&path)?; > > Is there a particular reason you use a `PathBuf` here this way? Looks > like you could just `format!()` it all the same? Since none of these > types are `Path`s to begin with anyway. > > Since those components are all strings, IMO you could work with a > `String` from the start and only convert to PathBuf at the end. > > Would save you the extra String allocation below. ok will do > > So if I see this right, the file will then be > /run/proxmox-backup/.locks/$store/${type}/${id}/${timestamp}.index.json.lck > > seems reasonable apart from the dot in `.locks` ;-) > > However, do we really need the directory structure here? > Shouldn't a flat `.../locks/${type}.${id}.${timestamp}.index.json` be > fine as well? (I don't really mind, it would just be less code ;-) ) for now, ids do not really have a length limit besides the fs filename limit of 255 bytes and since i had to factor that out, i did for datastore/type as well (would look even weirder to use something like: .../locks/${datastore}.${type}/${id}/${timestamp}.index.json.lck ) though we probably should limit the id length anyway... > >> + >> + path.push(format!( "{}{}", backup_dir.backup_time_string(), &MANIFEST_LOCK_NAME)); >> + >> + Ok(path) >> + } >> + >> fn lock_manifest( >> &self, >> backup_dir: &BackupDir, >> ) -> Result<File, Error> { >> - let mut path = self.base_path(); >> - path.push(backup_dir.relative_path()); >> - path.push(&MANIFEST_LOCK_NAME); >> + let path = self.manifest_lock_path(backup_dir)?; >> >> // update_manifest should never take a long time, so if someone else has >> // the lock we can simply block a bit and should get it soon >> -- >> 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run 2020-12-02 13:58 ` Dominik Csapak @ 2020-12-02 14:07 ` Wolfgang Bumiller 0 siblings, 0 replies; 6+ messages in thread From: Wolfgang Bumiller @ 2020-12-02 14:07 UTC (permalink / raw) To: Proxmox Backup Server development discussion > On 12/02/2020 2:58 PM Dominik Csapak <d.csapak@proxmox.com> wrote: > > > On 12/2/20 2:50 PM, Wolfgang Bumiller wrote: > > On Wed, Dec 02, 2020 at 02:19:57PM +0100, Dominik Csapak wrote: > >> this fixes the issue that on some filesystems, you cannot recursively > >> remove a directory when you hold a lock on a file inside (e.g. nfs/cifs) > >> > >> it is not really backwards compatible (so during an upgrade, there > >> could be two daemons have the lock), but since the locking was > >> broken before (see previous patch) it should not really matter > >> (also it seems very unlikely that someone will trigger this) > >> > >> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> > >> --- > >> src/backup/datastore.rs | 26 +++++++++++++++++++++++--- > >> 1 file changed, 23 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs > >> index 0f74ac3c..9cc88906 100644 > >> --- a/src/backup/datastore.rs > >> +++ b/src/backup/datastore.rs > >> @@ -257,6 +257,12 @@ impl DataStore { > >> ) > >> })?; > >> > >> + // the manifest does not exists anymore, we do not need to keep the lock > >> + if let Ok(path) = self.manifest_lock_path(backup_dir) { > >> + // ignore errors > >> + let _ = std::fs::remove_file(path); > >> + } > >> + > >> Ok(()) > >> } > >> > >> @@ -698,13 +704,27 @@ impl DataStore { > >> )) > >> } > >> > > > > please describe the path in a doc comment here > > ok, but even in a private api? Depends on what the code looks like in the end. I needed a bit to chew through the .push() and format!() mix since one implicitly separates with a slash while the other obviously does not ;-) > > > > So if I see this right, the file will then be > > /run/proxmox-backup/.locks/$store/${type}/${id}/${timestamp}.index.json.lck > > > > seems reasonable apart from the dot in `.locks` ;-) > > > > However, do we really need the directory structure here? > > Shouldn't a flat `.../locks/${type}.${id}.${timestamp}.index.json` be > > fine as well? (I don't really mind, it would just be less code ;-) ) > > for now, ids do not really have a length limit besides the fs filename > limit of 255 bytes > and since i had to factor that out, i did for datastore/type as well > (would look even weirder to use something like: > .../locks/${datastore}.${type}/${id}/${timestamp}.index.json.lck > ) > > though we probably should limit the id length anyway... 255 definitely seems too huge ;-) ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pbs-devel] applied: [PATCH proxmox-backup 1/2] backup/datastore: really lock manifest on delete 2020-12-02 13:19 [pbs-devel] [PATCH proxmox-backup 1/2] backup/datastore: really lock manifest on delete Dominik Csapak 2020-12-02 13:19 ` [pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run Dominik Csapak @ 2020-12-02 13:40 ` Wolfgang Bumiller 1 sibling, 0 replies; 6+ messages in thread From: Wolfgang Bumiller @ 2020-12-02 13:40 UTC (permalink / raw) To: Dominik Csapak; +Cc: pbs-devel applied this one On Wed, Dec 02, 2020 at 02:19:56PM +0100, Dominik Csapak wrote: > 'lock_manifest' returns a Result<File, Error> so we always got the result, > even when we did not get the lock, but we acted like we had. > > bubble the locking error up > > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> > --- > src/backup/datastore.rs | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs > index e6be1f67..0f74ac3c 100644 > --- a/src/backup/datastore.rs > +++ b/src/backup/datastore.rs > @@ -244,7 +244,7 @@ impl DataStore { > let (_guard, _manifest_guard); > if !force { > _guard = lock_dir_noblock(&full_path, "snapshot", "possibly running or in use")?; > - _manifest_guard = self.lock_manifest(backup_dir); > + _manifest_guard = self.lock_manifest(backup_dir)?; > } > > log::info!("removing backup snapshot {:?}", full_path); > -- > 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-02 14:07 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-02 13:19 [pbs-devel] [PATCH proxmox-backup 1/2] backup/datastore: really lock manifest on delete Dominik Csapak 2020-12-02 13:19 ` [pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run Dominik Csapak 2020-12-02 13:50 ` Wolfgang Bumiller 2020-12-02 13:58 ` Dominik Csapak 2020-12-02 14:07 ` Wolfgang Bumiller 2020-12-02 13:40 ` [pbs-devel] applied: [PATCH proxmox-backup 1/2] backup/datastore: really lock manifest on delete Wolfgang Bumiller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox