[PATCH] ext4/mballoc: No need to generate from free list

[PATCH] ext4/mballoc: No need to generate from free list

[Wang Jianjian]

Commit 7a2fcbf7f85('ext4: don't use blocks freed but
not yet committed in buddy cache init) walk the rbtree of
freed data and mark them free in buddy to avoid reuse them
before journal committing them, However, it is unnecessary to
do that, because we have extra page references to buddy and bitmap
pages, they will be released iff journal has committed and after
process freed data.

Fixes: 7a2fcbf7f85('ext4: don't use blocks freed but not yet committed in buddy cache init')
Signed-off-by: Wang Jianjian <wangjianjian0@foxmail.com>
---
 fs/ext4/mballoc.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 5b2ae37a8b80..b512a134a5fb 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -404,8 +404,6 @@ static const char * const ext4_groupinfo_slab_names[NR_GRPINFO_CACHES] = {
 
 static void ext4_mb_generate_from_pa(struct super_block *sb, void *bitmap,
 					ext4_group_t group);
-static void ext4_mb_generate_from_freelist(struct super_block *sb, void *bitmap,
-						ext4_group_t group);
 static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac);
 
 static bool ext4_mb_good_group(struct ext4_allocation_context *ac,
@@ -1274,7 +1272,6 @@ static int ext4_mb_init_cache(struct page *page, char *incore, gfp_t gfp)
 
 			/* mark all preallocated blks used in in-core bitmap */
 			ext4_mb_generate_from_pa(sb, data, group);
-			ext4_mb_generate_from_freelist(sb, data, group);
 			ext4_unlock_group(sb, group);
 
 			/* set incore so that the buddy information can be
@@ -4440,30 +4437,6 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)
 	return false;
 }
 
-/*
- * the function goes through all block freed in the group
- * but not yet committed and marks them used in in-core bitmap.
- * buddy must be generated from this bitmap
- * Need to be called with the ext4 group lock held
- */
-static void ext4_mb_generate_from_freelist(struct super_block *sb, void *bitmap,
-						ext4_group_t group)
-{
-	struct rb_node *n;
-	struct ext4_group_info *grp;
-	struct ext4_free_data *entry;
-
-	grp = ext4_get_group_info(sb, group);
-	n = rb_first(&(grp->bb_free_root));
-
-	while (n) {
-		entry = rb_entry(n, struct ext4_free_data, efd_node);
-		mb_set_bits(bitmap, entry->efd_start_cluster, entry->efd_count);
-		n = rb_next(n);
-	}
-	return;
-}
-
 /*
  * the function goes through all preallocation in this group and marks them
  * used in in-core bitmap. buddy must be generated from this bitmap
-- 
2.34.3

Re: [PATCH] ext4/mballoc: No need to generate from free list

[Jan Kara]

On Thu 24-08-23 23:56:31, Wang Jianjian wrote:
> Commit 7a2fcbf7f85('ext4: don't use blocks freed but
> not yet committed in buddy cache init) walk the rbtree of
> freed data and mark them free in buddy to avoid reuse them
> before journal committing them, However, it is unnecessary to
> do that, because we have extra page references to buddy and bitmap
> pages, they will be released iff journal has committed and after
> process freed data.

So do you mean that buddy bitmap cannot be freed until the transaction that
frees the blocks in it commits? Indeed ext4_mb_free_metadata() grabs buddy
page references and ext4_free_data_in_buddy() drops them. 

Perhaps I'd rephrase the changelog as:

Commit 7a2fcbf7f85 ("ext4: don't use blocks freed but not yet committed in
buddy cache init") added a code to mark as used blocks in the list of not yet
committed freed blocks during initialization of a buddy page. However
ext4_mb_free_metadata() makes sure buddy page is already loaded and takes a
reference to it so it cannot happen that ext4_mb_init_cache() is called
when efd list is non-empty. Just remove the
ext4_mb_generate_from_freelist() call.

> @@ -1274,7 +1272,6 @@ static int ext4_mb_init_cache(struct page *page, char *incore, gfp_t gfp)
>  
>  			/* mark all preallocated blks used in in-core bitmap */
>  			ext4_mb_generate_from_pa(sb, data, group);
> -			ext4_mb_generate_from_freelist(sb, data, group);

And just to be sure I'd add here:

			WARN_ON_ONCE(!RB_EMPTY_ROOT(&grp->bb_free_root));

								Honza


>  			ext4_unlock_group(sb, group);
>  
>  			/* set incore so that the buddy information can be
> @@ -4440,30 +4437,6 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)
>  	return false;
>  }
>  
> -/*
> - * the function goes through all block freed in the group
> - * but not yet committed and marks them used in in-core bitmap.
> - * buddy must be generated from this bitmap
> - * Need to be called with the ext4 group lock held
> - */
> -static void ext4_mb_generate_from_freelist(struct super_block *sb, void *bitmap,
> -						ext4_group_t group)
> -{
> -	struct rb_node *n;
> -	struct ext4_group_info *grp;
> -	struct ext4_free_data *entry;
> -
> -	grp = ext4_get_group_info(sb, group);
> -	n = rb_first(&(grp->bb_free_root));
> -
> -	while (n) {
> -		entry = rb_entry(n, struct ext4_free_data, efd_node);
> -		mb_set_bits(bitmap, entry->efd_start_cluster, entry->efd_count);
> -		n = rb_next(n);
> -	}
> -	return;
> -}
> -
>  /*
>   * the function goes through all preallocation in this group and marks them
>   * used in in-core bitmap. buddy must be generated from this bitmap
> -- 
> 2.34.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4/mballoc: No need to generate from free list

[Ritesh Harjani]

Jan Kara <jack@suse.cz> writes:

> On Thu 24-08-23 23:56:31, Wang Jianjian wrote:
>> Commit 7a2fcbf7f85('ext4: don't use blocks freed but
>> not yet committed in buddy cache init) walk the rbtree of
>> freed data and mark them free in buddy to avoid reuse them
>> before journal committing them, However, it is unnecessary to
>> do that, because we have extra page references to buddy and bitmap
>> pages, they will be released iff journal has committed and after
>> process freed data.
>
> So do you mean that buddy bitmap cannot be freed until the transaction that
> frees the blocks in it commits? Indeed ext4_mb_free_metadata() grabs buddy
> page references and ext4_free_data_in_buddy() drops them. 
>
> Perhaps I'd rephrase the changelog as:
>
> Commit 7a2fcbf7f85 ("ext4: don't use blocks freed but not yet committed in
> buddy cache init") added a code to mark as used blocks in the list of not yet
> committed freed blocks during initialization of a buddy page. However
> ext4_mb_free_metadata() makes sure buddy page is already loaded and takes a
> reference to it so it cannot happen that ext4_mb_init_cache() is called
> when efd list is non-empty. Just remove the
> ext4_mb_generate_from_freelist() call.
>

The above changelog is very clear. Thanks!

The observation made in this patch is very subtle. Indeed we cannot have
ext4_mb_init_cache() get called for a group as long we have an entry in
grp->bb_free_root because it holds the reference to the buddy and
incore-bitmap pages. These entry gets freed up after a transaction
commit is completed via callback ext4_process_freed_data() -> ext4_free_data_in_buddy()

Nice catch. It's from 2009 :)

>> @@ -1274,7 +1272,6 @@ static int ext4_mb_init_cache(struct page *page, char *incore, gfp_t gfp)
>>  
>>  			/* mark all preallocated blks used in in-core bitmap */
>>  			ext4_mb_generate_from_pa(sb, data, group);
>> -			ext4_mb_generate_from_freelist(sb, data, group);
>
> And just to be sure I'd add here:
>
> 			WARN_ON_ONCE(!RB_EMPTY_ROOT(&grp->bb_free_root));
>

Right. That might catch any wrong use.


-ritesh

Re: [PATCH] ext4/mballoc: No need to generate from free list

[Theodore Ts'o]

On Thu, 24 Aug 2023 23:56:31 +0800, Wang Jianjian wrote:
> Commit 7a2fcbf7f85('ext4: don't use blocks freed but
> not yet committed in buddy cache init) walk the rbtree of
> freed data and mark them free in buddy to avoid reuse them
> before journal committing them, However, it is unnecessary to
> do that, because we have extra page references to buddy and bitmap
> pages, they will be released iff journal has committed and after
> process freed data.
> 
> [...]

Applied, thanks!

[1/1] ext4/mballoc: No need to generate from free list
      commit: ebf6cb7c6e1241984f75f29f1bdbfa2fe7168f88

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>