On Sat, Mar 13, 2021 at 7:31 AM Wang Yugui <wangyu...@e16-tech.com> wrote:
>
> Hi,
>
> It will be more easy to backport for heavy i/o load if we put this patch
> before '[PATCH 3/9] btrfs: move the tree mod log code into its own file' ?
Only bug fixes and fixes for serious performance regressions are
backported to stable kernels.
This patch is neither of them. It's a cleanup.
It mentions that replacing the memory barriers with test_bit() may
provide a slight benefit. If it does it's such a micro optimization
that I doubt it causes any observable effect on any workload.
Even if it does, which I seriously doubt, it's still not a candidate
for stable backports.
>
> And this patch can be merged into one with the following
> '[PATCH 6/9] btrfs: use the new bit BTRFS_FS_TREE_MOD_LOG_USERS at
> btrfs_free_tree_block()'?
They do the same thing, but for slightly different cases. So I prefer
to keep them separate for ease of review.
>
> Best Regards
> Wang Yugui (wangyu...@e16-tech.com)
> 2021/03/13
>
> > From: Filipe Manana <fdman...@suse.com>
> >
> > The tree modification log functions are called very frequently, basically
> > they are called everytime a btree is modified (a pointer added or removed
> > to a node, a new root for a btree is set, etc). Because of that, to avoid
> > heavy lock contention on the lock that protects the list of tree mod log
> > users, we have checks that test the emptiness of the list with a full
> > memory barrier before the checks, so that when there are no tree mod log
> > users we avoid taking the lock.
> >
> > Replace the memory barrier and list emptiness check with a test for a new
> > bit set at fs_info->flags. This bit is used to indicate when there are
> > tree mod log users, set whenever a user is added to the list and cleared
> > when the last user is removed from the list. This makes the intention a
> > bit more obvious and possibly more efficient (assuming test_bit() may be
> > cheaper than a full memory barrier on some architectures).
> >
> > Signed-off-by: Filipe Manana <fdman...@suse.com>
> > ---
> > fs/btrfs/ctree.h | 3 +++
> > fs/btrfs/tree-mod-log.c | 11 ++++++-----
> > 2 files changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 50208673bd55..90184ee2f832 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -580,6 +580,9 @@ enum {
> >
> > /* Indicate that we can't trust the free space tree for caching yet */
> > BTRFS_FS_FREE_SPACE_TREE_UNTRUSTED,
> > +
> > + /* Indicate whether there are any tree modification log users. */
> > + BTRFS_FS_TREE_MOD_LOG_USERS,
> > };
> >
> > /*
> > diff --git a/fs/btrfs/tree-mod-log.c b/fs/btrfs/tree-mod-log.c
> > index f2a6da1b2903..b912b82c36c9 100644
> > --- a/fs/btrfs/tree-mod-log.c
> > +++ b/fs/btrfs/tree-mod-log.c
> > @@ -60,6 +60,7 @@ u64 btrfs_get_tree_mod_seq(struct btrfs_fs_info *fs_info,
> > if (!elem->seq) {
> > elem->seq = btrfs_inc_tree_mod_seq(fs_info);
> > list_add_tail(&elem->list, &fs_info->tree_mod_seq_list);
> > + set_bit(BTRFS_FS_TREE_MOD_LOG_USERS, &fs_info->flags);
> > }
> > write_unlock(&fs_info->tree_mod_log_lock);
> >
> > @@ -83,7 +84,9 @@ void btrfs_put_tree_mod_seq(struct btrfs_fs_info *fs_info,
> > list_del(&elem->list);
> > elem->seq = 0;
> >
> > - if (!list_empty(&fs_info->tree_mod_seq_list)) {
> > + if (list_empty(&fs_info->tree_mod_seq_list)) {
> > + clear_bit(BTRFS_FS_TREE_MOD_LOG_USERS, &fs_info->flags);
> > + } else {
> > struct btrfs_seq_list *first;
> >
> > first = list_first_entry(&fs_info->tree_mod_seq_list,
> > @@ -166,8 +169,7 @@ static noinline int tree_mod_log_insert(struct
> > btrfs_fs_info *fs_info,
> > static inline bool tree_mod_dont_log(struct btrfs_fs_info *fs_info,
> > struct extent_buffer *eb)
> > {
> > - smp_mb();
> > - if (list_empty(&(fs_info)->tree_mod_seq_list))
> > + if (!test_bit(BTRFS_FS_TREE_MOD_LOG_USERS, &fs_info->flags))
> > return true;
> > if (eb && btrfs_header_level(eb) == 0)
> > return true;
> > @@ -185,8 +187,7 @@ static inline bool tree_mod_dont_log(struct
> > btrfs_fs_info *fs_info,
> > static inline bool tree_mod_need_log(const struct btrfs_fs_info *fs_info,
> > struct extent_buffer *eb)
> > {
> > - smp_mb();
> > - if (list_empty(&(fs_info)->tree_mod_seq_list))
> > + if (!test_bit(BTRFS_FS_TREE_MOD_LOG_USERS, &fs_info->flags))
> > return false;
> > if (eb && btrfs_header_level(eb) == 0)
> > return false;
> > --
> > 2.28.0
>
>
