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

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

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