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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
