Re: [PATCH V2] xfs: initialise attr fork on inode create
From: Christoph Hellwig <hch@infradead.org>
Date: 2021-02-25 08:10:40
This look really nice to me, but a few comments on the overall structure:
quoted hunk ↗ jump to hunk
+/* + * Set an inode attr fork offset based on the format of the data fork. + * + * If a size of zero is passed in, then caller does not know the size of + * the attribute that might be added (i.e. pre-emptive attr fork creation). + * Hence in this case just set the fork offset to the default so that we don't + * need to modify the supported attr format in the superblock. + */ int xfs_bmap_set_attrforkoff( struct xfs_inode *ip,@@ -1041,6 +1048,11 @@ xfs_bmap_set_attrforkoff( case XFS_DINODE_FMT_LOCAL: case XFS_DINODE_FMT_EXTENTS: case XFS_DINODE_FMT_BTREE: + if (size == 0) { + ASSERT(!version); + ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3; + break; + } ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size); if (!ip->i_d.di_forkoff) ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
I don't think cramming this special case into xfs_bmap_set_attrforkoff
makes a whole lot of sense. I'd rather just open code this logic into
the caller like this:
if (init_xattrs) {
ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
}
which seems a whole lot simpler and much more obvious than the rather
arcane calling conventions for this magic invocation of
xfs_bmap_set_attrforkoffxfs_bmap_set_attrforkoff.
+struct xfs_ifork *
+xfs_ifork_alloc(
+ enum xfs_dinode_fmt format,
+ xfs_extnum_t nextents)
+{
+ struct xfs_ifork *ifp;
+
+ ifp = kmem_cache_zalloc(xfs_ifork_zone, GFP_NOFS | __GFP_NOFAIL);
+ ifp->if_format = format;
+ ifp->if_nextents = nextents;
+ return ifp;
+}Please split the addition of xfs_ifork_alloc and the conversion of the existing calles into a prep patch.
- if (unlikely(ip->i_afp->if_format == 0)) /* pre IRIX 6.2 file system */ - ip->i_afp->if_format = XFS_DINODE_FMT_EXTENTS;
This check is lost. I think we're fine as we don't support such old file systems at all, but we should probably document this change (and maybe even split it into a separate prep patch).
quoted hunk ↗ jump to hunk
struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state); int xfs_iformat_data_fork(struct xfs_inode *, struct xfs_dinode *);diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 636ac13b1df2..95e3a5e6e5e2 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c@@ -773,6 +773,7 @@ xfs_init_new_inode( xfs_nlink_t nlink, dev_t rdev, prid_t prid, + bool init_xattrs, struct xfs_inode **ipp) { struct inode *dir = pip ? VFS_I(pip) : NULL;
So instead of passing the parameter down a few levels I think we can
just take the decision inside of xfs_init_new_inode with a simple check
like:
if (pip && nlink > 0 && !S_ISLNK(mode) &&
xfs_create_need_xattr(dir, default_acl, acl)) {
...
}
+static inline bool
+xfs_create_need_xattr(
+ struct inode *dir,
+ struct posix_acl *default_acl,
+ struct posix_acl *acl)
+{
+ if (acl)
+ return true;
+ if (default_acl)
+ return true;
+ if (!IS_ENABLED(CONFIG_SECURITY))
+ return false;
+ if (dir->i_sb->s_security)
+ return true;
+ return false;
+}This isn't XFS-specific. Please move it to fs.h and split it into another prep patch. Also this won't compile as-is as s_security only exists when CONFIG_SECURITY is defined, so the IS_ENABLED needs to be replaced with an ifdef.