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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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