This look really nice to me, but a few comments on the overall
structure:
> +/*
> + * 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).
> 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.
