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

[Gabriel Goller <g.goller@proxmox.com>]

On 9/7/23 17:05, Thomas Lamprecht wrote:
> 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)
I could check if `err` is an `io:Error`, thus returning early. Then 
calling `.unwrap()`
to get the actual `io::Error` later on.
>> +                            // 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.
How about a "ignoring/skipping vanished file: {path}" on the info log 
level?
>> +                        }
>> +                        _ => {
>> +                            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)