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

[pbs-devel] [PATCH v2 proxmox-backup 0/5] 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.

Changes since version 1:
- Avoid tuples in return values by downcasting anyhow::Error to Errno
  when latter is required
- Add report stale file handle helper
- Refactor report vanished/changed file helpers

Christian Ebner (5):
  client: pxar: refactor report vanished/changed helpers
  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/link read

 pbs-client/src/pxar/create.rs | 94 +++++++++++++++++++++++++----------
 1 file changed, 69 insertions(+), 25 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 v2 proxmox-backup 1/5] client: pxar: refactor report vanished/changed helpers

[Christian Ebner]

Switch from mutable reference to shared reference on `self` and drop
unused return value.

These helpers only write log messages, there is currently no need for
a mutable reference to `self`, nor to return a `Result`.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- not present in previous version

 pbs-client/src/pxar/create.rs | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index c0c492f8d..4d1883118 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -476,7 +476,7 @@ impl Archiver {
                 Ok(fd) => Ok(Some(fd)),
                 Err(Errno::ENOENT) => {
                     if existed {
-                        self.report_vanished_file()?;
+                        self.report_vanished_file();
                     }
                     Ok(None)
                 }
@@ -671,25 +671,22 @@ impl Archiver {
         Ok(file_list)
     }
 
-    fn report_vanished_file(&mut self) -> Result<(), Error> {
+    fn report_vanished_file(&self) {
         log::warn!("warning: file vanished while reading: {:?}", self.path);
-        Ok(())
     }
 
-    fn report_file_shrunk_while_reading(&mut self) -> Result<(), Error> {
+    fn report_file_shrunk_while_reading(&self) {
         log::warn!(
             "warning: file size shrunk while reading: {:?}, file will be padded with zeros!",
             self.path,
         );
-        Ok(())
     }
 
-    fn report_file_grew_while_reading(&mut self) -> Result<(), Error> {
+    fn report_file_grew_while_reading(&self) {
         log::warn!(
             "warning: file size increased while reading: {:?}, file will be truncated!",
             self.path,
         );
-        Ok(())
     }
 
     async fn add_entry<T: SeqWrite + Send>(
@@ -1239,14 +1236,14 @@ impl Archiver {
                 Err(err) => bail!(err),
             };
             if got as u64 > remaining {
-                self.report_file_grew_while_reading()?;
+                self.report_file_grew_while_reading();
                 got = remaining as usize;
             }
             out.write_all(&self.file_copy_buffer[..got]).await?;
             remaining -= got as u64;
         }
         if remaining > 0 {
-            self.report_file_shrunk_while_reading()?;
+            self.report_file_shrunk_while_reading();
             let to_zero = remaining.min(self.file_copy_buffer.len() as u64) as usize;
             vec::clear(&mut self.file_copy_buffer[..to_zero]);
             while remaining != 0 {
-- 
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 v2 proxmox-backup 2/5] 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.

Introduce a helper method to report entries for which a stale file
handle was encountered, providing an optional path for cases where
the `Archiver`s state does not store the correct path.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- introduce and use report_stale_file_handle helper method

 pbs-client/src/pxar/create.rs | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 4d1883118..8196c49d8 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)?;
@@ -671,6 +671,11 @@ impl Archiver {
         Ok(file_list)
     }
 
+    fn report_stale_file_handle(&self, path: Option<&PathBuf>) {
+        let path = path.unwrap_or(&self.path);
+        log::warn!("warning: stale file handle encountered while reading: {path:?}");
+    }
+
     fn report_vanished_file(&self) {
         log::warn!("warning: file vanished while reading: {:?}", self.path);
     }
@@ -1160,20 +1165,20 @@ 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) => {
+                    self.report_stale_file_handle(None);
+                    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;
 
@@ -1184,6 +1189,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 v2 proxmox-backup 3/5] 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>
---
changes since version 1:
- use report_stale_file_handle helper method

 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 8196c49d8..a7521424f 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -641,15 +641,30 @@ impl Archiver {
                 }
                 Ok(_) => (),
                 Err(err) if err.not_found() => continue,
+                Err(Errno::ESTALE) => {
+                    self.report_stale_file_handle(Some(&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) => {
+                        self.report_stale_file_handle(Some(&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 v2 proxmox-backup 4/5] 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>
---
changes since version 1:
- use report_stale_file_handle helper method

 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 a7521424f..3a6e9b157 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) => {
+                    self.report_stale_file_handle(None);
+                    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 v2 proxmox-backup 5/5] fix #5853: client: pxar: exclude stale files on metadata/link 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, or the
target in case of a symbolic link.

Instead of returning with a hard error, report the stale file handle
and skip over encoding this file entry in the pxar archive.

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>
---
changes since version 1:
- avoid return value tuples to signal stale file handles downcast
  anyhow::Error to Errno when check is required
- use report_stale_file_handle helper method

 pbs-client/src/pxar/create.rs | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 3a6e9b157..5d7957970 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -740,14 +740,23 @@ impl Archiver {
             None => return Ok(()),
         };
 
-        let metadata = get_metadata(
+        let metadata = match get_metadata(
             fd.as_raw_fd(),
             stat,
             self.flags(),
             self.fs_magic,
             &mut self.fs_feature_flags,
             self.skip_e2big_xattr,
-        )?;
+        ) {
+            Ok(metadata) => metadata,
+            Err(err) => {
+                if let Some(Errno::ESTALE) = err.downcast_ref::<Errno>() {
+                    self.report_stale_file_handle(None);
+                    return Ok(());
+                }
+                return Err(err);
+            }
+        };
 
         if self.previous_payload_index.is_none() {
             return self
@@ -1294,7 +1303,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) => {
+                self.report_stale_file_handle(None);
+                return Ok(());
+            }
+            Err(err) => return Err(err.into()),
+        };
         encoder.add_symlink(metadata, file_name, dest).await?;
         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] applied-series: [PATCH v2 proxmox-backup 0/5] fix #5853: ignore stale files

[Fabian Grünbichler]

thanks! this version looks much cleaner now :)

On November 14, 2024 3:41 pm, Christian Ebner wrote:
> 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.
> 
> Changes since version 1:
> - Avoid tuples in return values by downcasting anyhow::Error to Errno
>   when latter is required
> - Add report stale file handle helper
> - Refactor report vanished/changed file helpers
> 
> Christian Ebner (5):
>   client: pxar: refactor report vanished/changed helpers
>   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/link read
> 
>  pbs-client/src/pxar/create.rs | 94 +++++++++++++++++++++++++----------
>  1 file changed, 69 insertions(+), 25 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 mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel