Re: [PATCH 4/4] xfs_db: add command to copy directory trees out of filesystems

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Feb 18, 2025 at 07:54:09AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 18, 2025 at 12:36:33AM -0800, Christoph Hellwig wrote:
> > On Thu, Feb 06, 2025 at 03:03:32PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Aheada of deprecating V4 support in the kernel, let's give people a way
> > > to extract their files from a filesystem without needing to mount.
> > 
> > So I've wanted a userspace file access for a while, but if we deprecate
> > the v4 support in the kernel that will propagte to libxfs quickly,
> > and this code won't help you with v4 file systems either.  So I don't
> > think the rationale here seems very good.
> 
> We aren't removing V4 support from the kernel until September 2030 and
> xfsprogs effectively builds with CONFIG_XFS_SUPPORT_V4=y.  That should
> be enough time, right?
> 
> > >  extern void		bmapinflate_init(void);
> > > +extern void		rdump_init(void);
> > 
> > No need for the extern.
> 
> Ok.
> 
> > > +	/* XXX cannot copy fsxattrs */
> > 
> > Should this be fixed first?  Or document in a full sentence comment
> > explaining why it can't should not be?
> 
> 	/* XXX cannot copy fsxattrs until setfsxattrat() syscall merge */
> 
> > > +		[1] = {
> > > +			.tv_sec  = inode_get_mtime_sec(VFS_I(ip)),
> > > +			.tv_nsec = inode_get_mtime_nsec(VFS_I(ip)),
> > > +		},
> > > +	};
> > > +	int			ret;
> > > +
> > > +	/* XXX cannot copy ctime or btime */
> > 
> > Same for this and others.
> 
> Is there a way to set ctime or btime?  I don't know of any.

Now that I'm past all the morning meetings and have had time to research
things a little more deeply/wake up more: no, there's no way to set the
inode change or birth times.

> 	/* Cannot set ctime or btime */

So I'll go with ^^ this comment.

> > > +	/* Format xattr name */
> > > +	if (attr_flags & XFS_ATTR_ROOT)
> > > +		nsp = XATTR_TRUSTED_PREFIX;
> > > +	else if (attr_flags & XFS_ATTR_SECURE)
> > > +		nsp = XATTR_SECURITY_PREFIX;
> > > +	else
> > > +		nsp = XATTR_USER_PREFIX;
> > 
> > Add a self-cotained helper for this?  I'm pretty sure we do this
> > translation in a few places.

Actually, xfsprogs doesn't:

$ git grep XATTR_SECURITY_PREFIX
db/rdump.c:293:         nsp = XATTR_SECURITY_PREFIX;
mkfs/proto.c:393:       } else if (!strncmp(attrname, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
mkfs/proto.c:394:               args.name = (unsigned char *)attrname + XATTR_SECURITY_PREFIX_LEN;

The kernel gets a little closer in xfs_xattr.c:

$ git grep XATTR_SECURITY_PREFIX include/ fs/*.c fs/xfs/
fs/xattr.c:128: if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN) ||
fs/xattr.c:228: int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
fs/xattr.c:229:                            XATTR_SECURITY_PREFIX_LEN);
fs/xattr.c:249:                 const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
fs/xattr.c:442: if (!strncmp(name, XATTR_SECURITY_PREFIX,
fs/xattr.c:443:                         XATTR_SECURITY_PREFIX_LEN)) {
fs/xattr.c:444:         const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
fs/xfs/xfs_xattr.c:207: .prefix = XATTR_SECURITY_PREFIX,
fs/xfs/xfs_xattr.c:304:         prefix = XATTR_SECURITY_PREFIX;
fs/xfs/xfs_xattr.c:305:         prefix_len = XATTR_SECURITY_PREFIX_LEN;

But xfs_xattr_put_listent has some custom logic in it:

	if (flags & XFS_ATTR_ROOT) {
#ifdef CONFIG_XFS_POSIX_ACL
		if (namelen == SGI_ACL_FILE_SIZE &&
		    strncmp(name, SGI_ACL_FILE,
			    SGI_ACL_FILE_SIZE) == 0) {
			__xfs_xattr_put_listent(
					context, XATTR_SYSTEM_PREFIX,
					XATTR_SYSTEM_PREFIX_LEN,
					XATTR_POSIX_ACL_ACCESS,
					strlen(XATTR_POSIX_ACL_ACCESS));
		} else if (namelen == SGI_ACL_DEFAULT_SIZE &&
			 strncmp(name, SGI_ACL_DEFAULT,
				 SGI_ACL_DEFAULT_SIZE) == 0) {
			__xfs_xattr_put_listent(
					context, XATTR_SYSTEM_PREFIX,
					XATTR_SYSTEM_PREFIX_LEN,
					XATTR_POSIX_ACL_DEFAULT,
					strlen(XATTR_POSIX_ACL_DEFAULT));
		}
#endif

		/*
		 * Only show root namespace entries if we are actually allowed to
		 * see them.
		 */
		if (!capable(CAP_SYS_ADMIN))
			return;

		prefix = XATTR_TRUSTED_PREFIX;
		prefix_len = XATTR_TRUSTED_PREFIX_LEN;

But I do see that rdump should translate SGI_ACL_FILE to
XATTR_POSIX_ACL_ACCESS so I'll go do that.

--D

> Ok.  I think at least scrub phase5 does this.
> 
> > > +	if (XFS_IS_REALTIME_INODE(ip))
> > > +		btp = ip->i_mount->m_rtdev_targp;
> > > +	else
> > > +		btp = ip->i_mount->m_ddev_targp;
> > 
> > Should be move xfs_inode_buftarg from kernel code to common code?
> 
> Hmm.  The xfs_inode -> xfs_buftarg translation could be moved to
> libxfs/xfs_inode_util.c, yes.  Though that can't happen until 6.15
> because we're well past the merge window.  For now I think it's the only
> place in xfsprogs where we do that.
> 
> --D
>