Re: [pbs-devel] [PATCH proxmox-backup 3/4] gc: remove .bad files on garbage collect

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

On 03.09.20 16:17, Stefan Reiter wrote:

> diff --git a/src/backup/chunk_store.rs b/src/backup/chunk_store.rs
> index e1da5a8a..5c2fb29d 100644
> --- a/src/backup/chunk_store.rs
> +++ b/src/backup/chunk_store.rs
> @@ -187,7 +187,7 @@ impl ChunkStore {
>      pub fn get_chunk_iterator(
>          &self,
>      ) -> Result<
> -        impl Iterator<Item = (Result<tools::fs::ReadDirEntry, Error>, usize)> + std::iter::FusedIterator,
> +        impl Iterator<Item = (Result<tools::fs::ReadDirEntry, Error>, usize, bool)> + std::iter::FusedIterator,
>          Error
>      > {
>          use nix::dir::Dir;
> @@ -218,20 +218,26 @@ impl ChunkStore {
>                      match inner.next() {
>                          Some(Ok(entry)) => {
>                              // skip files if they're not a hash
> -                            let bytes = entry.file_name().to_bytes();
> -                            if bytes.len() != 64 {
> -                                continue;
> +                            let hash = {
> +                                let bytes = entry.file_name().to_bytes();
> +                                bytes.len() == 64 && bytes.iter().all(u8::is_ascii_hexdigit)

why change the check, we do not care for deleting a non hex named file,
this is a private controlled directory after all. I'd avoid doing extra
checks, their costs, even if small, are amplified a lot.

> +                            };
> +
> +                            if hash {
> +                                return Some((Ok(entry), percentage, false));
> +                            } else if let Ok(name) = entry.file_name().to_str() {
> +                                if name.ends_with(".bad") {
> +                                    return Some((Ok(entry), percentage, true));
> +                                }

we only want to remove bad chunks if a good exists again, else we may
loose information - e.g., if the detected corruption was caused by bad
memory (RAM) not the chunk itself.

> [...]
> @@ -321,7 +327,20 @@ impl ChunkStore {
>              let lock = self.mutex.lock();
>  
>              if let Ok(stat) = fstatat(dirfd, filename, nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW) {
> -                if stat.st_atime < min_atime {
> +                if bad {
> +                    let res = unsafe { libc::unlinkat(dirfd, filename.as_ptr(), 0) };

don't we have something for this already? Else I'd move this out to a closure
or local function

> +                    if res != 0 {
> +                        let err = nix::Error::last();
> +                        worker.warn(format!(
> +                            "unlink .bad file {:?} failed on store '{}' - {}",

"unlinking corrupt chunk ...

> +                            filename,
> +                            self.name,
> +                            err,
> +                        ));
> +                    }
> +                    status.removed_bad += 1;
> +                    status.removed_bytes += stat.st_size as u64;

you count this up even if the unlinkat failed?

> +                } else if stat.st_atime < min_atime {
>                      //let age = now - stat.st_atime;
>                      //println!("UNLINK {}  {:?}", age/(3600*24), filename);
>                      let res = unsafe { libc::unlinkat(dirfd, filename.as_ptr(), 0) };