On Fri, Feb 26, 2021 at 07:32:12AM +1100, Dave Chinner wrote:
> 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 don't recall being adamant that you use xfs_bmap_set_attrforkoff here;
I think I only didn't want forkoff setter functions getting scattered
all over the codebase.
Regardless of whatever I said for V1, now I can see what exactly that
looks like, and I don't like it. xfs_bmap_set_attrforkoff is the
function you call to set forkoff when you want to set an xattr of a
particular size, and need to adjust forkoff. We don't know what the
security xattr(s) are going to be yet, so this seems like a misuse of
that function.
Just do it the way Christoph said.
> 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.
IRIX 6.2 was released what, 25 years ago? Probably fine to just turn
that into a comment.
> > > 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.
Agreed, the next fs to want this can hoist it.
--D
>
> > 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
