From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id B993F1FF173 for ; Mon, 11 Nov 2024 14:37:35 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9724DBB96; Mon, 11 Nov 2024 14:37:34 +0100 (CET) Date: Mon, 11 Nov 2024 14:37:26 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20241105140153.282980-1-c.ebner@proxmox.com> <20241105140153.282980-5-c.ebner@proxmox.com> In-Reply-To: <20241105140153.282980-5-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1731330267.220wgkqhtc.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.048 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup 4/4] fix #5853: client: pxar: exclude stale files on metadata read X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 > --- > 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 { > +) -> 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 { 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 { > 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 { > 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 { 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