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

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

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