* [PATCH 1/8] xfs: initialise attr fork on inode create
2021-03-17 4:56 [PATCH v4 0/8] xfs: miscellaneous optimisations Dave Chinner
@ 2021-03-17 4:56 ` Dave Chinner
2021-03-17 16:44 ` Gao Xiang
2021-03-17 4:57 ` [PATCH 2/8] xfs: reduce buffer log item shadow allocations Dave Chinner
` (6 subsequent siblings)
7 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2021-03-17 4:56 UTC (permalink / raw)
To: linux-xfs
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:
- 15.30% xfs_init_security
- 15.23% security_inode_init_security
- 13.05% xfs_initxattrs
- 12.94% xfs_attr_set
- 6.75% xfs_bmap_add_attrfork
- 5.51% xfs_trans_commit
- 5.48% __xfs_trans_commit
- 5.35% xfs_log_commit_cil
- 3.86% _raw_spin_lock
- do_raw_spin_lock
__pv_queued_spin_lock_slowpath
- 0.70% xfs_trans_alloc
0.52% xfs_trans_reserve
- 5.41% xfs_attr_set_args
- 5.39% xfs_attr_set_shortform.constprop.0
- 4.46% xfs_trans_commit
- 4.46% __xfs_trans_commit
- 4.33% xfs_log_commit_cil
- 2.74% _raw_spin_lock
- do_raw_spin_lock
__pv_queued_spin_lock_slowpath
0.60% xfs_inode_item_format
0.90% xfs_attr_try_sf_addname
- 1.99% selinux_inode_init_security
- 1.02% security_sid_to_context_force
- 1.00% security_sid_to_context_core
- 0.92% sidtab_entry_to_string
- 0.90% sidtab_sid2str_get
0.59% sidtab_sid2str_put.part.0
- 0.82% selinux_determine_inode_label
- 0.77% security_transition_sid
0.70% security_compute_sid.part.0
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:
- 8.99% xfs_init_security
- 9.00% security_inode_init_security
- 6.43% xfs_initxattrs
- 6.37% xfs_attr_set
- 5.45% xfs_attr_set_args
- 5.42% xfs_attr_set_shortform.constprop.0
- 4.51% xfs_trans_commit
- 4.54% __xfs_trans_commit
- 4.59% xfs_log_commit_cil
- 2.67% _raw_spin_lock
- 3.28% do_raw_spin_lock
3.08% __pv_queued_spin_lock_slowpath
0.66% xfs_inode_item_format
- 0.90% xfs_attr_try_sf_addname
- 0.60% xfs_trans_alloc
- 2.35% selinux_inode_init_security
- 1.25% security_sid_to_context_force
- 1.21% security_sid_to_context_core
- 1.19% sidtab_entry_to_string
- 1.20% sidtab_sid2str_get
- 0.86% sidtab_sid2str_put.part.0
- 0.62% _raw_spin_lock_irqsave
- 0.77% do_raw_spin_lock
__pv_queued_spin_lock_slowpath
- 0.84% selinux_determine_inode_label
- 0.83% security_transition_sid
0.86% security_compute_sid.part.0
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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/libxfs/xfs_bmap.c | 9 ++++-----
fs/xfs/libxfs/xfs_inode_fork.c | 20 +++++++++++++++-----
fs/xfs/libxfs/xfs_inode_fork.h | 2 ++
fs/xfs/xfs_inode.c | 24 +++++++++++++++++++++---
fs/xfs/xfs_inode.h | 6 ++++--
fs/xfs/xfs_iops.c | 34 +++++++++++++++++++++++++++++++++-
fs/xfs/xfs_qm.c | 2 +-
fs/xfs/xfs_symlink.c | 2 +-
8 files changed, 81 insertions(+), 18 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index e0905ad171f0..5574d345d066 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1027,7 +1027,9 @@ xfs_bmap_add_attrfork_local(
return -EFSCORRUPTED;
}
-/* Set an inode attr fork off based on the format */
+/*
+ * Set an inode attr fork offset based on the format of the data fork.
+ */
int
xfs_bmap_set_attrforkoff(
struct xfs_inode *ip,
@@ -1092,10 +1094,7 @@ xfs_bmap_add_attrfork(
goto trans_cancel;
ASSERT(ip->i_afp == NULL);
- ip->i_afp = kmem_cache_zalloc(xfs_ifork_zone,
- GFP_KERNEL | __GFP_NOFAIL);
-
- ip->i_afp->if_format = XFS_DINODE_FMT_EXTENTS;
+ ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
ip->i_afp->if_flags = XFS_IFEXTENTS;
logflags = 0;
switch (ip->i_df.if_format) {
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index e080d7e07643..c606c1a77e5a 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -282,6 +282,19 @@ xfs_dfork_attr_shortform_size(
return be16_to_cpu(atp->hdr.totsize);
}
+struct xfs_ifork *
+xfs_ifork_alloc(
+ enum xfs_dinode_fmt format,
+ xfs_extnum_t nextents)
+{
+ struct xfs_ifork *ifp;
+
+ ifp = kmem_cache_zalloc(xfs_ifork_zone, GFP_NOFS | __GFP_NOFAIL);
+ ifp->if_format = format;
+ ifp->if_nextents = nextents;
+ return ifp;
+}
+
int
xfs_iformat_attr_fork(
struct xfs_inode *ip,
@@ -293,11 +306,8 @@ xfs_iformat_attr_fork(
* Initialize the extent count early, as the per-format routines may
* depend on it.
*/
- ip->i_afp = kmem_cache_zalloc(xfs_ifork_zone, GFP_NOFS | __GFP_NOFAIL);
- ip->i_afp->if_format = dip->di_aformat;
- if (unlikely(ip->i_afp->if_format == 0)) /* pre IRIX 6.2 file system */
- ip->i_afp->if_format = XFS_DINODE_FMT_EXTENTS;
- ip->i_afp->if_nextents = be16_to_cpu(dip->di_anextents);
+ ip->i_afp = xfs_ifork_alloc(dip->di_aformat,
+ be16_to_cpu(dip->di_anextents));
switch (ip->i_afp->if_format) {
case XFS_DINODE_FMT_LOCAL:
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 9e2137cd7372..a0717ab0e5c5 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -141,6 +141,8 @@ static inline int8_t xfs_ifork_format(struct xfs_ifork *ifp)
return ifp->if_format;
}
+struct xfs_ifork *xfs_ifork_alloc(enum xfs_dinode_fmt format,
+ xfs_extnum_t nextents);
struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
int xfs_iformat_data_fork(struct xfs_inode *, struct xfs_dinode *);
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index f93370bd7b1e..97c5dd081ead 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -774,6 +774,7 @@ xfs_init_new_inode(
xfs_nlink_t nlink,
dev_t rdev,
prid_t prid,
+ bool init_xattrs,
struct xfs_inode **ipp)
{
struct inode *dir = pip ? VFS_I(pip) : NULL;
@@ -877,6 +878,20 @@ xfs_init_new_inode(
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 and
+ * this saves us from needing to run a separate transaction to set the
+ * fork offset in the immediate future.
+ */
+ if (init_xattrs) {
+ ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3;
+ ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
+ }
+
/*
* Log the new values stuffed into the inode.
*/
@@ -910,6 +925,7 @@ xfs_dir_ialloc(
xfs_nlink_t nlink,
dev_t rdev,
prid_t prid,
+ bool init_xattrs,
struct xfs_inode **ipp)
{
struct xfs_buf *agibp;
@@ -937,7 +953,7 @@ xfs_dir_ialloc(
ASSERT(ino != NULLFSINO);
return xfs_init_new_inode(mnt_userns, *tpp, dp, ino, mode, nlink, rdev,
- prid, ipp);
+ prid, init_xattrs, ipp);
}
/*
@@ -982,6 +998,7 @@ xfs_create(
struct xfs_name *name,
umode_t mode,
dev_t rdev,
+ bool init_xattrs,
xfs_inode_t **ipp)
{
int is_dir = S_ISDIR(mode);
@@ -1053,7 +1070,7 @@ xfs_create(
* pointing to itself.
*/
error = xfs_dir_ialloc(mnt_userns, &tp, dp, mode, is_dir ? 2 : 1, rdev,
- prid, &ip);
+ prid, init_xattrs, &ip);
if (error)
goto out_trans_cancel;
@@ -1173,7 +1190,8 @@ xfs_create_tmpfile(
if (error)
goto out_release_dquots;
- error = xfs_dir_ialloc(mnt_userns, &tp, dp, mode, 0, 0, prid, &ip);
+ error = xfs_dir_ialloc(mnt_userns, &tp, dp, mode, 0, 0, prid,
+ false, &ip);
if (error)
goto out_trans_cancel;
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 88ee4c3930ae..a2cacdb76d55 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -371,7 +371,8 @@ int xfs_lookup(struct xfs_inode *dp, struct xfs_name *name,
struct xfs_inode **ipp, struct xfs_name *ci_name);
int xfs_create(struct user_namespace *mnt_userns,
struct xfs_inode *dp, struct xfs_name *name,
- umode_t mode, dev_t rdev, struct xfs_inode **ipp);
+ umode_t mode, dev_t rdev, bool need_xattr,
+ struct xfs_inode **ipp);
int xfs_create_tmpfile(struct user_namespace *mnt_userns,
struct xfs_inode *dp, umode_t mode,
struct xfs_inode **ipp);
@@ -413,7 +414,8 @@ xfs_extlen_t xfs_get_cowextsz_hint(struct xfs_inode *ip);
int xfs_dir_ialloc(struct user_namespace *mnt_userns,
struct xfs_trans **tpp, struct xfs_inode *dp,
umode_t mode, xfs_nlink_t nlink, dev_t dev,
- prid_t prid, struct xfs_inode **ipp);
+ prid_t prid, bool need_xattr,
+ struct xfs_inode **ipp);
static inline int
xfs_itruncate_extents(
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 66ebccb5a6ff..a9d466b78646 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -126,6 +126,37 @@ xfs_cleanup_inode(
xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
}
+/*
+ * Check to see if we are likely to need an extended attribute to be added to
+ * the inode we are about to allocate. This allows the attribute fork to be
+ * created during the inode allocation, reducing the number of transactions we
+ * need to do in this fast path.
+ *
+ * The security checks are optimistic, but not guaranteed. The two LSMs that
+ * require xattrs to be added here (selinux and smack) are also the only two
+ * LSMs that add a sb->s_security structure to the superblock. Hence if security
+ * is enabled and sb->s_security is set, we have a pretty good idea that we are
+ * going to be asked to add a security xattr immediately after allocating the
+ * xfs inode and instantiating the VFS inode.
+ */
+static inline bool
+xfs_create_need_xattr(
+ struct inode *dir,
+ struct posix_acl *default_acl,
+ struct posix_acl *acl)
+{
+ if (acl)
+ return true;
+ if (default_acl)
+ return true;
+ if (!IS_ENABLED(CONFIG_SECURITY))
+ return false;
+ if (dir->i_sb->s_security)
+ return true;
+ return false;
+}
+
+
STATIC int
xfs_generic_create(
struct user_namespace *mnt_userns,
@@ -163,7 +194,8 @@ xfs_generic_create(
if (!tmpfile) {
error = xfs_create(mnt_userns, XFS_I(dir), &name, mode, rdev,
- &ip);
+ xfs_create_need_xattr(dir, default_acl, acl),
+ &ip);
} else {
error = xfs_create_tmpfile(mnt_userns, XFS_I(dir), mode, &ip);
}
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index bfa4164990b1..6fde318b9fed 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -788,7 +788,7 @@ xfs_qm_qino_alloc(
if (need_alloc) {
error = xfs_dir_ialloc(&init_user_ns, &tp, NULL, S_IFREG, 1, 0,
- 0, ipp);
+ 0, false, ipp);
if (error) {
xfs_trans_cancel(tp);
return error;
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 7f368b10ded1..db9c400f9258 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -224,7 +224,7 @@ xfs_symlink(
* Allocate an inode for the symlink.
*/
error = xfs_dir_ialloc(mnt_userns, &tp, dp, S_IFLNK | (mode & ~S_IFMT),
- 1, 0, prid, &ip);
+ 1, 0, prid, false, &ip);
if (error)
goto out_trans_cancel;
--
2.30.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/8] xfs: initialise attr fork on inode create
2021-03-17 4:56 ` [PATCH 1/8] xfs: initialise attr fork on inode create Dave Chinner
@ 2021-03-17 16:44 ` Gao Xiang
0 siblings, 0 replies; 11+ messages in thread
From: Gao Xiang @ 2021-03-17 16:44 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Wed, Mar 17, 2021 at 03:56:59PM +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:
>
> - 15.30% xfs_init_security
> - 15.23% security_inode_init_security
> - 13.05% xfs_initxattrs
> - 12.94% xfs_attr_set
> - 6.75% xfs_bmap_add_attrfork
> - 5.51% xfs_trans_commit
> - 5.48% __xfs_trans_commit
> - 5.35% xfs_log_commit_cil
> - 3.86% _raw_spin_lock
> - do_raw_spin_lock
> __pv_queued_spin_lock_slowpath
> - 0.70% xfs_trans_alloc
> 0.52% xfs_trans_reserve
> - 5.41% xfs_attr_set_args
> - 5.39% xfs_attr_set_shortform.constprop.0
> - 4.46% xfs_trans_commit
> - 4.46% __xfs_trans_commit
> - 4.33% xfs_log_commit_cil
> - 2.74% _raw_spin_lock
> - do_raw_spin_lock
> __pv_queued_spin_lock_slowpath
> 0.60% xfs_inode_item_format
> 0.90% xfs_attr_try_sf_addname
> - 1.99% selinux_inode_init_security
> - 1.02% security_sid_to_context_force
> - 1.00% security_sid_to_context_core
> - 0.92% sidtab_entry_to_string
> - 0.90% sidtab_sid2str_get
> 0.59% sidtab_sid2str_put.part.0
> - 0.82% selinux_determine_inode_label
> - 0.77% security_transition_sid
> 0.70% security_compute_sid.part.0
>
> 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:
>
> - 8.99% xfs_init_security
> - 9.00% security_inode_init_security
> - 6.43% xfs_initxattrs
> - 6.37% xfs_attr_set
> - 5.45% xfs_attr_set_args
> - 5.42% xfs_attr_set_shortform.constprop.0
> - 4.51% xfs_trans_commit
> - 4.54% __xfs_trans_commit
> - 4.59% xfs_log_commit_cil
> - 2.67% _raw_spin_lock
> - 3.28% do_raw_spin_lock
> 3.08% __pv_queued_spin_lock_slowpath
> 0.66% xfs_inode_item_format
> - 0.90% xfs_attr_try_sf_addname
> - 0.60% xfs_trans_alloc
> - 2.35% selinux_inode_init_security
> - 1.25% security_sid_to_context_force
> - 1.21% security_sid_to_context_core
> - 1.19% sidtab_entry_to_string
> - 1.20% sidtab_sid2str_get
> - 0.86% sidtab_sid2str_put.part.0
> - 0.62% _raw_spin_lock_irqsave
> - 0.77% do_raw_spin_lock
> __pv_queued_spin_lock_slowpath
> - 0.84% selinux_determine_inode_label
> - 0.83% security_transition_sid
> 0.86% security_compute_sid.part.0
>
> 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>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Looks good to me, although I've also noticed i_afp->if_format == 0
case, and checked the previous discussion about this as well:
Reviewed-by: Gao Xiang <hsiangkao@redhat.com>
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/8] xfs: reduce buffer log item shadow allocations
2021-03-17 4:56 [PATCH v4 0/8] xfs: miscellaneous optimisations Dave Chinner
2021-03-17 4:56 ` [PATCH 1/8] xfs: initialise attr fork on inode create Dave Chinner
@ 2021-03-17 4:57 ` Dave Chinner
2021-03-17 16:52 ` Gao Xiang
2021-03-17 4:57 ` [PATCH 3/8] xfs: xfs_buf_item_size_segment() needs to pass segment offset Dave Chinner
` (5 subsequent siblings)
7 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2021-03-17 4:57 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
When we modify btrees repeatedly, we regularly increase the size of
the logged region by a single chunk at a time (per transaction
commit). This results in the CIL formatting code having to
reallocate the log vector buffer every time the buffer dirty region
grows. Hence over a typical 4kB btree buffer, we might grow the log
vector 4096/128 = 32x over a short period where we repeatedly add
or remove records to/from the buffer over a series of running
transaction. This means we are doing 32 memory allocations and frees
over this time during a performance critical path in the journal.
The amount of space tracked in the CIL for the object is calculated
during the ->iop_format() call for the buffer log item, but the
buffer memory allocated for it is calculated by the ->iop_size()
call. The size callout determines the size of the buffer, the format
call determines the space used in the buffer.
Hence we can oversize the buffer space required in the size
calculation without impacting the amount of space used and accounted
to the CIL for the changes being logged. This allows us to reduce
the number of allocations by rounding up the buffer size to allow
for future growth. This can safe a substantial amount of CPU time in
this path:
- 46.52% 2.02% [kernel] [k] xfs_log_commit_cil
- 44.49% xfs_log_commit_cil
- 30.78% _raw_spin_lock
- 30.75% do_raw_spin_lock
30.27% __pv_queued_spin_lock_slowpath
(oh, ouch!)
....
- 1.05% kmem_alloc_large
- 1.02% kmem_alloc
0.94% __kmalloc
This overhead here us what this patch is aimed at. After:
- 0.76% kmem_alloc_large
- 0.75% kmem_alloc
0.70% __kmalloc
The size of 512 bytes is based on the bitmap chunk size being 128
bytes and that random directory entry updates almost never require
more than 3-4 128 byte regions to be logged in the directory block.
The other observation is for per-ag btrees. When we are inserting
into a new btree block, we'll pack it from the front. Hence the
first few records land in the first 128 bytes so we log only 128
bytes, the next 8-16 records land in the second region so now we log
256 bytes. And so on. If we are doing random updates, it will only
allocate every 4 random 128 byte regions that are dirtied instead of
every single one.
Any larger than 512 bytes and I noticed an increase in memory
footprint in my scalability workloads. Any less than this and I
didn't really see any significant benefit to CPU usage.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf_item.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index dc0be2a639cc..cb8fd8afd140 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -143,6 +143,7 @@ xfs_buf_item_size(
{
struct xfs_buf_log_item *bip = BUF_ITEM(lip);
int i;
+ int bytes;
ASSERT(atomic_read(&bip->bli_refcount) > 0);
if (bip->bli_flags & XFS_BLI_STALE) {
@@ -174,7 +175,7 @@ xfs_buf_item_size(
}
/*
- * the vector count is based on the number of buffer vectors we have
+ * The vector count is based on the number of buffer vectors we have
* dirty bits in. This will only be greater than one when we have a
* compound buffer with more than one segment dirty. Hence for compound
* buffers we need to track which segment the dirty bits correspond to,
@@ -182,10 +183,18 @@ xfs_buf_item_size(
* count for the extra buf log format structure that will need to be
* written.
*/
+ bytes = 0;
for (i = 0; i < bip->bli_format_count; i++) {
xfs_buf_item_size_segment(bip, &bip->bli_formats[i],
- nvecs, nbytes);
+ nvecs, &bytes);
}
+
+ /*
+ * Round up the buffer size required to minimise the number of memory
+ * allocations that need to be done as this item grows when relogged by
+ * repeated modifications.
+ */
+ *nbytes = round_up(bytes, 512);
trace_xfs_buf_item_size(bip);
}
--
2.30.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/8] xfs: reduce buffer log item shadow allocations
2021-03-17 4:57 ` [PATCH 2/8] xfs: reduce buffer log item shadow allocations Dave Chinner
@ 2021-03-17 16:52 ` Gao Xiang
0 siblings, 0 replies; 11+ messages in thread
From: Gao Xiang @ 2021-03-17 16:52 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Wed, Mar 17, 2021 at 03:57:00PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> When we modify btrees repeatedly, we regularly increase the size of
> the logged region by a single chunk at a time (per transaction
> commit). This results in the CIL formatting code having to
> reallocate the log vector buffer every time the buffer dirty region
> grows. Hence over a typical 4kB btree buffer, we might grow the log
> vector 4096/128 = 32x over a short period where we repeatedly add
> or remove records to/from the buffer over a series of running
> transaction. This means we are doing 32 memory allocations and frees
> over this time during a performance critical path in the journal.
>
> The amount of space tracked in the CIL for the object is calculated
> during the ->iop_format() call for the buffer log item, but the
> buffer memory allocated for it is calculated by the ->iop_size()
> call. The size callout determines the size of the buffer, the format
> call determines the space used in the buffer.
>
> Hence we can oversize the buffer space required in the size
> calculation without impacting the amount of space used and accounted
> to the CIL for the changes being logged. This allows us to reduce
> the number of allocations by rounding up the buffer size to allow
> for future growth. This can safe a substantial amount of CPU time in
> this path:
>
> - 46.52% 2.02% [kernel] [k] xfs_log_commit_cil
> - 44.49% xfs_log_commit_cil
> - 30.78% _raw_spin_lock
> - 30.75% do_raw_spin_lock
> 30.27% __pv_queued_spin_lock_slowpath
>
> (oh, ouch!)
> ....
> - 1.05% kmem_alloc_large
> - 1.02% kmem_alloc
> 0.94% __kmalloc
>
> This overhead here us what this patch is aimed at. After:
>
> - 0.76% kmem_alloc_large
> - 0.75% kmem_alloc
> 0.70% __kmalloc
>
> The size of 512 bytes is based on the bitmap chunk size being 128
> bytes and that random directory entry updates almost never require
> more than 3-4 128 byte regions to be logged in the directory block.
>
> The other observation is for per-ag btrees. When we are inserting
> into a new btree block, we'll pack it from the front. Hence the
> first few records land in the first 128 bytes so we log only 128
> bytes, the next 8-16 records land in the second region so now we log
> 256 bytes. And so on. If we are doing random updates, it will only
> allocate every 4 random 128 byte regions that are dirtied instead of
> every single one.
>
> Any larger than 512 bytes and I noticed an increase in memory
> footprint in my scalability workloads. Any less than this and I
> didn't really see any significant benefit to CPU usage.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Gao Xiang <hsiangkao@redhat.com>
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/8] xfs: xfs_buf_item_size_segment() needs to pass segment offset
2021-03-17 4:56 [PATCH v4 0/8] xfs: miscellaneous optimisations Dave Chinner
2021-03-17 4:56 ` [PATCH 1/8] xfs: initialise attr fork on inode create Dave Chinner
2021-03-17 4:57 ` [PATCH 2/8] xfs: reduce buffer log item shadow allocations Dave Chinner
@ 2021-03-17 4:57 ` Dave Chinner
2021-03-17 4:57 ` [PATCH 4/8] xfs: optimise xfs_buf_item_size/format for contiguous regions Dave Chinner
` (4 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2021-03-17 4:57 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
Otherwise it doesn't correctly calculate the number of vectors
in a logged buffer that has a contiguous map that gets split into
multiple regions because the range spans discontigous memory.
Probably never been hit in practice - we don't log contiguous ranges
on unmapped buffers (inode clusters).
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf_item.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index cb8fd8afd140..189a4534e0b2 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -55,6 +55,18 @@ xfs_buf_log_format_size(
(blfp->blf_map_size * sizeof(blfp->blf_data_map[0]));
}
+static inline bool
+xfs_buf_item_straddle(
+ struct xfs_buf *bp,
+ uint offset,
+ int next_bit,
+ int last_bit)
+{
+ return xfs_buf_offset(bp, offset + (next_bit << XFS_BLF_SHIFT)) !=
+ (xfs_buf_offset(bp, offset + (last_bit << XFS_BLF_SHIFT)) +
+ XFS_BLF_CHUNK);
+}
+
/*
* This returns the number of log iovecs needed to log the
* given buf log item.
@@ -69,6 +81,7 @@ STATIC void
xfs_buf_item_size_segment(
struct xfs_buf_log_item *bip,
struct xfs_buf_log_format *blfp,
+ uint offset,
int *nvecs,
int *nbytes)
{
@@ -103,12 +116,8 @@ xfs_buf_item_size_segment(
*/
if (next_bit == -1) {
break;
- } else if (next_bit != last_bit + 1) {
- last_bit = next_bit;
- (*nvecs)++;
- } else if (xfs_buf_offset(bp, next_bit * XFS_BLF_CHUNK) !=
- (xfs_buf_offset(bp, last_bit * XFS_BLF_CHUNK) +
- XFS_BLF_CHUNK)) {
+ } else if (next_bit != last_bit + 1 ||
+ xfs_buf_item_straddle(bp, offset, next_bit, last_bit)) {
last_bit = next_bit;
(*nvecs)++;
} else {
@@ -142,8 +151,10 @@ xfs_buf_item_size(
int *nbytes)
{
struct xfs_buf_log_item *bip = BUF_ITEM(lip);
+ struct xfs_buf *bp = bip->bli_buf;
int i;
int bytes;
+ uint offset = 0;
ASSERT(atomic_read(&bip->bli_refcount) > 0);
if (bip->bli_flags & XFS_BLI_STALE) {
@@ -185,8 +196,9 @@ xfs_buf_item_size(
*/
bytes = 0;
for (i = 0; i < bip->bli_format_count; i++) {
- xfs_buf_item_size_segment(bip, &bip->bli_formats[i],
+ xfs_buf_item_size_segment(bip, &bip->bli_formats[i], offset,
nvecs, &bytes);
+ offset += BBTOB(bp->b_maps[i].bm_len);
}
/*
@@ -213,18 +225,6 @@ xfs_buf_item_copy_iovec(
nbits * XFS_BLF_CHUNK);
}
-static inline bool
-xfs_buf_item_straddle(
- struct xfs_buf *bp,
- uint offset,
- int next_bit,
- int last_bit)
-{
- return xfs_buf_offset(bp, offset + (next_bit << XFS_BLF_SHIFT)) !=
- (xfs_buf_offset(bp, offset + (last_bit << XFS_BLF_SHIFT)) +
- XFS_BLF_CHUNK);
-}
-
static void
xfs_buf_item_format_segment(
struct xfs_buf_log_item *bip,
--
2.30.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 4/8] xfs: optimise xfs_buf_item_size/format for contiguous regions
2021-03-17 4:56 [PATCH v4 0/8] xfs: miscellaneous optimisations Dave Chinner
` (2 preceding siblings ...)
2021-03-17 4:57 ` [PATCH 3/8] xfs: xfs_buf_item_size_segment() needs to pass segment offset Dave Chinner
@ 2021-03-17 4:57 ` Dave Chinner
2021-03-17 4:57 ` [PATCH 5/8] xfs: type verification is expensive Dave Chinner
` (3 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2021-03-17 4:57 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
We process the buf_log_item bitmap one set bit at a time with
xfs_next_bit() so we can detect if a region crosses a memcpy
discontinuity in the buffer data address. This has massive overhead
on large buffers (e.g. 64k directory blocks) because we do a lot of
unnecessary checks and xfs_buf_offset() calls.
For example, 16-way concurrent create workload on debug kernel
running CPU bound has this at the top of the profile at ~120k
create/s on 64kb directory block size:
20.66% [kernel] [k] xfs_dir3_leaf_check_int
7.10% [kernel] [k] memcpy
6.22% [kernel] [k] xfs_next_bit
3.55% [kernel] [k] xfs_buf_offset
3.53% [kernel] [k] xfs_buf_item_format
3.34% [kernel] [k] __pv_queued_spin_lock_slowpath
3.04% [kernel] [k] do_raw_spin_lock
2.84% [kernel] [k] xfs_buf_item_size_segment.isra.0
2.31% [kernel] [k] __raw_callee_save___pv_queued_spin_unlock
1.36% [kernel] [k] xfs_log_commit_cil
(debug checks hurt large blocks)
The only buffers with discontinuities in the data address are
unmapped buffers, and they are only used for inode cluster buffers
and only for logging unlinked pointers. IOWs, it is -rare- that we
even need to detect a discontinuity in the buffer item formatting
code.
Optimise all this by using xfs_contig_bits() to find the size of
the contiguous regions, then test for a discontiunity inside it. If
we find one, do the slow "bit at a time" method we do now. If we
don't, then just copy the entire contiguous range in one go.
Profile now looks like:
25.26% [kernel] [k] xfs_dir3_leaf_check_int
9.25% [kernel] [k] memcpy
5.01% [kernel] [k] __pv_queued_spin_lock_slowpath
2.84% [kernel] [k] do_raw_spin_lock
2.22% [kernel] [k] __raw_callee_save___pv_queued_spin_unlock
1.88% [kernel] [k] xfs_buf_find
1.53% [kernel] [k] memmove
1.47% [kernel] [k] xfs_log_commit_cil
....
0.34% [kernel] [k] xfs_buf_item_format
....
0.21% [kernel] [k] xfs_buf_offset
....
0.16% [kernel] [k] xfs_contig_bits
....
0.13% [kernel] [k] xfs_buf_item_size_segment.isra.0
So the bit scanning over for the dirty region tracking for the
buffer log items is basically gone. Debug overhead hurts even more
now...
Perf comparison
dir block creates unlink
size (kb) time rate time
Original 4 4m08s 220k 5m13s
Original 64 7m21s 115k 13m25s
Patched 4 3m59s 230k 5m03s
Patched 64 6m23s 143k 12m33s
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf_item.c | 102 +++++++++++++++++++++++++++++++++++-------
1 file changed, 87 insertions(+), 15 deletions(-)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 189a4534e0b2..fb69879e4b2b 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -59,12 +59,18 @@ static inline bool
xfs_buf_item_straddle(
struct xfs_buf *bp,
uint offset,
- int next_bit,
- int last_bit)
+ int first_bit,
+ int nbits)
{
- return xfs_buf_offset(bp, offset + (next_bit << XFS_BLF_SHIFT)) !=
- (xfs_buf_offset(bp, offset + (last_bit << XFS_BLF_SHIFT)) +
- XFS_BLF_CHUNK);
+ void *first, *last;
+
+ first = xfs_buf_offset(bp, offset + (first_bit << XFS_BLF_SHIFT));
+ last = xfs_buf_offset(bp,
+ offset + ((first_bit + nbits) << XFS_BLF_SHIFT));
+
+ if (last - first != nbits * XFS_BLF_CHUNK)
+ return true;
+ return false;
}
/*
@@ -86,20 +92,51 @@ xfs_buf_item_size_segment(
int *nbytes)
{
struct xfs_buf *bp = bip->bli_buf;
+ int first_bit;
+ int nbits;
int next_bit;
int last_bit;
- last_bit = xfs_next_bit(blfp->blf_data_map, blfp->blf_map_size, 0);
- if (last_bit == -1)
+ first_bit = xfs_next_bit(blfp->blf_data_map, blfp->blf_map_size, 0);
+ if (first_bit == -1)
return;
- /*
- * initial count for a dirty buffer is 2 vectors - the format structure
- * and the first dirty region.
- */
- *nvecs += 2;
- *nbytes += xfs_buf_log_format_size(blfp) + XFS_BLF_CHUNK;
+ (*nvecs)++;
+ *nbytes += xfs_buf_log_format_size(blfp);
+
+ do {
+ nbits = xfs_contig_bits(blfp->blf_data_map,
+ blfp->blf_map_size, first_bit);
+ ASSERT(nbits > 0);
+
+ /*
+ * Straddling a page is rare because we don't log contiguous
+ * chunks of unmapped buffers anywhere.
+ */
+ if (nbits > 1 &&
+ xfs_buf_item_straddle(bp, offset, first_bit, nbits))
+ goto slow_scan;
+
+ (*nvecs)++;
+ *nbytes += nbits * XFS_BLF_CHUNK;
+
+ /*
+ * This takes the bit number to start looking from and
+ * returns the next set bit from there. It returns -1
+ * if there are no more bits set or the start bit is
+ * beyond the end of the bitmap.
+ */
+ first_bit = xfs_next_bit(blfp->blf_data_map, blfp->blf_map_size,
+ (uint)first_bit + nbits + 1);
+ } while (first_bit != -1);
+ return;
+
+slow_scan:
+ /* Count the first bit we jumped out of the above loop from */
+ (*nvecs)++;
+ *nbytes += XFS_BLF_CHUNK;
+ last_bit = first_bit;
while (last_bit != -1) {
/*
* This takes the bit number to start looking from and
@@ -117,11 +154,14 @@ xfs_buf_item_size_segment(
if (next_bit == -1) {
break;
} else if (next_bit != last_bit + 1 ||
- xfs_buf_item_straddle(bp, offset, next_bit, last_bit)) {
+ xfs_buf_item_straddle(bp, offset, first_bit, nbits)) {
last_bit = next_bit;
+ first_bit = next_bit;
(*nvecs)++;
+ nbits = 1;
} else {
last_bit++;
+ nbits++;
}
*nbytes += XFS_BLF_CHUNK;
}
@@ -277,6 +317,38 @@ xfs_buf_item_format_segment(
/*
* Fill in an iovec for each set of contiguous chunks.
*/
+ do {
+ ASSERT(first_bit >= 0);
+ nbits = xfs_contig_bits(blfp->blf_data_map,
+ blfp->blf_map_size, first_bit);
+ ASSERT(nbits > 0);
+
+ /*
+ * Straddling a page is rare because we don't log contiguous
+ * chunks of unmapped buffers anywhere.
+ */
+ if (nbits > 1 &&
+ xfs_buf_item_straddle(bp, offset, first_bit, nbits))
+ goto slow_scan;
+
+ xfs_buf_item_copy_iovec(lv, vecp, bp, offset,
+ first_bit, nbits);
+ blfp->blf_size++;
+
+ /*
+ * This takes the bit number to start looking from and
+ * returns the next set bit from there. It returns -1
+ * if there are no more bits set or the start bit is
+ * beyond the end of the bitmap.
+ */
+ first_bit = xfs_next_bit(blfp->blf_data_map, blfp->blf_map_size,
+ (uint)first_bit + nbits + 1);
+ } while (first_bit != -1);
+
+ return;
+
+slow_scan:
+ ASSERT(bp->b_addr == NULL);
last_bit = first_bit;
nbits = 1;
for (;;) {
@@ -301,7 +373,7 @@ xfs_buf_item_format_segment(
blfp->blf_size++;
break;
} else if (next_bit != last_bit + 1 ||
- xfs_buf_item_straddle(bp, offset, next_bit, last_bit)) {
+ xfs_buf_item_straddle(bp, offset, first_bit, nbits)) {
xfs_buf_item_copy_iovec(lv, vecp, bp, offset,
first_bit, nbits);
blfp->blf_size++;
--
2.30.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 5/8] xfs: type verification is expensive
2021-03-17 4:56 [PATCH v4 0/8] xfs: miscellaneous optimisations Dave Chinner
` (3 preceding siblings ...)
2021-03-17 4:57 ` [PATCH 4/8] xfs: optimise xfs_buf_item_size/format for contiguous regions Dave Chinner
@ 2021-03-17 4:57 ` Dave Chinner
2021-03-17 4:57 ` [PATCH 6/8] xfs: No need for inode number error injection in __xfs_dir3_data_check Dave Chinner
` (2 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2021-03-17 4:57 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
From a concurrent rm -rf workload:
41.04% [kernel] [k] xfs_dir3_leaf_check_int
9.85% [kernel] [k] __xfs_dir3_data_check
5.60% [kernel] [k] xfs_verify_ino
5.32% [kernel] [k] xfs_agino_range
4.21% [kernel] [k] memcpy
3.06% [kernel] [k] xfs_errortag_test
2.57% [kernel] [k] xfs_dir_ino_validate
1.66% [kernel] [k] xfs_dir2_data_get_ftype
1.17% [kernel] [k] do_raw_spin_lock
1.11% [kernel] [k] xfs_verify_dir_ino
0.84% [kernel] [k] __raw_callee_save___pv_queued_spin_unlock
0.83% [kernel] [k] xfs_buf_find
0.64% [kernel] [k] xfs_log_commit_cil
THere's an awful lot of overhead in just range checking inode
numbers in that, but each inode number check is not a lot of code.
The total is a bit over 14.5% of the CPU time is spent validating
inode numbers.
The problem is that they deeply nested global scope functions so the
overhead here is all in function call marshalling.
text data bss dec hex filename
2077 0 0 2077 81d fs/xfs/libxfs/xfs_types.o.orig
2197 0 0 2197 895 fs/xfs/libxfs/xfs_types.o
There's a small increase in binary size by inlining all the local
nested calls in the verifier functions, but the same workload now
profiles as:
40.69% [kernel] [k] xfs_dir3_leaf_check_int
10.52% [kernel] [k] __xfs_dir3_data_check
6.68% [kernel] [k] xfs_verify_dir_ino
4.22% [kernel] [k] xfs_errortag_test
4.15% [kernel] [k] memcpy
3.53% [kernel] [k] xfs_dir_ino_validate
1.87% [kernel] [k] xfs_dir2_data_get_ftype
1.37% [kernel] [k] do_raw_spin_lock
0.98% [kernel] [k] xfs_buf_find
0.94% [kernel] [k] __raw_callee_save___pv_queued_spin_unlock
0.73% [kernel] [k] xfs_log_commit_cil
Now we only spend just over 10% of the time validing inode numbers
for the same workload. Hence a few "inline" keyworks is good enough
to reduce the validation overhead by 30%...
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/libxfs/xfs_types.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
index b254fbeaaa50..04801362e1a7 100644
--- a/fs/xfs/libxfs/xfs_types.c
+++ b/fs/xfs/libxfs/xfs_types.c
@@ -13,7 +13,7 @@
#include "xfs_mount.h"
/* Find the size of the AG, in blocks. */
-xfs_agblock_t
+inline xfs_agblock_t
xfs_ag_block_count(
struct xfs_mount *mp,
xfs_agnumber_t agno)
@@ -29,7 +29,7 @@ xfs_ag_block_count(
* Verify that an AG block number pointer neither points outside the AG
* nor points at static metadata.
*/
-bool
+inline bool
xfs_verify_agbno(
struct xfs_mount *mp,
xfs_agnumber_t agno,
@@ -49,7 +49,7 @@ xfs_verify_agbno(
* Verify that an FS block number pointer neither points outside the
* filesystem nor points at static AG metadata.
*/
-bool
+inline bool
xfs_verify_fsbno(
struct xfs_mount *mp,
xfs_fsblock_t fsbno)
@@ -85,7 +85,7 @@ xfs_verify_fsbext(
}
/* Calculate the first and last possible inode number in an AG. */
-void
+inline void
xfs_agino_range(
struct xfs_mount *mp,
xfs_agnumber_t agno,
@@ -116,7 +116,7 @@ xfs_agino_range(
* Verify that an AG inode number pointer neither points outside the AG
* nor points at static metadata.
*/
-bool
+inline bool
xfs_verify_agino(
struct xfs_mount *mp,
xfs_agnumber_t agno,
@@ -146,7 +146,7 @@ xfs_verify_agino_or_null(
* Verify that an FS inode number pointer neither points outside the
* filesystem nor points at static AG metadata.
*/
-bool
+inline bool
xfs_verify_ino(
struct xfs_mount *mp,
xfs_ino_t ino)
@@ -162,7 +162,7 @@ xfs_verify_ino(
}
/* Is this an internal inode number? */
-bool
+inline bool
xfs_internal_inum(
struct xfs_mount *mp,
xfs_ino_t ino)
@@ -190,7 +190,7 @@ xfs_verify_dir_ino(
* Verify that an realtime block number pointer doesn't point off the
* end of the realtime device.
*/
-bool
+inline bool
xfs_verify_rtbno(
struct xfs_mount *mp,
xfs_rtblock_t rtbno)
@@ -215,7 +215,7 @@ xfs_verify_rtext(
}
/* Calculate the range of valid icount values. */
-void
+inline void
xfs_icount_range(
struct xfs_mount *mp,
unsigned long long *min,
--
2.30.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 6/8] xfs: No need for inode number error injection in __xfs_dir3_data_check
2021-03-17 4:56 [PATCH v4 0/8] xfs: miscellaneous optimisations Dave Chinner
` (4 preceding siblings ...)
2021-03-17 4:57 ` [PATCH 5/8] xfs: type verification is expensive Dave Chinner
@ 2021-03-17 4:57 ` Dave Chinner
2021-03-17 4:57 ` [PATCH 7/8] xfs: reduce debug overhead of dir leaf/node checks Dave Chinner
2021-03-17 4:57 ` [PATCH 8/8] xfs: __percpu_counter_compare() inode count debug too expensive Dave Chinner
7 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2021-03-17 4:57 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
We call xfs_dir_ino_validate() for every dir entry in a directory
when doing validity checking of the directory. It calls
xfs_verify_dir_ino() then emits a corruption report if bad or does
error injection if good. It is extremely costly:
43.27% [kernel] [k] xfs_dir3_leaf_check_int
10.28% [kernel] [k] __xfs_dir3_data_check
6.61% [kernel] [k] xfs_verify_dir_ino
4.16% [kernel] [k] xfs_errortag_test
4.00% [kernel] [k] memcpy
3.48% [kernel] [k] xfs_dir_ino_validate
7% of the cpu usage in this directory traversal workload is
xfs_dir_ino_validate() doing absolutely nothing.
We don't need error injection to simulate a bad inode numbers in the
directory structure because we can do that by fuzzing the structure
on disk.
And we don't need a corruption report, because the
__xfs_dir3_data_check() will emit one if the inode number is bad.
So just call xfs_verify_dir_ino() directly here, and get rid of all
this unnecessary overhead:
40.30% [kernel] [k] xfs_dir3_leaf_check_int
10.98% [kernel] [k] __xfs_dir3_data_check
8.10% [kernel] [k] xfs_verify_dir_ino
4.42% [kernel] [k] memcpy
2.22% [kernel] [k] xfs_dir2_data_get_ftype
1.52% [kernel] [k] do_raw_spin_lock
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_dir2_data.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
index 375b3edb2ad2..e67fa086f2c1 100644
--- a/fs/xfs/libxfs/xfs_dir2_data.c
+++ b/fs/xfs/libxfs/xfs_dir2_data.c
@@ -218,7 +218,7 @@ __xfs_dir3_data_check(
*/
if (dep->namelen == 0)
return __this_address;
- if (xfs_dir_ino_validate(mp, be64_to_cpu(dep->inumber)))
+ if (!xfs_verify_dir_ino(mp, be64_to_cpu(dep->inumber)))
return __this_address;
if (offset + xfs_dir2_data_entsize(mp, dep->namelen) > end)
return __this_address;
--
2.30.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 7/8] xfs: reduce debug overhead of dir leaf/node checks
2021-03-17 4:56 [PATCH v4 0/8] xfs: miscellaneous optimisations Dave Chinner
` (5 preceding siblings ...)
2021-03-17 4:57 ` [PATCH 6/8] xfs: No need for inode number error injection in __xfs_dir3_data_check Dave Chinner
@ 2021-03-17 4:57 ` Dave Chinner
2021-03-17 4:57 ` [PATCH 8/8] xfs: __percpu_counter_compare() inode count debug too expensive Dave Chinner
7 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2021-03-17 4:57 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
On debug kernels, we call xfs_dir3_leaf_check_int() multiple times
on every directory modification. The robust hash ordering checks it
does on every entry in the leaf on every call results in a massive
CPU overhead which slows down debug kernels by a large amount.
We use xfs_dir3_leaf_check_int() for the verifiers as well, so we
can't just gut the function to reduce overhead. What we can do,
however, is reduce the work it does when it is called from the
debug interfaces, just leaving the high level checks in place and
leaving the robust validation to the verifiers. This means the debug
checks will catch gross errors, but subtle bugs might not be caught
until a verifier is run.
It is easy enough to restore the existing debug behaviour if the
developer needs it (just change a call parameter in the debug code),
but overwise the overhead makes testing large directory block sizes
on debug kernels very slow.
Profile at an unlink rate of ~80k file/s on a 64k block size
filesystem before the patch:
40.30% [kernel] [k] xfs_dir3_leaf_check_int
10.98% [kernel] [k] __xfs_dir3_data_check
8.10% [kernel] [k] xfs_verify_dir_ino
4.42% [kernel] [k] memcpy
2.22% [kernel] [k] xfs_dir2_data_get_ftype
1.52% [kernel] [k] do_raw_spin_lock
Profile after, at an unlink rate of ~125k files/s (+50% improvement)
has largely dropped the leaf verification debug overhead out of the
profile.
16.53% [kernel] [k] __xfs_dir3_data_check
12.53% [kernel] [k] xfs_verify_dir_ino
7.97% [kernel] [k] memcpy
3.36% [kernel] [k] xfs_dir2_data_get_ftype
2.86% [kernel] [k] __pv_queued_spin_lock_slowpath
Create shows a similar change in profile and a +25% improvement in
performance.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_dir2_leaf.c | 10 +++++++---
fs/xfs/libxfs/xfs_dir2_node.c | 2 +-
fs/xfs/libxfs/xfs_dir2_priv.h | 3 ++-
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index 95d2a3f92d75..ccd8d0aa62b8 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -113,7 +113,7 @@ xfs_dir3_leaf1_check(
} else if (leafhdr.magic != XFS_DIR2_LEAF1_MAGIC)
return __this_address;
- return xfs_dir3_leaf_check_int(dp->i_mount, &leafhdr, leaf);
+ return xfs_dir3_leaf_check_int(dp->i_mount, &leafhdr, leaf, false);
}
static inline void
@@ -139,7 +139,8 @@ xfs_failaddr_t
xfs_dir3_leaf_check_int(
struct xfs_mount *mp,
struct xfs_dir3_icleaf_hdr *hdr,
- struct xfs_dir2_leaf *leaf)
+ struct xfs_dir2_leaf *leaf,
+ bool expensive_checking)
{
struct xfs_da_geometry *geo = mp->m_dir_geo;
xfs_dir2_leaf_tail_t *ltp;
@@ -162,6 +163,9 @@ xfs_dir3_leaf_check_int(
(char *)&hdr->ents[hdr->count] > (char *)xfs_dir2_leaf_bests_p(ltp))
return __this_address;
+ if (!expensive_checking)
+ return NULL;
+
/* Check hash value order, count stale entries. */
for (i = stale = 0; i < hdr->count; i++) {
if (i + 1 < hdr->count) {
@@ -195,7 +199,7 @@ xfs_dir3_leaf_verify(
return fa;
xfs_dir2_leaf_hdr_from_disk(mp, &leafhdr, bp->b_addr);
- return xfs_dir3_leaf_check_int(mp, &leafhdr, bp->b_addr);
+ return xfs_dir3_leaf_check_int(mp, &leafhdr, bp->b_addr, true);
}
static void
diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
index 5d51265d29d6..80a64117b460 100644
--- a/fs/xfs/libxfs/xfs_dir2_node.c
+++ b/fs/xfs/libxfs/xfs_dir2_node.c
@@ -73,7 +73,7 @@ xfs_dir3_leafn_check(
} else if (leafhdr.magic != XFS_DIR2_LEAFN_MAGIC)
return __this_address;
- return xfs_dir3_leaf_check_int(dp->i_mount, &leafhdr, leaf);
+ return xfs_dir3_leaf_check_int(dp->i_mount, &leafhdr, leaf, false);
}
static inline void
diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
index 44c6a77cba05..94943ce49cab 100644
--- a/fs/xfs/libxfs/xfs_dir2_priv.h
+++ b/fs/xfs/libxfs/xfs_dir2_priv.h
@@ -127,7 +127,8 @@ xfs_dir3_leaf_find_entry(struct xfs_dir3_icleaf_hdr *leafhdr,
extern int xfs_dir2_node_to_leaf(struct xfs_da_state *state);
extern xfs_failaddr_t xfs_dir3_leaf_check_int(struct xfs_mount *mp,
- struct xfs_dir3_icleaf_hdr *hdr, struct xfs_dir2_leaf *leaf);
+ struct xfs_dir3_icleaf_hdr *hdr, struct xfs_dir2_leaf *leaf,
+ bool expensive_checks);
/* xfs_dir2_node.c */
void xfs_dir2_free_hdr_from_disk(struct xfs_mount *mp,
--
2.30.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 8/8] xfs: __percpu_counter_compare() inode count debug too expensive
2021-03-17 4:56 [PATCH v4 0/8] xfs: miscellaneous optimisations Dave Chinner
` (6 preceding siblings ...)
2021-03-17 4:57 ` [PATCH 7/8] xfs: reduce debug overhead of dir leaf/node checks Dave Chinner
@ 2021-03-17 4:57 ` Dave Chinner
7 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2021-03-17 4:57 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
- 21.92% __xfs_trans_commit
- 21.62% xfs_log_commit_cil
- 11.69% xfs_trans_unreserve_and_mod_sb
- 11.58% __percpu_counter_compare
- 11.45% __percpu_counter_sum
- 10.29% _raw_spin_lock_irqsave
- 10.28% do_raw_spin_lock
__pv_queued_spin_lock_slowpath
We debated just getting rid of it last time this came up and
there was no real objection to removing it. Now it's the biggest
scalability limitation for debug kernels even on smallish machines,
so let's just get rid of it.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/xfs_trans.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index b22a09e9daee..631cca73198f 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -618,19 +618,12 @@ xfs_trans_unreserve_and_mod_sb(
ASSERT(!error);
}
- if (idelta) {
+ if (idelta)
percpu_counter_add_batch(&mp->m_icount, idelta,
XFS_ICOUNT_BATCH);
- if (idelta < 0)
- ASSERT(__percpu_counter_compare(&mp->m_icount, 0,
- XFS_ICOUNT_BATCH) >= 0);
- }
- if (ifreedelta) {
+ if (ifreedelta)
percpu_counter_add(&mp->m_ifree, ifreedelta);
- if (ifreedelta < 0)
- ASSERT(percpu_counter_compare(&mp->m_ifree, 0) >= 0);
- }
if (rtxdelta == 0 && !(tp->t_flags & XFS_TRANS_SB_DIRTY))
return;
--
2.30.1
^ permalink raw reply related [flat|nested] 11+ messages in thread