[pbs-devel] [PATCH proxmox-backup 1/2] backup/datastore: really lock manifest on delete

[pbs-devel] [PATCH proxmox-backup 1/2] backup/datastore: really lock manifest on delete

[Dominik Csapak]

'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

[pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run

[Dominik Csapak]

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

[pbs-devel] applied: [PATCH proxmox-backup 1/2] backup/datastore: really lock manifest on delete

[Wolfgang Bumiller]

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

Re: [pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run

[Wolfgang Bumiller]

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

Re: [pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run

[Dominik Csapak]

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

Re: [pbs-devel] [PATCH proxmox-backup 2/2] backup/datastore: move manifest locking to /run

[Wolfgang Bumiller]

> 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 ;-)