Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Mon, 18 Sep 2023 17:36:19 +0200
From:   Jan Kara <jack@suse.cz>
To:     Wang Jianjian <wangjianjian0@foxmail.com>
Cc:     linux-ext4@vger.kernel.org, aneesh.kumar@linux.vnet.ibm.com
Subject: Re: [PATCH] ext4/mballoc: No need to generate from free list
Message-ID: <20230918153619.tku6d4556os5tvzj@quack3>
References: <tencent_53CBCB1668358AE862684E453DF37B722008@qq.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <tencent_53CBCB1668358AE862684E453DF37B722008@qq.com>
Precedence: bulk

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