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

[Dave Chinner <david@fromorbit.com>]

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:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > When we allocate a new inode, we often need to add an attribute to
> > the inode as part of the create. This can happen as a result of
> > needing to add default ACLs or security labels before the inode is
> > made visible to userspace.
> > 
> > This is highly inefficient right now. We do the create transaction
> > to allocate the inode, then we do an "add attr fork" transaction to
> > modify the just created empty inode to set the inode fork offset to
> > allow attributes to be stored, then we go and do the attribute
> > creation.
> > 
> > This means 3 transactions instead of 1 to allocate an inode, and
> > this greatly increases the load on the CIL commit code, resulting in
> > excessive contention on the CIL spin locks and performance
> > degradation:
> > 
> >  18.99%  [kernel]                [k] __pv_queued_spin_lock_slowpath
> >   3.57%  [kernel]                [k] do_raw_spin_lock
> >   2.51%  [kernel]                [k] __raw_callee_save___pv_queued_spin_unlock
> >   2.48%  [kernel]                [k] memcpy
> >   2.34%  [kernel]                [k] xfs_log_commit_cil
> > 
> > The typical profile resulting from running fsmark on a selinux enabled
> > filesytem is adds this overhead to the create path:
> > 
> ...
> > 
> > And fsmark creation rate performance drops by ~25%. The key point to
> > note here is that half the additional overhead comes from adding the
> > attribute fork to the newly created inode. That's crazy, considering
> > we can do this same thing at inode create time with a couple of
> > lines of code and no extra overhead.
> > 
> > So, if we know we are going to add an attribute immediately after
> > creating the inode, let's just initialise the attribute fork inside
> > the create transaction and chop that whole chunk of code out of
> > the create fast path. This completely removes the performance
> > drop caused by enabling SELinux, and the profile looks like:
> > 
> ...
> > 
> > Which indicates the XFS overhead of creating the selinux xattr has
> > been halved. This doesn't fix the CIL lock contention problem, just
> > means it's not a limiting factor for this workload. Lock contention
> > in the security subsystems is going to be an issue soon, though...
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_inode_fork.c | 20 +++++++++++++++-----
> >  fs/xfs/libxfs/xfs_inode_fork.h |  1 +
> >  fs/xfs/xfs_inode.c             | 24 ++++++++++++++++++++----
> >  fs/xfs/xfs_inode.h             |  5 +++--
> >  fs/xfs/xfs_iops.c              | 10 +++++++++-
> >  fs/xfs/xfs_qm.c                |  2 +-
> >  fs/xfs/xfs_symlink.c           |  2 +-
> >  7 files changed, 50 insertions(+), 14 deletions(-)
> > 
> ...
> > 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.

> At a glance, it looks like there are some subtle differences in
> the initial setup of the attr fork for a device node inode, for example.

Yes, there's a difference, but it's largely irrelevant as adding
the first attribute to a device format inode will reset the
forkoffset to the min via xfs_attr_shortform_bytesfit().

And if the attribute is larger than will fit in the default fork
offset space, but can fit the attr in shrotform by shrinking the
empty data fork space, xfs_attr_shortform_bytesfit() will do that as
well. IOWs, we only need to set a non-zero fork offset here and init
the ip->i_afp pointer - immediately setting an attribute on the
empty inode literal area will do the rest for the fork offset setup
for us...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com