[pbs-devel] [PATCH proxmox-backup 0/4] fix #5853: ignore stale files

[pbs-devel] [PATCH proxmox-backup 0/4] fix #5853: ignore stale files

[Christian Ebner]

When files and their associated metadata get invalidated, I/O
operations on network filesystems return ESTALE to indicate that the
filehandle does not reference a valid file anymore.

Currently, the proxmox-backup-client does not cover such cases, it
will fail with a hard error when a stale file handle is encountered.
Any concurrent operation invalidating file handles has the potential
to lead to the backups failing if timed accordingly. For local
filesystems this is not an issue, as the file remains accessible
until the file handle is closed.

Make the backup client more resilient by handling the ESTALE errors
gracefully, warning the user about the vanished/invalidated files,
while generating a valid and consistent backup archive nevertheless.

Christian Ebner (4):
  client: pxar: skip directories on stale file handle
  client: pxar: skip directory entries on stale file handle
  client: pxar: warn user and ignore stale file handles on file open
  fix #5853: client: pxar: exclude stale files on metadata read

 pbs-client/src/pxar/create.rs | 155 +++++++++++++++++++++++-----------
 1 file changed, 108 insertions(+), 47 deletions(-)

-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 1/4] client: pxar: skip directories on stale file handle

[Christian Ebner]

Skip over the whole directory in case the file handle was invalidated
and therefore the filesystem type check returns with ESTALE.

Encode the directory start entry in the archive and the catalog only
after the filesystem type check, so the directory can be fully skipped.
At this point it is still possible to ignore the invalidated
directory. If the directory is invalidated afterwards, it will be
backed up only partially.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-client/src/pxar/create.rs | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index c48524c4c..a2b9b3e30 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -72,7 +72,7 @@ pub struct PxarPrevRef {
     pub archive_name: String,
 }
 
-fn detect_fs_type(fd: RawFd) -> Result<i64, Error> {
+fn detect_fs_type(fd: RawFd) -> Result<i64, Errno> {
     let mut fs_stat = std::mem::MaybeUninit::uninit();
     let res = unsafe { libc::fstatfs(fd, fs_stat.as_mut_ptr()) };
     Errno::result(res)?;
@@ -1169,20 +1169,23 @@ impl Archiver {
     ) -> Result<(), Error> {
         let dir_name = OsStr::from_bytes(c_dir_name.to_bytes());
 
-        if !self.cache.caching_enabled() {
-            if let Some(ref catalog) = self.catalog {
-                catalog.lock().unwrap().start_directory(c_dir_name)?;
-            }
-            encoder.create_directory(dir_name, metadata).await?;
-        }
-
         let old_fs_magic = self.fs_magic;
         let old_fs_feature_flags = self.fs_feature_flags;
         let old_st_dev = self.current_st_dev;
 
         let mut skip_contents = false;
         if old_st_dev != stat.st_dev {
-            self.fs_magic = detect_fs_type(dir.as_raw_fd())?;
+            match detect_fs_type(dir.as_raw_fd()) {
+                Ok(fs_magic) => self.fs_magic = fs_magic,
+                Err(Errno::ESTALE) => {
+                    log::warn!(
+                        "encountered stale file handle, skipping directory: {:?}",
+                        self.path
+                    );
+                    return Ok(());
+                }
+                Err(err) => return Err(err.into()),
+            }
             self.fs_feature_flags = Flags::from_magic(self.fs_magic);
             self.current_st_dev = stat.st_dev;
 
@@ -1193,6 +1196,13 @@ impl Archiver {
             }
         }
 
+        if !self.cache.caching_enabled() {
+            if let Some(ref catalog) = self.catalog {
+                catalog.lock().unwrap().start_directory(c_dir_name)?;
+            }
+            encoder.create_directory(dir_name, metadata).await?;
+        }
+
         let result = if skip_contents {
             log::info!("skipping mount point: {:?}", self.path);
             Ok(())
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 2/4] client: pxar: skip directory entries on stale file handle

[Christian Ebner]

Skip over the entries when a stale file handle is encountered during
generation of the entry list of a directory entry.

This will lead to the directory not being backed up if the directory
itself was invalidated, as then reading all child entries will fail
also, or the directory is backed up without entries which have been
invalidated.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-client/src/pxar/create.rs | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index a2b9b3e30..8685e8d42 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -638,15 +638,30 @@ impl Archiver {
                 Ok(Some(MatchType::Exclude)) => continue,
                 Ok(_) => (),
                 Err(err) if err.not_found() => continue,
+                Err(Errno::ESTALE) => {
+                    log::warn!("stale file handle, skip {full_path:?}");
+                    continue;
+                }
                 Err(err) => {
                     return Err(err).with_context(|| format!("stat failed on {full_path:?}"))
                 }
             }
 
-            let stat = stat_results
-                .map(Ok)
-                .unwrap_or_else(get_file_mode)
-                .with_context(|| format!("stat failed on {full_path:?}"))?;
+            let stat = match stat_results {
+                Some(mode) => mode,
+                None => match get_file_mode() {
+                    Ok(mode) => mode,
+                    Err(Errno::ESTALE) => {
+                        log::warn!("stale file handle, skip {full_path:?}");
+                        continue;
+                    }
+                    Err(err) => {
+                        return Err(
+                            Error::from(err).context(format!("stat failed on {full_path:?}"))
+                        )
+                    }
+                },
+            };
 
             self.entry_counter += 1;
             if self.entry_counter > self.entry_limit {
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 3/4] client: pxar: warn user and ignore stale file handles on file open

[Christian Ebner]

Do not fail hard if a file open fails because of a stale file handle.
Warn the user and ignore the file, just like the client already does
in case of missing privileges to access the file.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-client/src/pxar/create.rs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 8685e8d42..2a844922c 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -484,6 +484,10 @@ impl Archiver {
                     log::warn!("failed to open file: {:?}: access denied", file_name);
                     Ok(None)
                 }
+                Err(Errno::ESTALE) => {
+                    log::warn!("failed to open file: {file_name:?}: stale file handle");
+                    Ok(None)
+                }
                 Err(Errno::EPERM) if !noatime.is_empty() => {
                     // Retry without O_NOATIME:
                     noatime = OFlag::empty();
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 4/4] fix #5853: client: pxar: exclude stale files on metadata read

[Christian Ebner]

Skip and warn the user for files which returned a stale file handle
error while reading the metadata associated to that file.

Instead of returning with an error when getting the metadata, return
a boolean flag signaling if a stale file handle has been encountered.

Link to issue in bugtracker:
https://bugzilla.proxmox.com/show_bug.cgi?id=5853

Link to thread in community forum:
https://forum.proxmox.com/threads/156822/

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-client/src/pxar/create.rs | 100 ++++++++++++++++++++++------------
 1 file changed, 66 insertions(+), 34 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 2a844922c..85be00db4 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -228,7 +228,7 @@ where
     let mut fs_feature_flags = Flags::from_magic(fs_magic);
 
     let stat = nix::sys::stat::fstat(source_dir.as_raw_fd())?;
-    let metadata = get_metadata(
+    let (metadata, stale_fd) = get_metadata(
         source_dir.as_raw_fd(),
         &stat,
         feature_flags & fs_feature_flags,
@@ -744,7 +744,7 @@ impl Archiver {
             return Ok(());
         }
 
-        let metadata = get_metadata(
+        let (metadata, stale_fd) = get_metadata(
             fd.as_raw_fd(),
             stat,
             self.flags(),
@@ -753,6 +753,11 @@ impl Archiver {
             self.skip_e2big_xattr,
         )?;
 
+        if stale_fd {
+            log::warn!("Stale filehandle encountered, skip {:?}", self.path);
+            return Ok(());
+        }
+
         if self.previous_payload_index.is_none() {
             return self
                 .add_entry_to_archive(encoder, &mut None, c_file_name, stat, fd, &metadata, None)
@@ -1301,7 +1306,14 @@ impl Archiver {
         file_name: &Path,
         metadata: &Metadata,
     ) -> Result<(), Error> {
-        let dest = nix::fcntl::readlinkat(fd.as_raw_fd(), &b""[..])?;
+        let dest = match nix::fcntl::readlinkat(fd.as_raw_fd(), &b""[..]) {
+            Ok(dest) => dest,
+            Err(Errno::ESTALE) => {
+                log::warn!("Stale file handle encountered, skip {file_name:?}");
+                return Ok(());
+            }
+            Err(err) => return Err(err.into()),
+        };
         encoder.add_symlink(metadata, file_name, dest).await?;
         Ok(())
     }
@@ -1397,9 +1409,10 @@ fn get_metadata(
     fs_magic: i64,
     fs_feature_flags: &mut Flags,
     skip_e2big_xattr: bool,
-) -> Result<Metadata, Error> {
+) -> Result<(Metadata, bool), Error> {
     // required for some of these
     let proc_path = Path::new("/proc/self/fd/").join(fd.to_string());
+    let mut stale_fd = false;
 
     let mut meta = Metadata {
         stat: pxar::Stat {
@@ -1412,18 +1425,27 @@ fn get_metadata(
         ..Default::default()
     };
 
-    get_xattr_fcaps_acl(
+    if get_xattr_fcaps_acl(
         &mut meta,
         fd,
         &proc_path,
         flags,
         fs_feature_flags,
         skip_e2big_xattr,
-    )?;
-    get_chattr(&mut meta, fd)?;
+    )? {
+        stale_fd = true;
+        log::warn!("Stale filehandle, xattrs incomplete");
+    }
+    if get_chattr(&mut meta, fd)? {
+        stale_fd = true;
+        log::warn!("Stale filehandle, chattr incomplete");
+    }
     get_fat_attr(&mut meta, fd, fs_magic)?;
-    get_quota_project_id(&mut meta, fd, flags, fs_magic)?;
-    Ok(meta)
+    if get_quota_project_id(&mut meta, fd, flags, fs_magic)? {
+        stale_fd = true;
+        log::warn!("Stale filehandle, quota  project id incomplete");
+    }
+    Ok((meta, stale_fd))
 }
 
 fn get_fcaps(
@@ -1431,22 +1453,23 @@ fn get_fcaps(
     fd: RawFd,
     flags: Flags,
     fs_feature_flags: &mut Flags,
-) -> Result<(), Error> {
+) -> Result<bool, Error> {
     if !flags.contains(Flags::WITH_FCAPS) {
-        return Ok(());
+        return Ok(false);
     }
 
     match xattr::fgetxattr(fd, xattr::XATTR_NAME_FCAPS) {
         Ok(data) => {
             meta.fcaps = Some(pxar::format::FCaps { data });
-            Ok(())
+            Ok(false)
         }
-        Err(Errno::ENODATA) => Ok(()),
+        Err(Errno::ENODATA) => Ok(false),
         Err(Errno::EOPNOTSUPP) => {
             fs_feature_flags.remove(Flags::WITH_FCAPS);
-            Ok(())
+            Ok(false)
         }
-        Err(Errno::EBADF) => Ok(()), // symlinks
+        Err(Errno::EBADF) => Ok(false), // symlinks
+        Err(Errno::ESTALE) => Ok(true),
         Err(err) => Err(err).context("failed to read file capabilities"),
     }
 }
@@ -1458,32 +1481,35 @@ fn get_xattr_fcaps_acl(
     flags: Flags,
     fs_feature_flags: &mut Flags,
     skip_e2big_xattr: bool,
-) -> Result<(), Error> {
+) -> Result<bool, Error> {
     if !flags.contains(Flags::WITH_XATTRS) {
-        return Ok(());
+        return Ok(false);
     }
 
     let xattrs = match xattr::flistxattr(fd) {
         Ok(names) => names,
         Err(Errno::EOPNOTSUPP) => {
             fs_feature_flags.remove(Flags::WITH_XATTRS);
-            return Ok(());
+            return Ok(false);
         }
         Err(Errno::E2BIG) => {
             match skip_e2big_xattr {
-                true => return Ok(()),
+                true => return Ok(false),
                 false => {
                     bail!("{} (try --skip-e2big-xattr)", Errno::E2BIG.to_string());
                 }
             };
         }
-        Err(Errno::EBADF) => return Ok(()), // symlinks
+        Err(Errno::EBADF) => return Ok(false), // symlinks
+        Err(Errno::ESTALE) => return Ok(true),
         Err(err) => return Err(err).context("failed to read xattrs"),
     };
 
     for attr in &xattrs {
         if xattr::is_security_capability(attr) {
-            get_fcaps(meta, fd, flags, fs_feature_flags)?;
+            if get_fcaps(meta, fd, flags, fs_feature_flags)? {
+                return Ok(true);
+            }
             continue;
         }
 
@@ -1505,35 +1531,37 @@ fn get_xattr_fcaps_acl(
             Err(Errno::EBADF) => (),   // symlinks, shouldn't be able to reach this either
             Err(Errno::E2BIG) => {
                 match skip_e2big_xattr {
-                    true => return Ok(()),
+                    true => return Ok(false),
                     false => {
                         bail!("{} (try --skip-e2big-xattr)", Errno::E2BIG.to_string());
                     }
                 };
             }
+            Err(Errno::ESTALE) => return Ok(true), // symlinks
             Err(err) => {
                 return Err(err).context(format!("error reading extended attribute {attr:?}"))
             }
         }
     }
 
-    Ok(())
+    Ok(false)
 }
 
-fn get_chattr(metadata: &mut Metadata, fd: RawFd) -> Result<(), Error> {
+fn get_chattr(metadata: &mut Metadata, fd: RawFd) -> Result<bool, Error> {
     let mut attr: libc::c_long = 0;
 
     match unsafe { fs::read_attr_fd(fd, &mut attr) } {
         Ok(_) => (),
+        Err(Errno::ESTALE) => return Ok(true),
         Err(errno) if errno_is_unsupported(errno) => {
-            return Ok(());
+            return Ok(false);
         }
         Err(err) => return Err(err).context("failed to read file attributes"),
     }
 
     metadata.stat.flags |= Flags::from_chattr(attr).bits();
 
-    Ok(())
+    Ok(false)
 }
 
 fn get_fat_attr(metadata: &mut Metadata, fd: RawFd, fs_magic: i64) -> Result<(), Error> {
@@ -1564,30 +1592,34 @@ fn get_quota_project_id(
     fd: RawFd,
     flags: Flags,
     magic: i64,
-) -> Result<(), Error> {
+) -> Result<bool, Error> {
     if !(metadata.is_dir() || metadata.is_regular_file()) {
-        return Ok(());
+        return Ok(false);
     }
 
     if !flags.contains(Flags::WITH_QUOTA_PROJID) {
-        return Ok(());
+        return Ok(false);
     }
 
     use proxmox_sys::linux::magic::*;
 
     match magic {
         EXT4_SUPER_MAGIC | XFS_SUPER_MAGIC | FUSE_SUPER_MAGIC | ZFS_SUPER_MAGIC => (),
-        _ => return Ok(()),
+        _ => return Ok(false),
     }
 
     let mut fsxattr = fs::FSXAttr::default();
     let res = unsafe { fs::fs_ioc_fsgetxattr(fd, &mut fsxattr) };
 
+    if let Err(Errno::ESTALE) = res {
+        return Ok(true);
+    }
+
     // On some FUSE filesystems it can happen that ioctl is not supported.
     // For these cases projid is set to 0 while the error is ignored.
     if let Err(errno) = res {
         if errno_is_unsupported(errno) {
-            return Ok(());
+            return Ok(false);
         } else {
             return Err(errno).context("error while reading quota project id");
         }
@@ -1597,7 +1629,7 @@ fn get_quota_project_id(
     if projid != 0 {
         metadata.quota_project_id = Some(pxar::format::QuotaProjectId { projid });
     }
-    Ok(())
+    Ok(false)
 }
 
 fn get_acl(
@@ -1840,7 +1872,7 @@ mod tests {
         let fs_magic = detect_fs_type(dir.as_raw_fd()).unwrap();
         let stat = nix::sys::stat::fstat(dir.as_raw_fd()).unwrap();
         let mut fs_feature_flags = Flags::from_magic(fs_magic);
-        let metadata = get_metadata(
+        let (metadata, _) = get_metadata(
             dir.as_raw_fd(),
             &stat,
             fs_feature_flags,
@@ -1937,7 +1969,7 @@ mod tests {
             let stat = nix::sys::stat::fstat(source_dir.as_raw_fd()).unwrap();
             let mut fs_feature_flags = Flags::from_magic(fs_magic);
 
-            let metadata = get_metadata(
+            let (metadata, _) = get_metadata(
                 source_dir.as_raw_fd(),
                 &stat,
                 fs_feature_flags,
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 3/4] client: pxar: warn user and ignore stale file handles on file open

[Fabian Grünbichler]

On November 5, 2024 3:01 pm, Christian Ebner wrote:
> Do not fail hard if a file open fails because of a stale file handle.
> Warn the user and ignore the file, just like the client already does
> in case of missing privileges to access the file.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-client/src/pxar/create.rs | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index 8685e8d42..2a844922c 100644
> --- a/pbs-client/src/pxar/create.rs
> +++ b/pbs-client/src/pxar/create.rs
> @@ -484,6 +484,10 @@ impl Archiver {
>                      log::warn!("failed to open file: {:?}: access denied", file_name);
>                      Ok(None)
>                  }
> +                Err(Errno::ESTALE) => {
> +                    log::warn!("failed to open file: {file_name:?}: stale file handle");
> +                    Ok(None)
> +                }

should we add a report_stale_file in vain of the other report helpers,
and use that in places where `self.path` contains the right information?

>                  Err(Errno::EPERM) if !noatime.is_empty() => {
>                      // Retry without O_NOATIME:
>                      noatime = OFlag::empty();
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 4/4] fix #5853: client: pxar: exclude stale files on metadata read

[Fabian Grünbichler]

behaviour wise this seems okay to me, but if possible, I'd avoid all the
return value tuples, see detailed comments below..

On November 5, 2024 3:01 pm, Christian Ebner wrote:
> Skip and warn the user for files which returned a stale file handle
> error while reading the metadata associated to that file.
> 
> Instead of returning with an error when getting the metadata, return
> a boolean flag signaling if a stale file handle has been encountered.
> 
> Link to issue in bugtracker:
> https://bugzilla.proxmox.com/show_bug.cgi?id=5853
> 
> Link to thread in community forum:
> https://forum.proxmox.com/threads/156822/
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-client/src/pxar/create.rs | 100 ++++++++++++++++++++++------------
>  1 file changed, 66 insertions(+), 34 deletions(-)
> 
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index 2a844922c..85be00db4 100644
> --- a/pbs-client/src/pxar/create.rs
> +++ b/pbs-client/src/pxar/create.rs
> @@ -228,7 +228,7 @@ where
>      let mut fs_feature_flags = Flags::from_magic(fs_magic);
>  
>      let stat = nix::sys::stat::fstat(source_dir.as_raw_fd())?;
> -    let metadata = get_metadata(
> +    let (metadata, stale_fd) = get_metadata(

stale_fd here is not used at all..

>          source_dir.as_raw_fd(),
>          &stat,
>          feature_flags & fs_feature_flags,
> @@ -744,7 +744,7 @@ impl Archiver {
>              return Ok(());
>          }
>  
> -        let metadata = get_metadata(
> +        let (metadata, stale_fd) = get_metadata(

this one is used

>              fd.as_raw_fd(),
>              stat,
>              self.flags(),
> @@ -753,6 +753,11 @@ impl Archiver {
>              self.skip_e2big_xattr,
>          )?;
>  
> +        if stale_fd {
> +            log::warn!("Stale filehandle encountered, skip {:?}", self.path);
> +            return Ok(());
> +        }

for this warning.. but get_metadata already logs (potentially multiple
times ;)) that things are incomplete cause of the stale filehandle, this
only adds the path context..

> +
>          if self.previous_payload_index.is_none() {
>              return self
>                  .add_entry_to_archive(encoder, &mut None, c_file_name, stat, fd, &metadata, None)
> @@ -1301,7 +1306,14 @@ impl Archiver {
>          file_name: &Path,
>          metadata: &Metadata,
>      ) -> Result<(), Error> {
> -        let dest = nix::fcntl::readlinkat(fd.as_raw_fd(), &b""[..])?;
> +        let dest = match nix::fcntl::readlinkat(fd.as_raw_fd(), &b""[..]) {
> +            Ok(dest) => dest,
> +            Err(Errno::ESTALE) => {
> +                log::warn!("Stale file handle encountered, skip {file_name:?}");
> +                return Ok(());
> +            }
> +            Err(err) => return Err(err.into()),
> +        };
>          encoder.add_symlink(metadata, file_name, dest).await?;
>          Ok(())
>      }
> @@ -1397,9 +1409,10 @@ fn get_metadata(
>      fs_magic: i64,
>      fs_feature_flags: &mut Flags,
>      skip_e2big_xattr: bool,
> -) -> Result<Metadata, Error> {
> +) -> Result<(Metadata, bool), Error> {
>      // required for some of these
>      let proc_path = Path::new("/proc/self/fd/").join(fd.to_string());
> +    let mut stale_fd = false;
>  
>      let mut meta = Metadata {
>          stat: pxar::Stat {
> @@ -1412,18 +1425,27 @@ fn get_metadata(
>          ..Default::default()
>      };
>  
> -    get_xattr_fcaps_acl(
> +    if get_xattr_fcaps_acl(

only call site, could just bubble up ESTALE

>          &mut meta,
>          fd,
>          &proc_path,
>          flags,
>          fs_feature_flags,
>          skip_e2big_xattr,
> -    )?;
> -    get_chattr(&mut meta, fd)?;
> +    )? {
> +        stale_fd = true;
> +        log::warn!("Stale filehandle, xattrs incomplete");
> +    }
> +    if get_chattr(&mut meta, fd)? {

same

> +        stale_fd = true;
> +        log::warn!("Stale filehandle, chattr incomplete");
> +    }
>      get_fat_attr(&mut meta, fd, fs_magic)?;
> -    get_quota_project_id(&mut meta, fd, flags, fs_magic)?;
> -    Ok(meta)
> +    if get_quota_project_id(&mut meta, fd, flags, fs_magic)? {

same

> +        stale_fd = true;
> +        log::warn!("Stale filehandle, quota  project id incomplete");
> +    }

see above and way down below, IMHO all of these could just bubble up the error..

> +    Ok((meta, stale_fd))
>  }
>  
>  fn get_fcaps(
> @@ -1431,22 +1453,23 @@ fn get_fcaps(
>      fd: RawFd,
>      flags: Flags,
>      fs_feature_flags: &mut Flags,
> -) -> Result<(), Error> {
> +) -> Result<bool, Error> {

this is only called by get_xattr_fcaps_acl, so could just bubble up
ESTALE as well..

>      if !flags.contains(Flags::WITH_FCAPS) {
> -        return Ok(());
> +        return Ok(false);
>      }
>  
>      match xattr::fgetxattr(fd, xattr::XATTR_NAME_FCAPS) {
>          Ok(data) => {
>              meta.fcaps = Some(pxar::format::FCaps { data });
> -            Ok(())
> +            Ok(false)
>          }
> -        Err(Errno::ENODATA) => Ok(()),
> +        Err(Errno::ENODATA) => Ok(false),
>          Err(Errno::EOPNOTSUPP) => {
>              fs_feature_flags.remove(Flags::WITH_FCAPS);
> -            Ok(())
> +            Ok(false)
>          }
> -        Err(Errno::EBADF) => Ok(()), // symlinks
> +        Err(Errno::EBADF) => Ok(false), // symlinks
> +        Err(Errno::ESTALE) => Ok(true),
>          Err(err) => Err(err).context("failed to read file capabilities"),
>      }
>  }
> @@ -1458,32 +1481,35 @@ fn get_xattr_fcaps_acl(
>      flags: Flags,
>      fs_feature_flags: &mut Flags,
>      skip_e2big_xattr: bool,
> -) -> Result<(), Error> {
> +) -> Result<bool, Error> {
>      if !flags.contains(Flags::WITH_XATTRS) {
> -        return Ok(());
> +        return Ok(false);
>      }
>  
>      let xattrs = match xattr::flistxattr(fd) {
>          Ok(names) => names,
>          Err(Errno::EOPNOTSUPP) => {
>              fs_feature_flags.remove(Flags::WITH_XATTRS);
> -            return Ok(());
> +            return Ok(false);
>          }
>          Err(Errno::E2BIG) => {
>              match skip_e2big_xattr {
> -                true => return Ok(()),
> +                true => return Ok(false),
>                  false => {
>                      bail!("{} (try --skip-e2big-xattr)", Errno::E2BIG.to_string());
>                  }
>              };
>          }
> -        Err(Errno::EBADF) => return Ok(()), // symlinks
> +        Err(Errno::EBADF) => return Ok(false), // symlinks
> +        Err(Errno::ESTALE) => return Ok(true),

see above

>          Err(err) => return Err(err).context("failed to read xattrs"),
>      };
>  
>      for attr in &xattrs {
>          if xattr::is_security_capability(attr) {
> -            get_fcaps(meta, fd, flags, fs_feature_flags)?;
> +            if get_fcaps(meta, fd, flags, fs_feature_flags)? {
> +                return Ok(true);

see above

> +            }
>              continue;
>          }
>  
> @@ -1505,35 +1531,37 @@ fn get_xattr_fcaps_acl(
>              Err(Errno::EBADF) => (),   // symlinks, shouldn't be able to reach this either
>              Err(Errno::E2BIG) => {
>                  match skip_e2big_xattr {
> -                    true => return Ok(()),
> +                    true => return Ok(false),
>                      false => {
>                          bail!("{} (try --skip-e2big-xattr)", Errno::E2BIG.to_string());
>                      }
>                  };
>              }
> +            Err(Errno::ESTALE) => return Ok(true), // symlinks

same here (and stray copy-paste comment I guess?)

>              Err(err) => {
>                  return Err(err).context(format!("error reading extended attribute {attr:?}"))
>              }
>          }
>      }
>  
> -    Ok(())
> +    Ok(false)
>  }
>  
> -fn get_chattr(metadata: &mut Metadata, fd: RawFd) -> Result<(), Error> {
> +fn get_chattr(metadata: &mut Metadata, fd: RawFd) -> Result<bool, Error> {
>      let mut attr: libc::c_long = 0;
>  
>      match unsafe { fs::read_attr_fd(fd, &mut attr) } {
>          Ok(_) => (),
> +        Err(Errno::ESTALE) => return Ok(true),
>          Err(errno) if errno_is_unsupported(errno) => {
> -            return Ok(());
> +            return Ok(false);
>          }
>          Err(err) => return Err(err).context("failed to read file attributes"),
>      }
>  
>      metadata.stat.flags |= Flags::from_chattr(attr).bits();
>  
> -    Ok(())
> +    Ok(false)
>  }
>  
>  fn get_fat_attr(metadata: &mut Metadata, fd: RawFd, fs_magic: i64) -> Result<(), Error> {
> @@ -1564,30 +1592,34 @@ fn get_quota_project_id(
>      fd: RawFd,
>      flags: Flags,
>      magic: i64,
> -) -> Result<(), Error> {
> +) -> Result<bool, Error> {

see above

>      if !(metadata.is_dir() || metadata.is_regular_file()) {
> -        return Ok(());
> +        return Ok(false);
>      }
>  
>      if !flags.contains(Flags::WITH_QUOTA_PROJID) {
> -        return Ok(());
> +        return Ok(false);
>      }
>  
>      use proxmox_sys::linux::magic::*;
>  
>      match magic {
>          EXT4_SUPER_MAGIC | XFS_SUPER_MAGIC | FUSE_SUPER_MAGIC | ZFS_SUPER_MAGIC => (),
> -        _ => return Ok(()),
> +        _ => return Ok(false),
>      }
>  
>      let mut fsxattr = fs::FSXAttr::default();
>      let res = unsafe { fs::fs_ioc_fsgetxattr(fd, &mut fsxattr) };
>  
> +    if let Err(Errno::ESTALE) = res {
> +        return Ok(true);
> +    }
> +
>      // On some FUSE filesystems it can happen that ioctl is not supported.
>      // For these cases projid is set to 0 while the error is ignored.
>      if let Err(errno) = res {
>          if errno_is_unsupported(errno) {
> -            return Ok(());
> +            return Ok(false);
>          } else {
>              return Err(errno).context("error while reading quota project id");
>          }
> @@ -1597,7 +1629,7 @@ fn get_quota_project_id(
>      if projid != 0 {
>          metadata.quota_project_id = Some(pxar::format::QuotaProjectId { projid });
>      }
> -    Ok(())
> +    Ok(false)
>  }
>  
>  fn get_acl(
> @@ -1840,7 +1872,7 @@ mod tests {
>          let fs_magic = detect_fs_type(dir.as_raw_fd()).unwrap();
>          let stat = nix::sys::stat::fstat(dir.as_raw_fd()).unwrap();
>          let mut fs_feature_flags = Flags::from_magic(fs_magic);
> -        let metadata = get_metadata(
> +        let (metadata, _) = get_metadata(

no use of the new return value

>              dir.as_raw_fd(),
>              &stat,
>              fs_feature_flags,
> @@ -1937,7 +1969,7 @@ mod tests {
>              let stat = nix::sys::stat::fstat(source_dir.as_raw_fd()).unwrap();
>              let mut fs_feature_flags = Flags::from_magic(fs_magic);
>  
> -            let metadata = get_metadata(
> +            let (metadata, _) = get_metadata(

no use either.. so wouldn't it make more sense to pass in a path and log
the context right in get_metadata? or treat the stale FD as an error,
and add the context/path as part of error handling?

the four call sites are:
- two related to tests, we can probably treat ESTALE as hard error there
- the one for obtaining the metadata of the source dir of the archive,
  if that is stale we can't create an archive -> hard error as well
- adding an entry: for the stale case, we already log a warning and
  proceed with the next entry, so we don't benefit from the fact that
  (incomplete) metadata and the staleness is returned, as opposed to
  just treating ESTALE as an error that we can "catch" and handle..

>                  source_dir.as_raw_fd(),
>                  &stat,
>                  fs_feature_flags,
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 4/4] fix #5853: client: pxar: exclude stale files on metadata read

[Christian Ebner]

On 11/11/24 14:37, Fabian Grünbichler wrote:
> behaviour wise this seems okay to me, but if possible, I'd avoid all the
> return value tuples, see detailed comments below..

Agreed, I am not a fan of passing the stale file handle error info along 
a the boolean in the return value as well.

But unfortunately passing along the error without loosing pre-existing 
error context and switching all the `get_metadata` related functions to 
return an `Errno` is not possible as is.

E.g. `process_acl` returns an `anyhow::Error` (could be defined to 
return an e.g. `Errno::EINVALID` instead?), special handling of 
`Errno::E2BIG` for the xattr case only, ...

The current approach was choosen to keep the current anyhow error 
context close to the actual errors when they occur.
  > On November 5, 2024 3:01 pm, Christian Ebner wrote:
>> Skip and warn the user for files which returned a stale file handle
>> error while reading the metadata associated to that file.
>>
>> Instead of returning with an error when getting the metadata, return
>> a boolean flag signaling if a stale file handle has been encountered.
>>
>> Link to issue in bugtracker:
>> https://bugzilla.proxmox.com/show_bug.cgi?id=5853
>>
>> Link to thread in community forum:
>> https://forum.proxmox.com/threads/156822/
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   pbs-client/src/pxar/create.rs | 100 ++++++++++++++++++++++------------
>>   1 file changed, 66 insertions(+), 34 deletions(-)
>>
>> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
>> index 2a844922c..85be00db4 100644
>> --- a/pbs-client/src/pxar/create.rs
>> +++ b/pbs-client/src/pxar/create.rs
>> @@ -228,7 +228,7 @@ where
>>       let mut fs_feature_flags = Flags::from_magic(fs_magic);
>>   
>>       let stat = nix::sys::stat::fstat(source_dir.as_raw_fd())?;
>> -    let metadata = get_metadata(
>> +    let (metadata, stale_fd) = get_metadata(
> 
> stale_fd here is not used at all..

Yes, that one should lead to a hard error as you mentioned, so should be 
handled accordingly. I will adapt this to be a hard error.

> 
>>           source_dir.as_raw_fd(),
>>           &stat,
>>           feature_flags & fs_feature_flags,
>> @@ -744,7 +744,7 @@ impl Archiver {
>>               return Ok(());
>>           }
>>   
>> -        let metadata = get_metadata(
>> +        let (metadata, stale_fd) = get_metadata(
> 
> this one is used
> 
>>               fd.as_raw_fd(),
>>               stat,
>>               self.flags(),
>> @@ -753,6 +753,11 @@ impl Archiver {
>>               self.skip_e2big_xattr,
>>           )?;
>>   
>> +        if stale_fd {
>> +            log::warn!("Stale filehandle encountered, skip {:?}", self.path);
>> +            return Ok(());
>> +        }
> 
> for this warning.. but get_metadata already logs (potentially multiple
> times ;)) that things are incomplete cause of the stale filehandle, this
> only adds the path context..

but here there is also an early return, not just the log... this skips 
over adding this entry, and any sub entries if the entry is a directory.

The logging could however be moved to the get_metadata call and only be 
logged once, agreed.

> 
>> +
>>           if self.previous_payload_index.is_none() {
>>               return self
>>                   .add_entry_to_archive(encoder, &mut None, c_file_name, stat, fd, &metadata, None)
>> @@ -1301,7 +1306,14 @@ impl Archiver {
>>           file_name: &Path,
>>           metadata: &Metadata,
>>       ) -> Result<(), Error> {
>> -        let dest = nix::fcntl::readlinkat(fd.as_raw_fd(), &b""[..])?;
>> +        let dest = match nix::fcntl::readlinkat(fd.as_raw_fd(), &b""[..]) {
>> +            Ok(dest) => dest,
>> +            Err(Errno::ESTALE) => {
>> +                log::warn!("Stale file handle encountered, skip {file_name:?}");
>> +                return Ok(());
>> +            }
>> +            Err(err) => return Err(err.into()),
>> +        };
>>           encoder.add_symlink(metadata, file_name, dest).await?;
>>           Ok(())
>>       }
>> @@ -1397,9 +1409,10 @@ fn get_metadata(
>>       fs_magic: i64,
>>       fs_feature_flags: &mut Flags,
>>       skip_e2big_xattr: bool,
>> -) -> Result<Metadata, Error> {
>> +) -> Result<(Metadata, bool), Error> {
>>       // required for some of these
>>       let proc_path = Path::new("/proc/self/fd/").join(fd.to_string());
>> +    let mut stale_fd = false;
>>   
>>       let mut meta = Metadata {
>>           stat: pxar::Stat {
>> @@ -1412,18 +1425,27 @@ fn get_metadata(
>>           ..Default::default()
>>       };
>>   
>> -    get_xattr_fcaps_acl(
>> +    if get_xattr_fcaps_acl(
> 
> only call site, could just bubble up ESTALE

As mentioned, this has 2 issues: Loss of anyhow error context for which 
sub-function the Errno occurred and sub-functions like `process_acl` 
which do not rely on ffi calls at all, returning plain `anyhow::Error`, 
which granted could be redefined to return an Errno.

> 
>>           &mut meta,
>>           fd,
>>           &proc_path,
>>           flags,
>>           fs_feature_flags,
>>           skip_e2big_xattr,
>> -    )?;
>> -    get_chattr(&mut meta, fd)?;
>> +    )? {
>> +        stale_fd = true;
>> +        log::warn!("Stale filehandle, xattrs incomplete");
>> +    }
>> +    if get_chattr(&mut meta, fd)? {
> 
> same
> 
>> +        stale_fd = true;
>> +        log::warn!("Stale filehandle, chattr incomplete");
>> +    }
>>       get_fat_attr(&mut meta, fd, fs_magic)?;
>> -    get_quota_project_id(&mut meta, fd, flags, fs_magic)?;
>> -    Ok(meta)
>> +    if get_quota_project_id(&mut meta, fd, flags, fs_magic)? {
> 
> same
> 
>> +        stale_fd = true;
>> +        log::warn!("Stale filehandle, quota  project id incomplete");
>> +    }
> 
> see above and way down below, IMHO all of these could just bubble up the error..
> 
>> +    Ok((meta, stale_fd))
>>   }
>>   
>>   fn get_fcaps(
>> @@ -1431,22 +1453,23 @@ fn get_fcaps(
>>       fd: RawFd,
>>       flags: Flags,
>>       fs_feature_flags: &mut Flags,
>> -) -> Result<(), Error> {
>> +) -> Result<bool, Error> {
> 
> this is only called by get_xattr_fcaps_acl, so could just bubble up
> ESTALE as well..
> 
>>       if !flags.contains(Flags::WITH_FCAPS) {
>> -        return Ok(());
>> +        return Ok(false);
>>       }
>>   
>>       match xattr::fgetxattr(fd, xattr::XATTR_NAME_FCAPS) {
>>           Ok(data) => {
>>               meta.fcaps = Some(pxar::format::FCaps { data });
>> -            Ok(())
>> +            Ok(false)
>>           }
>> -        Err(Errno::ENODATA) => Ok(()),
>> +        Err(Errno::ENODATA) => Ok(false),
>>           Err(Errno::EOPNOTSUPP) => {
>>               fs_feature_flags.remove(Flags::WITH_FCAPS);
>> -            Ok(())
>> +            Ok(false)
>>           }
>> -        Err(Errno::EBADF) => Ok(()), // symlinks
>> +        Err(Errno::EBADF) => Ok(false), // symlinks
>> +        Err(Errno::ESTALE) => Ok(true),
>>           Err(err) => Err(err).context("failed to read file capabilities"),
>>       }
>>   }
>> @@ -1458,32 +1481,35 @@ fn get_xattr_fcaps_acl(
>>       flags: Flags,
>>       fs_feature_flags: &mut Flags,
>>       skip_e2big_xattr: bool,
>> -) -> Result<(), Error> {
>> +) -> Result<bool, Error> {
>>       if !flags.contains(Flags::WITH_XATTRS) {
>> -        return Ok(());
>> +        return Ok(false);
>>       }
>>   
>>       let xattrs = match xattr::flistxattr(fd) {
>>           Ok(names) => names,
>>           Err(Errno::EOPNOTSUPP) => {
>>               fs_feature_flags.remove(Flags::WITH_XATTRS);
>> -            return Ok(());
>> +            return Ok(false);
>>           }
>>           Err(Errno::E2BIG) => {
>>               match skip_e2big_xattr {
>> -                true => return Ok(()),
>> +                true => return Ok(false),
>>                   false => {
>>                       bail!("{} (try --skip-e2big-xattr)", Errno::E2BIG.to_string());
>>                   }
>>               };
>>           }
>> -        Err(Errno::EBADF) => return Ok(()), // symlinks
>> +        Err(Errno::EBADF) => return Ok(false), // symlinks
>> +        Err(Errno::ESTALE) => return Ok(true),
> 
> see above
> 
>>           Err(err) => return Err(err).context("failed to read xattrs"),
>>       };
>>   
>>       for attr in &xattrs {
>>           if xattr::is_security_capability(attr) {
>> -            get_fcaps(meta, fd, flags, fs_feature_flags)?;
>> +            if get_fcaps(meta, fd, flags, fs_feature_flags)? {
>> +                return Ok(true);
> 
> see above
> 
>> +            }
>>               continue;
>>           }
>>   
>> @@ -1505,35 +1531,37 @@ fn get_xattr_fcaps_acl(
>>               Err(Errno::EBADF) => (),   // symlinks, shouldn't be able to reach this either
>>               Err(Errno::E2BIG) => {
>>                   match skip_e2big_xattr {
>> -                    true => return Ok(()),
>> +                    true => return Ok(false),
>>                       false => {
>>                           bail!("{} (try --skip-e2big-xattr)", Errno::E2BIG.to_string());
>>                       }
>>                   };
>>               }
>> +            Err(Errno::ESTALE) => return Ok(true), // symlinks
> 
> same here (and stray copy-paste comment I guess?)
> 
>>               Err(err) => {
>>                   return Err(err).context(format!("error reading extended attribute {attr:?}"))
>>               }
>>           }
>>       }
>>   
>> -    Ok(())
>> +    Ok(false)
>>   }
>>   
>> -fn get_chattr(metadata: &mut Metadata, fd: RawFd) -> Result<(), Error> {
>> +fn get_chattr(metadata: &mut Metadata, fd: RawFd) -> Result<bool, Error> {
>>       let mut attr: libc::c_long = 0;
>>   
>>       match unsafe { fs::read_attr_fd(fd, &mut attr) } {
>>           Ok(_) => (),
>> +        Err(Errno::ESTALE) => return Ok(true),
>>           Err(errno) if errno_is_unsupported(errno) => {
>> -            return Ok(());
>> +            return Ok(false);
>>           }
>>           Err(err) => return Err(err).context("failed to read file attributes"),
>>       }
>>   
>>       metadata.stat.flags |= Flags::from_chattr(attr).bits();
>>   
>> -    Ok(())
>> +    Ok(false)
>>   }
>>   
>>   fn get_fat_attr(metadata: &mut Metadata, fd: RawFd, fs_magic: i64) -> Result<(), Error> {
>> @@ -1564,30 +1592,34 @@ fn get_quota_project_id(
>>       fd: RawFd,
>>       flags: Flags,
>>       magic: i64,
>> -) -> Result<(), Error> {
>> +) -> Result<bool, Error> {
> 
> see above
> 
>>       if !(metadata.is_dir() || metadata.is_regular_file()) {
>> -        return Ok(());
>> +        return Ok(false);
>>       }
>>   
>>       if !flags.contains(Flags::WITH_QUOTA_PROJID) {
>> -        return Ok(());
>> +        return Ok(false);
>>       }
>>   
>>       use proxmox_sys::linux::magic::*;
>>   
>>       match magic {
>>           EXT4_SUPER_MAGIC | XFS_SUPER_MAGIC | FUSE_SUPER_MAGIC | ZFS_SUPER_MAGIC => (),
>> -        _ => return Ok(()),
>> +        _ => return Ok(false),
>>       }
>>   
>>       let mut fsxattr = fs::FSXAttr::default();
>>       let res = unsafe { fs::fs_ioc_fsgetxattr(fd, &mut fsxattr) };
>>   
>> +    if let Err(Errno::ESTALE) = res {
>> +        return Ok(true);
>> +    }
>> +
>>       // On some FUSE filesystems it can happen that ioctl is not supported.
>>       // For these cases projid is set to 0 while the error is ignored.
>>       if let Err(errno) = res {
>>           if errno_is_unsupported(errno) {
>> -            return Ok(());
>> +            return Ok(false);
>>           } else {
>>               return Err(errno).context("error while reading quota project id");
>>           }
>> @@ -1597,7 +1629,7 @@ fn get_quota_project_id(
>>       if projid != 0 {
>>           metadata.quota_project_id = Some(pxar::format::QuotaProjectId { projid });
>>       }
>> -    Ok(())
>> +    Ok(false)
>>   }
>>   
>>   fn get_acl(
>> @@ -1840,7 +1872,7 @@ mod tests {
>>           let fs_magic = detect_fs_type(dir.as_raw_fd()).unwrap();
>>           let stat = nix::sys::stat::fstat(dir.as_raw_fd()).unwrap();
>>           let mut fs_feature_flags = Flags::from_magic(fs_magic);
>> -        let metadata = get_metadata(
>> +        let (metadata, _) = get_metadata(
> 
> no use of the new return value
> 
>>               dir.as_raw_fd(),
>>               &stat,
>>               fs_feature_flags,
>> @@ -1937,7 +1969,7 @@ mod tests {
>>               let stat = nix::sys::stat::fstat(source_dir.as_raw_fd()).unwrap();
>>               let mut fs_feature_flags = Flags::from_magic(fs_magic);
>>   
>> -            let metadata = get_metadata(
>> +            let (metadata, _) = get_metadata(
> 
> no use either.. so wouldn't it make more sense to pass in a path and log
> the context right in get_metadata? or treat the stale FD as an error,
> and add the context/path as part of error handling?

The first approach seems better, will however not help to differentiate 
the (hard) errors from the soft error ESTALE, which requires to skip 
over entries at the `get_metadata` call side conditionally.

Returning the stale file handle error as `Anyhow::Error` also does not 
allow to distinguish from other (hard) errors, so again it cannot be 
handled as soft error at the call site.

And returning all errors as `Errno` has the loss of error context issue 
as described above.

I will see if I can cover this better by refactoring the code, as most 
of the helpers have a single call side, so it should be possible to 
reorganize without much side effects.

> 
> the four call sites are:
> - two related to tests, we can probably treat ESTALE as hard error there
> - the one for obtaining the metadata of the source dir of the archive,
>    if that is stale we can't create an archive -> hard error as well
> - adding an entry: for the stale case, we already log a warning and
>    proceed with the next entry, so we don't benefit from the fact that
>    (incomplete) metadata and the staleness is returned, as opposed to
>    just treating ESTALE as an error that we can "catch" and handle..
> 
>>                   source_dir.as_raw_fd(),
>>                   &stat,
>>                   fs_feature_flags,
>> -- 
>> 2.39.5
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 4/4] fix #5853: client: pxar: exclude stale files on metadata read

[Fabian Grünbichler]

On November 13, 2024 2:45 pm, Christian Ebner wrote:
> On 11/11/24 14:37, Fabian Grünbichler wrote:
>> behaviour wise this seems okay to me, but if possible, I'd avoid all the
>> return value tuples, see detailed comments below..
> 
> Agreed, I am not a fan of passing the stale file handle error info along 
> a the boolean in the return value as well.
> 
> But unfortunately passing along the error without loosing pre-existing 
> error context and switching all the `get_metadata` related functions to 
> return an `Errno` is not possible as is.
> 
> E.g. `process_acl` returns an `anyhow::Error` (could be defined to 
> return an e.g. `Errno::EINVALID` instead?), special handling of 
> `Errno::E2BIG` for the xattr case only, ...
> 
> The current approach was choosen to keep the current anyhow error 
> context close to the actual errors when they occur.

I think that should be solvable with some refactoring/defining of a
proper error type..

but you can also attempt to downcast the anyhow error to get the ESTALE?

>   > On November 5, 2024 3:01 pm, Christian Ebner wrote:
>>> Skip and warn the user for files which returned a stale file handle
>>> error while reading the metadata associated to that file.
>>>
>>> Instead of returning with an error when getting the metadata, return
>>> a boolean flag signaling if a stale file handle has been encountered.
>>>
>>> Link to issue in bugtracker:
>>> https://bugzilla.proxmox.com/show_bug.cgi?id=5853
>>>
>>> Link to thread in community forum:
>>> https://forum.proxmox.com/threads/156822/
>>>
>>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>>> ---
>>>   pbs-client/src/pxar/create.rs | 100 ++++++++++++++++++++++------------
>>>   1 file changed, 66 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
>>> index 2a844922c..85be00db4 100644
>>> --- a/pbs-client/src/pxar/create.rs
>>> +++ b/pbs-client/src/pxar/create.rs
>>> @@ -228,7 +228,7 @@ where
>>>       let mut fs_feature_flags = Flags::from_magic(fs_magic);
>>>   
>>>       let stat = nix::sys::stat::fstat(source_dir.as_raw_fd())?;
>>> -    let metadata = get_metadata(
>>> +    let (metadata, stale_fd) = get_metadata(
>> 
>> stale_fd here is not used at all..
> 
> Yes, that one should lead to a hard error as you mentioned, so should be 
> handled accordingly. I will adapt this to be a hard error.
> 
>> 
>>>           source_dir.as_raw_fd(),
>>>           &stat,
>>>           feature_flags & fs_feature_flags,
>>> @@ -744,7 +744,7 @@ impl Archiver {
>>>               return Ok(());
>>>           }
>>>   
>>> -        let metadata = get_metadata(
>>> +        let (metadata, stale_fd) = get_metadata(
>> 
>> this one is used
>> 
>>>               fd.as_raw_fd(),
>>>               stat,
>>>               self.flags(),
>>> @@ -753,6 +753,11 @@ impl Archiver {
>>>               self.skip_e2big_xattr,
>>>           )?;
>>>   
>>> +        if stale_fd {
>>> +            log::warn!("Stale filehandle encountered, skip {:?}", self.path);
>>> +            return Ok(());
>>> +        }
>> 
>> for this warning.. but get_metadata already logs (potentially multiple
>> times ;)) that things are incomplete cause of the stale filehandle, this
>> only adds the path context..
> 
> but here there is also an early return, not just the log... this skips 
> over adding this entry, and any sub entries if the entry is a directory.
> 
> The logging could however be moved to the get_metadata call and only be 
> logged once, agreed.
> 
>> 
>>> +
>>>           if self.previous_payload_index.is_none() {
>>>               return self
>>>                   .add_entry_to_archive(encoder, &mut None, c_file_name, stat, fd, &metadata, None)
>>> @@ -1301,7 +1306,14 @@ impl Archiver {
>>>           file_name: &Path,
>>>           metadata: &Metadata,
>>>       ) -> Result<(), Error> {
>>> -        let dest = nix::fcntl::readlinkat(fd.as_raw_fd(), &b""[..])?;
>>> +        let dest = match nix::fcntl::readlinkat(fd.as_raw_fd(), &b""[..]) {
>>> +            Ok(dest) => dest,
>>> +            Err(Errno::ESTALE) => {
>>> +                log::warn!("Stale file handle encountered, skip {file_name:?}");
>>> +                return Ok(());
>>> +            }
>>> +            Err(err) => return Err(err.into()),
>>> +        };
>>>           encoder.add_symlink(metadata, file_name, dest).await?;
>>>           Ok(())
>>>       }
>>> @@ -1397,9 +1409,10 @@ fn get_metadata(
>>>       fs_magic: i64,
>>>       fs_feature_flags: &mut Flags,
>>>       skip_e2big_xattr: bool,
>>> -) -> Result<Metadata, Error> {
>>> +) -> Result<(Metadata, bool), Error> {
>>>       // required for some of these
>>>       let proc_path = Path::new("/proc/self/fd/").join(fd.to_string());
>>> +    let mut stale_fd = false;
>>>   
>>>       let mut meta = Metadata {
>>>           stat: pxar::Stat {
>>> @@ -1412,18 +1425,27 @@ fn get_metadata(
>>>           ..Default::default()
>>>       };
>>>   
>>> -    get_xattr_fcaps_acl(
>>> +    if get_xattr_fcaps_acl(
>> 
>> only call site, could just bubble up ESTALE
> 
> As mentioned, this has 2 issues: Loss of anyhow error context for which 
> sub-function the Errno occurred and sub-functions like `process_acl` 
> which do not rely on ffi calls at all, returning plain `anyhow::Error`, 
> which granted could be redefined to return an Errno.
> 
>> 
>>>           &mut meta,
>>>           fd,
>>>           &proc_path,
>>>           flags,
>>>           fs_feature_flags,
>>>           skip_e2big_xattr,
>>> -    )?;
>>> -    get_chattr(&mut meta, fd)?;
>>> +    )? {
>>> +        stale_fd = true;
>>> +        log::warn!("Stale filehandle, xattrs incomplete");
>>> +    }
>>> +    if get_chattr(&mut meta, fd)? {
>> 
>> same
>> 
>>> +        stale_fd = true;
>>> +        log::warn!("Stale filehandle, chattr incomplete");
>>> +    }
>>>       get_fat_attr(&mut meta, fd, fs_magic)?;
>>> -    get_quota_project_id(&mut meta, fd, flags, fs_magic)?;
>>> -    Ok(meta)
>>> +    if get_quota_project_id(&mut meta, fd, flags, fs_magic)? {
>> 
>> same
>> 
>>> +        stale_fd = true;
>>> +        log::warn!("Stale filehandle, quota  project id incomplete");
>>> +    }
>> 
>> see above and way down below, IMHO all of these could just bubble up the error..
>> 
>>> +    Ok((meta, stale_fd))
>>>   }
>>>   
>>>   fn get_fcaps(
>>> @@ -1431,22 +1453,23 @@ fn get_fcaps(
>>>       fd: RawFd,
>>>       flags: Flags,
>>>       fs_feature_flags: &mut Flags,
>>> -) -> Result<(), Error> {
>>> +) -> Result<bool, Error> {
>> 
>> this is only called by get_xattr_fcaps_acl, so could just bubble up
>> ESTALE as well..
>> 
>>>       if !flags.contains(Flags::WITH_FCAPS) {
>>> -        return Ok(());
>>> +        return Ok(false);
>>>       }
>>>   
>>>       match xattr::fgetxattr(fd, xattr::XATTR_NAME_FCAPS) {
>>>           Ok(data) => {
>>>               meta.fcaps = Some(pxar::format::FCaps { data });
>>> -            Ok(())
>>> +            Ok(false)
>>>           }
>>> -        Err(Errno::ENODATA) => Ok(()),
>>> +        Err(Errno::ENODATA) => Ok(false),
>>>           Err(Errno::EOPNOTSUPP) => {
>>>               fs_feature_flags.remove(Flags::WITH_FCAPS);
>>> -            Ok(())
>>> +            Ok(false)
>>>           }
>>> -        Err(Errno::EBADF) => Ok(()), // symlinks
>>> +        Err(Errno::EBADF) => Ok(false), // symlinks
>>> +        Err(Errno::ESTALE) => Ok(true),
>>>           Err(err) => Err(err).context("failed to read file capabilities"),
>>>       }
>>>   }
>>> @@ -1458,32 +1481,35 @@ fn get_xattr_fcaps_acl(
>>>       flags: Flags,
>>>       fs_feature_flags: &mut Flags,
>>>       skip_e2big_xattr: bool,
>>> -) -> Result<(), Error> {
>>> +) -> Result<bool, Error> {
>>>       if !flags.contains(Flags::WITH_XATTRS) {
>>> -        return Ok(());
>>> +        return Ok(false);
>>>       }
>>>   
>>>       let xattrs = match xattr::flistxattr(fd) {
>>>           Ok(names) => names,
>>>           Err(Errno::EOPNOTSUPP) => {
>>>               fs_feature_flags.remove(Flags::WITH_XATTRS);
>>> -            return Ok(());
>>> +            return Ok(false);
>>>           }
>>>           Err(Errno::E2BIG) => {
>>>               match skip_e2big_xattr {
>>> -                true => return Ok(()),
>>> +                true => return Ok(false),
>>>                   false => {
>>>                       bail!("{} (try --skip-e2big-xattr)", Errno::E2BIG.to_string());
>>>                   }
>>>               };
>>>           }
>>> -        Err(Errno::EBADF) => return Ok(()), // symlinks
>>> +        Err(Errno::EBADF) => return Ok(false), // symlinks
>>> +        Err(Errno::ESTALE) => return Ok(true),
>> 
>> see above
>> 
>>>           Err(err) => return Err(err).context("failed to read xattrs"),
>>>       };
>>>   
>>>       for attr in &xattrs {
>>>           if xattr::is_security_capability(attr) {
>>> -            get_fcaps(meta, fd, flags, fs_feature_flags)?;
>>> +            if get_fcaps(meta, fd, flags, fs_feature_flags)? {
>>> +                return Ok(true);
>> 
>> see above
>> 
>>> +            }
>>>               continue;
>>>           }
>>>   
>>> @@ -1505,35 +1531,37 @@ fn get_xattr_fcaps_acl(
>>>               Err(Errno::EBADF) => (),   // symlinks, shouldn't be able to reach this either
>>>               Err(Errno::E2BIG) => {
>>>                   match skip_e2big_xattr {
>>> -                    true => return Ok(()),
>>> +                    true => return Ok(false),
>>>                       false => {
>>>                           bail!("{} (try --skip-e2big-xattr)", Errno::E2BIG.to_string());
>>>                       }
>>>                   };
>>>               }
>>> +            Err(Errno::ESTALE) => return Ok(true), // symlinks
>> 
>> same here (and stray copy-paste comment I guess?)
>> 
>>>               Err(err) => {
>>>                   return Err(err).context(format!("error reading extended attribute {attr:?}"))
>>>               }
>>>           }
>>>       }
>>>   
>>> -    Ok(())
>>> +    Ok(false)
>>>   }
>>>   
>>> -fn get_chattr(metadata: &mut Metadata, fd: RawFd) -> Result<(), Error> {
>>> +fn get_chattr(metadata: &mut Metadata, fd: RawFd) -> Result<bool, Error> {
>>>       let mut attr: libc::c_long = 0;
>>>   
>>>       match unsafe { fs::read_attr_fd(fd, &mut attr) } {
>>>           Ok(_) => (),
>>> +        Err(Errno::ESTALE) => return Ok(true),
>>>           Err(errno) if errno_is_unsupported(errno) => {
>>> -            return Ok(());
>>> +            return Ok(false);
>>>           }
>>>           Err(err) => return Err(err).context("failed to read file attributes"),
>>>       }
>>>   
>>>       metadata.stat.flags |= Flags::from_chattr(attr).bits();
>>>   
>>> -    Ok(())
>>> +    Ok(false)
>>>   }
>>>   
>>>   fn get_fat_attr(metadata: &mut Metadata, fd: RawFd, fs_magic: i64) -> Result<(), Error> {
>>> @@ -1564,30 +1592,34 @@ fn get_quota_project_id(
>>>       fd: RawFd,
>>>       flags: Flags,
>>>       magic: i64,
>>> -) -> Result<(), Error> {
>>> +) -> Result<bool, Error> {
>> 
>> see above
>> 
>>>       if !(metadata.is_dir() || metadata.is_regular_file()) {
>>> -        return Ok(());
>>> +        return Ok(false);
>>>       }
>>>   
>>>       if !flags.contains(Flags::WITH_QUOTA_PROJID) {
>>> -        return Ok(());
>>> +        return Ok(false);
>>>       }
>>>   
>>>       use proxmox_sys::linux::magic::*;
>>>   
>>>       match magic {
>>>           EXT4_SUPER_MAGIC | XFS_SUPER_MAGIC | FUSE_SUPER_MAGIC | ZFS_SUPER_MAGIC => (),
>>> -        _ => return Ok(()),
>>> +        _ => return Ok(false),
>>>       }
>>>   
>>>       let mut fsxattr = fs::FSXAttr::default();
>>>       let res = unsafe { fs::fs_ioc_fsgetxattr(fd, &mut fsxattr) };
>>>   
>>> +    if let Err(Errno::ESTALE) = res {
>>> +        return Ok(true);
>>> +    }
>>> +
>>>       // On some FUSE filesystems it can happen that ioctl is not supported.
>>>       // For these cases projid is set to 0 while the error is ignored.
>>>       if let Err(errno) = res {
>>>           if errno_is_unsupported(errno) {
>>> -            return Ok(());
>>> +            return Ok(false);
>>>           } else {
>>>               return Err(errno).context("error while reading quota project id");
>>>           }
>>> @@ -1597,7 +1629,7 @@ fn get_quota_project_id(
>>>       if projid != 0 {
>>>           metadata.quota_project_id = Some(pxar::format::QuotaProjectId { projid });
>>>       }
>>> -    Ok(())
>>> +    Ok(false)
>>>   }
>>>   
>>>   fn get_acl(
>>> @@ -1840,7 +1872,7 @@ mod tests {
>>>           let fs_magic = detect_fs_type(dir.as_raw_fd()).unwrap();
>>>           let stat = nix::sys::stat::fstat(dir.as_raw_fd()).unwrap();
>>>           let mut fs_feature_flags = Flags::from_magic(fs_magic);
>>> -        let metadata = get_metadata(
>>> +        let (metadata, _) = get_metadata(
>> 
>> no use of the new return value
>> 
>>>               dir.as_raw_fd(),
>>>               &stat,
>>>               fs_feature_flags,
>>> @@ -1937,7 +1969,7 @@ mod tests {
>>>               let stat = nix::sys::stat::fstat(source_dir.as_raw_fd()).unwrap();
>>>               let mut fs_feature_flags = Flags::from_magic(fs_magic);
>>>   
>>> -            let metadata = get_metadata(
>>> +            let (metadata, _) = get_metadata(
>> 
>> no use either.. so wouldn't it make more sense to pass in a path and log
>> the context right in get_metadata? or treat the stale FD as an error,
>> and add the context/path as part of error handling?
> 
> The first approach seems better, will however not help to differentiate 
> the (hard) errors from the soft error ESTALE, which requires to skip 
> over entries at the `get_metadata` call side conditionally.
> 
> Returning the stale file handle error as `Anyhow::Error` also does not 
> allow to distinguish from other (hard) errors, so again it cannot be 
> handled as soft error at the call site.
> 
> And returning all errors as `Errno` has the loss of error context issue 
> as described above.
> 
> I will see if I can cover this better by refactoring the code, as most 
> of the helpers have a single call side, so it should be possible to 
> reorganize without much side effects.
> 
>> 
>> the four call sites are:
>> - two related to tests, we can probably treat ESTALE as hard error there
>> - the one for obtaining the metadata of the source dir of the archive,
>>    if that is stale we can't create an archive -> hard error as well
>> - adding an entry: for the stale case, we already log a warning and
>>    proceed with the next entry, so we don't benefit from the fact that
>>    (incomplete) metadata and the staleness is returned, as opposed to
>>    just treating ESTALE as an error that we can "catch" and handle..
>> 
>>>                   source_dir.as_raw_fd(),
>>>                   &stat,
>>>                   fs_feature_flags,
>>> -- 
>>> 2.39.5
>>>
>>>
>>>
>>> _______________________________________________
>>> pbs-devel mailing list
>>> pbs-devel@lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>>
>>>
>>>
>> 
>> 
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>> 
>> 
> 
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 4/4] fix #5853: client: pxar: exclude stale files on metadata read

[Christian Ebner]

On 11/13/24 14:55, Fabian Grünbichler wrote:
> On November 13, 2024 2:45 pm, Christian Ebner wrote:
>> On 11/11/24 14:37, Fabian Grünbichler wrote:
>>> behaviour wise this seems okay to me, but if possible, I'd avoid all the
>>> return value tuples, see detailed comments below..
>>
>> Agreed, I am not a fan of passing the stale file handle error info along
>> a the boolean in the return value as well.
>>
>> But unfortunately passing along the error without loosing pre-existing
>> error context and switching all the `get_metadata` related functions to
>> return an `Errno` is not possible as is.
>>
>> E.g. `process_acl` returns an `anyhow::Error` (could be defined to
>> return an e.g. `Errno::EINVALID` instead?), special handling of
>> `Errno::E2BIG` for the xattr case only, ...
>>
>> The current approach was choosen to keep the current anyhow error
>> context close to the actual errors when they occur.
> 
> I think that should be solvable with some refactoring/defining of a
> proper error type..
> 
> but you can also attempt to downcast the anyhow error to get the ESTALE?

Ah true, thanks for the pointer!

I will send a new version of the patches incorporating your feedback.

> 
>>    > On November 5, 2024 3:01 pm, Christian Ebner wrote:
>>>> Skip and warn the user for files which returned a stale file handle
>>>> error while reading the metadata associated to that file.
>>>>
>>>> Instead of returning with an error when getting the metadata, return
>>>> a boolean flag signaling if a stale file handle has been encountered.
>>>>
>>>> Link to issue in bugtracker:
>>>> https://bugzilla.proxmox.com/show_bug.cgi?id=5853
>>>>
>>>> Link to thread in community forum:
>>>> https://forum.proxmox.com/threads/156822/
>>>>
>>>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>>>> ---
>>>>    pbs-client/src/pxar/create.rs | 100 ++++++++++++++++++++++------------
>>>>    1 file changed, 66 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
>>>> index 2a844922c..85be00db4 100644
>>>> --- a/pbs-client/src/pxar/create.rs
>>>> +++ b/pbs-client/src/pxar/create.rs
>>>> @@ -228,7 +228,7 @@ where
>>>>        let mut fs_feature_flags = Flags::from_magic(fs_magic);
>>>>    
>>>>        let stat = nix::sys::stat::fstat(source_dir.as_raw_fd())?;
>>>> -    let metadata = get_metadata(
>>>> +    let (metadata, stale_fd) = get_metadata(
>>>
>>> stale_fd here is not used at all..
>>
>> Yes, that one should lead to a hard error as you mentioned, so should be
>> handled accordingly. I will adapt this to be a hard error.
>>
>>>
>>>>            source_dir.as_raw_fd(),
>>>>            &stat,
>>>>            feature_flags & fs_feature_flags,
>>>> @@ -744,7 +744,7 @@ impl Archiver {
>>>>                return Ok(());
>>>>            }
>>>>    
>>>> -        let metadata = get_metadata(
>>>> +        let (metadata, stale_fd) = get_metadata(
>>>
>>> this one is used
>>>
>>>>                fd.as_raw_fd(),
>>>>                stat,
>>>>                self.flags(),
>>>> @@ -753,6 +753,11 @@ impl Archiver {
>>>>                self.skip_e2big_xattr,
>>>>            )?;
>>>>    
>>>> +        if stale_fd {
>>>> +            log::warn!("Stale filehandle encountered, skip {:?}", self.path);
>>>> +            return Ok(());
>>>> +        }
>>>
>>> for this warning.. but get_metadata already logs (potentially multiple
>>> times ;)) that things are incomplete cause of the stale filehandle, this
>>> only adds the path context..
>>
>> but here there is also an early return, not just the log... this skips
>> over adding this entry, and any sub entries if the entry is a directory.
>>
>> The logging could however be moved to the get_metadata call and only be
>> logged once, agreed.
>>
>>>
>>>> +
>>>>            if self.previous_payload_index.is_none() {
>>>>                return self
>>>>                    .add_entry_to_archive(encoder, &mut None, c_file_name, stat, fd, &metadata, None)
>>>> @@ -1301,7 +1306,14 @@ impl Archiver {
>>>>            file_name: &Path,
>>>>            metadata: &Metadata,
>>>>        ) -> Result<(), Error> {
>>>> -        let dest = nix::fcntl::readlinkat(fd.as_raw_fd(), &b""[..])?;
>>>> +        let dest = match nix::fcntl::readlinkat(fd.as_raw_fd(), &b""[..]) {
>>>> +            Ok(dest) => dest,
>>>> +            Err(Errno::ESTALE) => {
>>>> +                log::warn!("Stale file handle encountered, skip {file_name:?}");
>>>> +                return Ok(());
>>>> +            }
>>>> +            Err(err) => return Err(err.into()),
>>>> +        };
>>>>            encoder.add_symlink(metadata, file_name, dest).await?;
>>>>            Ok(())
>>>>        }
>>>> @@ -1397,9 +1409,10 @@ fn get_metadata(
>>>>        fs_magic: i64,
>>>>        fs_feature_flags: &mut Flags,
>>>>        skip_e2big_xattr: bool,
>>>> -) -> Result<Metadata, Error> {
>>>> +) -> Result<(Metadata, bool), Error> {
>>>>        // required for some of these
>>>>        let proc_path = Path::new("/proc/self/fd/").join(fd.to_string());
>>>> +    let mut stale_fd = false;
>>>>    
>>>>        let mut meta = Metadata {
>>>>            stat: pxar::Stat {
>>>> @@ -1412,18 +1425,27 @@ fn get_metadata(
>>>>            ..Default::default()
>>>>        };
>>>>    
>>>> -    get_xattr_fcaps_acl(
>>>> +    if get_xattr_fcaps_acl(
>>>
>>> only call site, could just bubble up ESTALE
>>
>> As mentioned, this has 2 issues: Loss of anyhow error context for which
>> sub-function the Errno occurred and sub-functions like `process_acl`
>> which do not rely on ffi calls at all, returning plain `anyhow::Error`,
>> which granted could be redefined to return an Errno.
>>
>>>
>>>>            &mut meta,
>>>>            fd,
>>>>            &proc_path,
>>>>            flags,
>>>>            fs_feature_flags,
>>>>            skip_e2big_xattr,
>>>> -    )?;
>>>> -    get_chattr(&mut meta, fd)?;
>>>> +    )? {
>>>> +        stale_fd = true;
>>>> +        log::warn!("Stale filehandle, xattrs incomplete");
>>>> +    }
>>>> +    if get_chattr(&mut meta, fd)? {
>>>
>>> same
>>>
>>>> +        stale_fd = true;
>>>> +        log::warn!("Stale filehandle, chattr incomplete");
>>>> +    }
>>>>        get_fat_attr(&mut meta, fd, fs_magic)?;
>>>> -    get_quota_project_id(&mut meta, fd, flags, fs_magic)?;
>>>> -    Ok(meta)
>>>> +    if get_quota_project_id(&mut meta, fd, flags, fs_magic)? {
>>>
>>> same
>>>
>>>> +        stale_fd = true;
>>>> +        log::warn!("Stale filehandle, quota  project id incomplete");
>>>> +    }
>>>
>>> see above and way down below, IMHO all of these could just bubble up the error..
>>>
>>>> +    Ok((meta, stale_fd))
>>>>    }
>>>>    
>>>>    fn get_fcaps(
>>>> @@ -1431,22 +1453,23 @@ fn get_fcaps(
>>>>        fd: RawFd,
>>>>        flags: Flags,
>>>>        fs_feature_flags: &mut Flags,
>>>> -) -> Result<(), Error> {
>>>> +) -> Result<bool, Error> {
>>>
>>> this is only called by get_xattr_fcaps_acl, so could just bubble up
>>> ESTALE as well..
>>>
>>>>        if !flags.contains(Flags::WITH_FCAPS) {
>>>> -        return Ok(());
>>>> +        return Ok(false);
>>>>        }
>>>>    
>>>>        match xattr::fgetxattr(fd, xattr::XATTR_NAME_FCAPS) {
>>>>            Ok(data) => {
>>>>                meta.fcaps = Some(pxar::format::FCaps { data });
>>>> -            Ok(())
>>>> +            Ok(false)
>>>>            }
>>>> -        Err(Errno::ENODATA) => Ok(()),
>>>> +        Err(Errno::ENODATA) => Ok(false),
>>>>            Err(Errno::EOPNOTSUPP) => {
>>>>                fs_feature_flags.remove(Flags::WITH_FCAPS);
>>>> -            Ok(())
>>>> +            Ok(false)
>>>>            }
>>>> -        Err(Errno::EBADF) => Ok(()), // symlinks
>>>> +        Err(Errno::EBADF) => Ok(false), // symlinks
>>>> +        Err(Errno::ESTALE) => Ok(true),
>>>>            Err(err) => Err(err).context("failed to read file capabilities"),
>>>>        }
>>>>    }
>>>> @@ -1458,32 +1481,35 @@ fn get_xattr_fcaps_acl(
>>>>        flags: Flags,
>>>>        fs_feature_flags: &mut Flags,
>>>>        skip_e2big_xattr: bool,
>>>> -) -> Result<(), Error> {
>>>> +) -> Result<bool, Error> {
>>>>        if !flags.contains(Flags::WITH_XATTRS) {
>>>> -        return Ok(());
>>>> +        return Ok(false);
>>>>        }
>>>>    
>>>>        let xattrs = match xattr::flistxattr(fd) {
>>>>            Ok(names) => names,
>>>>            Err(Errno::EOPNOTSUPP) => {
>>>>                fs_feature_flags.remove(Flags::WITH_XATTRS);
>>>> -            return Ok(());
>>>> +            return Ok(false);
>>>>            }
>>>>            Err(Errno::E2BIG) => {
>>>>                match skip_e2big_xattr {
>>>> -                true => return Ok(()),
>>>> +                true => return Ok(false),
>>>>                    false => {
>>>>                        bail!("{} (try --skip-e2big-xattr)", Errno::E2BIG.to_string());
>>>>                    }
>>>>                };
>>>>            }
>>>> -        Err(Errno::EBADF) => return Ok(()), // symlinks
>>>> +        Err(Errno::EBADF) => return Ok(false), // symlinks
>>>> +        Err(Errno::ESTALE) => return Ok(true),
>>>
>>> see above
>>>
>>>>            Err(err) => return Err(err).context("failed to read xattrs"),
>>>>        };
>>>>    
>>>>        for attr in &xattrs {
>>>>            if xattr::is_security_capability(attr) {
>>>> -            get_fcaps(meta, fd, flags, fs_feature_flags)?;
>>>> +            if get_fcaps(meta, fd, flags, fs_feature_flags)? {
>>>> +                return Ok(true);
>>>
>>> see above
>>>
>>>> +            }
>>>>                continue;
>>>>            }
>>>>    
>>>> @@ -1505,35 +1531,37 @@ fn get_xattr_fcaps_acl(
>>>>                Err(Errno::EBADF) => (),   // symlinks, shouldn't be able to reach this either
>>>>                Err(Errno::E2BIG) => {
>>>>                    match skip_e2big_xattr {
>>>> -                    true => return Ok(()),
>>>> +                    true => return Ok(false),
>>>>                        false => {
>>>>                            bail!("{} (try --skip-e2big-xattr)", Errno::E2BIG.to_string());
>>>>                        }
>>>>                    };
>>>>                }
>>>> +            Err(Errno::ESTALE) => return Ok(true), // symlinks
>>>
>>> same here (and stray copy-paste comment I guess?)
>>>
>>>>                Err(err) => {
>>>>                    return Err(err).context(format!("error reading extended attribute {attr:?}"))
>>>>                }
>>>>            }
>>>>        }
>>>>    
>>>> -    Ok(())
>>>> +    Ok(false)
>>>>    }
>>>>    
>>>> -fn get_chattr(metadata: &mut Metadata, fd: RawFd) -> Result<(), Error> {
>>>> +fn get_chattr(metadata: &mut Metadata, fd: RawFd) -> Result<bool, Error> {
>>>>        let mut attr: libc::c_long = 0;
>>>>    
>>>>        match unsafe { fs::read_attr_fd(fd, &mut attr) } {
>>>>            Ok(_) => (),
>>>> +        Err(Errno::ESTALE) => return Ok(true),
>>>>            Err(errno) if errno_is_unsupported(errno) => {
>>>> -            return Ok(());
>>>> +            return Ok(false);
>>>>            }
>>>>            Err(err) => return Err(err).context("failed to read file attributes"),
>>>>        }
>>>>    
>>>>        metadata.stat.flags |= Flags::from_chattr(attr).bits();
>>>>    
>>>> -    Ok(())
>>>> +    Ok(false)
>>>>    }
>>>>    
>>>>    fn get_fat_attr(metadata: &mut Metadata, fd: RawFd, fs_magic: i64) -> Result<(), Error> {
>>>> @@ -1564,30 +1592,34 @@ fn get_quota_project_id(
>>>>        fd: RawFd,
>>>>        flags: Flags,
>>>>        magic: i64,
>>>> -) -> Result<(), Error> {
>>>> +) -> Result<bool, Error> {
>>>
>>> see above
>>>
>>>>        if !(metadata.is_dir() || metadata.is_regular_file()) {
>>>> -        return Ok(());
>>>> +        return Ok(false);
>>>>        }
>>>>    
>>>>        if !flags.contains(Flags::WITH_QUOTA_PROJID) {
>>>> -        return Ok(());
>>>> +        return Ok(false);
>>>>        }
>>>>    
>>>>        use proxmox_sys::linux::magic::*;
>>>>    
>>>>        match magic {
>>>>            EXT4_SUPER_MAGIC | XFS_SUPER_MAGIC | FUSE_SUPER_MAGIC | ZFS_SUPER_MAGIC => (),
>>>> -        _ => return Ok(()),
>>>> +        _ => return Ok(false),
>>>>        }
>>>>    
>>>>        let mut fsxattr = fs::FSXAttr::default();
>>>>        let res = unsafe { fs::fs_ioc_fsgetxattr(fd, &mut fsxattr) };
>>>>    
>>>> +    if let Err(Errno::ESTALE) = res {
>>>> +        return Ok(true);
>>>> +    }
>>>> +
>>>>        // On some FUSE filesystems it can happen that ioctl is not supported.
>>>>        // For these cases projid is set to 0 while the error is ignored.
>>>>        if let Err(errno) = res {
>>>>            if errno_is_unsupported(errno) {
>>>> -            return Ok(());
>>>> +            return Ok(false);
>>>>            } else {
>>>>                return Err(errno).context("error while reading quota project id");
>>>>            }
>>>> @@ -1597,7 +1629,7 @@ fn get_quota_project_id(
>>>>        if projid != 0 {
>>>>            metadata.quota_project_id = Some(pxar::format::QuotaProjectId { projid });
>>>>        }
>>>> -    Ok(())
>>>> +    Ok(false)
>>>>    }
>>>>    
>>>>    fn get_acl(
>>>> @@ -1840,7 +1872,7 @@ mod tests {
>>>>            let fs_magic = detect_fs_type(dir.as_raw_fd()).unwrap();
>>>>            let stat = nix::sys::stat::fstat(dir.as_raw_fd()).unwrap();
>>>>            let mut fs_feature_flags = Flags::from_magic(fs_magic);
>>>> -        let metadata = get_metadata(
>>>> +        let (metadata, _) = get_metadata(
>>>
>>> no use of the new return value
>>>
>>>>                dir.as_raw_fd(),
>>>>                &stat,
>>>>                fs_feature_flags,
>>>> @@ -1937,7 +1969,7 @@ mod tests {
>>>>                let stat = nix::sys::stat::fstat(source_dir.as_raw_fd()).unwrap();
>>>>                let mut fs_feature_flags = Flags::from_magic(fs_magic);
>>>>    
>>>> -            let metadata = get_metadata(
>>>> +            let (metadata, _) = get_metadata(
>>>
>>> no use either.. so wouldn't it make more sense to pass in a path and log
>>> the context right in get_metadata? or treat the stale FD as an error,
>>> and add the context/path as part of error handling?
>>
>> The first approach seems better, will however not help to differentiate
>> the (hard) errors from the soft error ESTALE, which requires to skip
>> over entries at the `get_metadata` call side conditionally.
>>
>> Returning the stale file handle error as `Anyhow::Error` also does not
>> allow to distinguish from other (hard) errors, so again it cannot be
>> handled as soft error at the call site.
>>
>> And returning all errors as `Errno` has the loss of error context issue
>> as described above.
>>
>> I will see if I can cover this better by refactoring the code, as most
>> of the helpers have a single call side, so it should be possible to
>> reorganize without much side effects.
>>
>>>
>>> the four call sites are:
>>> - two related to tests, we can probably treat ESTALE as hard error there
>>> - the one for obtaining the metadata of the source dir of the archive,
>>>     if that is stale we can't create an archive -> hard error as well
>>> - adding an entry: for the stale case, we already log a warning and
>>>     proceed with the next entry, so we don't benefit from the fact that
>>>     (incomplete) metadata and the staleness is returned, as opposed to
>>>     just treating ESTALE as an error that we can "catch" and handle..
>>>
>>>>                    source_dir.as_raw_fd(),
>>>>                    &stat,
>>>>                    fs_feature_flags,
>>>> -- 
>>>> 2.39.5
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> pbs-devel mailing list
>>>> pbs-devel@lists.proxmox.com
>>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>>>
>>>>
>>>>
>>>
>>>
>>> _______________________________________________
>>> pbs-devel mailing list
>>> pbs-devel@lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>>
>>>
>>
>>



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 0/4] fix #5853: ignore stale files

[Christian Ebner]

superseded-by version 2:
https://lore.proxmox.com/pbs-devel/20241114144114.375987-1-c.ebner@proxmox.com/T/


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel