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

[Dave Chinner <david@fromorbit.com>]

On Sat, Dec 05, 2020 at 06:34:44AM -0500, Brian Foster wrote:
> On Sat, Dec 05, 2020 at 08:22:22AM +1100, Dave Chinner wrote:
> > On Fri, Dec 04, 2020 at 07:31:37AM -0500, Brian Foster wrote:
> > > On Thu, Dec 03, 2020 at 10:27:24AM +1100, Dave Chinner wrote:
> > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > > index 2bfbcf28b1bd..9ee2e0b4c6fd 100644
> > > > --- a/fs/xfs/xfs_inode.c
> > > > +++ b/fs/xfs/xfs_inode.c
> > > ...
> > > > @@ -918,6 +919,18 @@ xfs_ialloc(
> > > >  		ASSERT(0);
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * If we need to create attributes immediately after allocating the
> > > > +	 * inode, initialise an empty attribute fork right now. We use the
> > > > +	 * default fork offset for attributes here as we don't know exactly what
> > > > +	 * size or how many attributes we might be adding. We can do this safely
> > > > +	 * here because we know the data fork is completely empty right now.
> > > > +	 */
> > > > +	if (init_attrs) {
> > > > +		ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
> > > > +		ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
> > > > +	}
> > > > +
> > > 
> > > Seems reasonable in principle, but why not refactor
> > > xfs_bmap_add_attrfork() such that the internals (i.e. everything within
> > > the transaction/ilock code) can be properly reused in both contexts
> > > rather than open-coding (and thus duplicating) a somewhat stripped down
> > > version?
> > 
> > We don't know the size of the attribute that is being created, so
> > the attr size dependent parts of it can't be used.
> 
> Not sure I see the problem here. It looks to me that
> xfs_bmap_add_attrfork() would do the right thing if we just passed a
> size of zero.

Yes, but it also does an awful lot that we do not need.

> The only place the size value is actually used is down in
> xfs_attr_shortform_bytesfit(), and I'd expect that to identify that the
> requested size is <= than the current afork size (also zero for a newly
> allocated inode..?) and bail out.

RIght it ends up doing that because an uninitialised inode fork
(di_forkoff = 0) is the same size as the requested size of zero, and
then it does ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;

But that's decided another two function calls deep, after a lot of
branches and shifting and comparisons to determine that the attr
fork is empty. Yet we already know that the attr fork is empty here
so all that extra CPU work is completely unnecessary.

Keep in mind we do exactly the same thing in
xfs_bmap_forkoff_reset(). We don't care about all the setup stuff in
xfs_bmap_add_attrfork(), we just reset the attr fork offset to the
default if the attr fork had grown larger than the default offset.

> That said, I wouldn't be opposed to tweaking xfs_bmap_set_attrforkoff()
> by a line or two to just skip the shortform call if size == 0. Then we
> can be more explicit about the "size == 0 means preemptive fork alloc,
> use the default offset" use case and perhaps actually document it with
> some comments as well.

It just seems wrong to me to code a special case into some function
to optimise that special case when the code that needs the special
case has no need to call that function in the first place.....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com