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

Commit Message

Filipe Manana March 11, 2021, 2:31 p.m. UTC

From: Filipe Manana <[e-mail]>

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 <[e-mail]>
---
 fs/btrfs/ctree.h        |  3 +++
 fs/btrfs/tree-mod-log.c | 11 ++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

Comments

Wang Yugui March 13, 2021, 7:26 a.m. UTC | #1

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' ?

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()'?

Best Regards
Wang Yugui ([e-mail])
2021/03/13

> From: Filipe Manana <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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

Filipe Manana March 15, 2021, 9:52 a.m. UTC | #2

On Sat, Mar 13, 2021 at 7:31 AM Wang Yugui <[e-mail]> 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 ([e-mail])
> 2021/03/13
>
> > From: Filipe Manana <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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
>
>

Patch

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;