Re: [PATCH 5/9] btrfs: use a bit to track the existence of tree mod log users

On Sat, Mar 13, 2021 at 7:31 AM Wang Yugui [off-list ref] 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 (wangyugui@e16-tech.com)
2021/03/13

quoted

From: Filipe Manana <redacted>

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 <redacted>
---
 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