Jan Kara <jack@xxxxxxx> 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
