Re: [pbs-devel] [PATCH proxmox-backup] fix #4823: datastore: ignore vanished files when walking directory

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 05/09/2023 11:37, Gabriel Goller wrote:
> When walking through a datastore on a GC run, it can
> happen that the snapshot is deleted, and then walked over.
> For example:
> - read dir entry for group
> - walk entries (snapshots)
> - snapshot X is removed/pruned
> - walking reaches snapshot X, but ENOENT
> Previously we bailed here, now we just ignore it.
> 

looks mostly fine, some style nits and a actual comment inline.

> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index fe75d9b5..d135ad90 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -869,18 +869,24 @@ impl DataStore {
>          let handle_entry_err = |err: walkdir::Error| {
>              if let Some(inner) = err.io_error() {
>                  if let Some(path) = err.path() {
> -                    if inner.kind() == io::ErrorKind::PermissionDenied {
> -                        // only allow to skip ext4 fsck directory, avoid GC if, for example,
> -                        // a user got file permissions wrong on datastore rsync to new server
> -                        if err.depth() > 1 || !path.ends_with("lost+found") {
> -                            bail!("cannot continue garbage-collection safely, permission denied on: {:?}", path)
> +                    match inner.kind() {
> +                        io::ErrorKind::PermissionDenied => {

that's some deep indentation level.. not a must, but maybe you find
some good/simple way to refactor some of this to make it a bit less
crowded here (if, then in a separate patch please) 

> +                            // only allow to skip ext4 fsck directory, avoid GC if, for example,
> +                            // a user got file permissions wrong on datastore rsync to new server
> +                            if err.depth() > 1 || !path.ends_with("lost+found") {
> +                                bail!("cannot continue garbage-collection safely, permission denied on: {:?}", path)
> +                            }
> +                        }
> +                        io::ErrorKind::NotFound => {
> +                            // ignore vanished file

would be still good to log that here, at least at debug level
if it can be noisy; but as there wasn't many that run into this
in the four years of PBS existing I'd guess a always visible
level is fine as long as the log message doesn't sounds scary. 

> +                        }
> +                        _ => {
> +                            bail!(
> +                                "unexpected error on datastore traversal: {} - {:?}",
> +                                inner,
> +                                path

as already mentioned once, please use captured variables directly
in the format strings for new additions or lines that you touch anyway.
E.g.:

bail!("unexpected error on datastore traversal: {inner} - {path:?}")

> +                            )
>                          }
> -                    } else {
> -                        bail!(
> -                            "unexpected error on datastore traversal: {} - {:?}",
> -                            inner,
> -                            path
> -                        )

same here

>                      }
>                  } else {
>                      bail!("unexpected error on datastore traversal: {}", inner)