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...
Brian
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
