Re: [PATCH V2] xfs: initialise attr fork on inode create

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

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@fromorbit.com