On Thu, Feb 25, 2021 at 08:09:00AM +0000, Christoph Hellwig wrote:
> 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.
AAARRRRGGGGGHHHHH!
That's exactly what I did with the first version and Brian and then
Darrick were both adamant that setting the attr fork had to be done
through xfs_bmap_set_attrforkoff() via formalising "size=0 means use
defaults".
I know, just doing it the way you suggest is simple, obvious and
straight forward and that's exactly the argument I made, but nobody
else wanted it that way.
>
> > +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).
I don't see any point in splitting it out into a separate patch.
It's dead code, so while I'm touching this exact piece of code. I'll
document it.
> > 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)) {
> ...
> }
We don't pass down the acl/default acl to this function. We have to
pass something down into here for it to do the right thing....
>
> > +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.
No. I'm not putting a special, targetted one-off
filesystem-implementation specific functions in fs.h even if it only
contain generic checks. There's already way too much crap in fs.h,
and this doesn't improve the situation. If you have need for it in
other filesystems, then pull it up out of the XFS code in that
patchset.
> 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.
I'll fix that.
Cheers,
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
