[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