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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Dec 07, 2020 at 11:30:18AM -0500, Brian Foster wrote:
> On Mon, Dec 07, 2020 at 10:33:22AM +1100, Dave Chinner wrote:
> > 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.
> > 
> 
> Hence the suggestion to refactor it..
> 
> > > 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.
> > 
> 
> xfs_bmap_add_attrfork() already asserts that the attr fork is
> nonexistent at the very top of the function, for one. The 25-30 lines of
> that function that we need can be trivially lifted out into a new helper
> that can equally as trivially accommodate the size == 0 case and skip
> all those shortform calculations.
> 
> > 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.
> > 
> 
> I'm not arguing that the attr fork needs to be set up in a particular
> way on initial creation. I'm arguing that we don't need yet a third
> unique code path to set/initialize a default/empty attr fork. We can
> slowly normalize them all to the _reset() technique you're effectively
> reimplementing here if that works well enough and is preferred...
> 
> > > 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.....
> > 
> 
> I'm not sure what's so odd or controversial about refactoring and
> reusing an existing operational (i.e. add fork) function to facilitate
> review and future maintenance of that particular operation being
> performed from new and various contexts. And speaking in generalities
> like this just obfuscates and overcomplicates the argument. Let's be
> clear, we're literally arguing over a delta that would look something
> like this:
> 
> xfs_bmap_set_attrforkoff()
> {
> ...
> +		if (size)
> -               ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size);
> +			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;
> 		...
> }
> 
> Given all of that, I'm not convinced this is nearly the problem you seem
> to insinuate, yet I also don't think I'll convince you otherwise so it's
> probably not worth continuing to debate. You have my feedback, I'll let
> others determine how this patch comes together from here...

Alternate take: If you /are/ going to have a "init empty attr fork"
shortcut function, can it at least be placed next to the regular one in
xfs_bmap.c so that the di_forkoff setters are only split across three
source files instead of four?

--D

> Brian
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 
>