Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Apr 2023 20:53:00 +0200
From:   Jan Kara <jack@suse.cz>
To:     Baokun Li <libaokun1@huawei.com>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu,
        adilger.kernel@dilger.ca, jack@suse.cz, ritesh.list@gmail.com,
        linux-kernel@vger.kernel.org, yi.zhang@huawei.com,
        yangerkun@huawei.com, yukuai3@huawei.com
Subject: Re: [PATCH v3 2/8] ext4: add a new helper to check if es must be kept
Message-ID: <20230412185300.rpfwdlxeiptqaxes@quack3>
References: <20230412124126.2286716-1-libaokun1@huawei.com>
 <20230412124126.2286716-3-libaokun1@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230412124126.2286716-3-libaokun1@huawei.com>
Precedence: bulk

On Wed 12-04-23 20:41:20, Baokun Li wrote:
> A helper function is added to help determine if the current extent can
> be dropped, although only ext4_es_is_delayed() extents cannot be dropped
> currently.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Just some small suggestions below...

> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 7bc221038c6c..f9dab2510bdc 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -448,6 +448,20 @@ static void ext4_es_list_del(struct inode *inode)
>  	spin_unlock(&sbi->s_es_lock);
>  }
>  
> +/*
> + * Returns 1 indicates that we cannot fail to allocate memory for this
> + * extent_status and cannot reclaim, clear, or free the extent until
> + * its status changes.
> + */
> +static inline int ext4_es_must_keep(struct extent_status *es)

Maybe we can return bool? Also I'd rephrase the comment as:

/*
 * Returns true if we cannot fail to allocate memory for this extent_status
 * entry and cannot reclaim it until its status changes.
 */

> +{
> +	/* filemap, bigalloc, and seek_data/hole need to use it. */
> +	if (ext4_es_is_delayed(es))
> +		return 1;
> +
> +	return 0;
> +}
> +
>  static struct extent_status *
>  ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
>  		     ext4_fsblk_t pblk)
> @@ -460,10 +474,8 @@ ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
>  	es->es_len = len;
>  	es->es_pblk = pblk;
>  
> -	/*
> -	 * We don't count delayed extent because we never try to reclaim them
> -	 */
> -	if (!ext4_es_is_delayed(es)) {
> +	/* We never try to reclaim a must kept extent, so we don't count it. */
> +	if (!ext4_es_must_keep(es)) {
>  		if (!EXT4_I(inode)->i_es_shk_nr++)
>  			ext4_es_list_add(inode);
>  		percpu_counter_inc(&EXT4_SB(inode->i_sb)->
> @@ -481,8 +493,8 @@ static void ext4_es_free_extent(struct inode *inode, struct extent_status *es)
>  	EXT4_I(inode)->i_es_all_nr--;
>  	percpu_counter_dec(&EXT4_SB(inode->i_sb)->s_es_stats.es_stats_all_cnt);
>  
> -	/* Decrease the shrink counter when this es is not delayed */
> -	if (!ext4_es_is_delayed(es)) {
> +	/* Decrease the shrink counter when this es is not a must be kept */

Let's rephrase the comment as:
	/* Decrease the shrink counter when we can reclaim the extent */

> +	if (!ext4_es_must_keep(es)) {
>  		BUG_ON(EXT4_I(inode)->i_es_shk_nr == 0);
>  		if (!--EXT4_I(inode)->i_es_shk_nr)
>  			ext4_es_list_del(inode);
> @@ -853,7 +865,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>  	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
>  					  128, EXT4_I(inode)))
>  		goto retry;
> -	if (err == -ENOMEM && !ext4_es_is_delayed(&newes))
> +	if (err == -ENOMEM && !ext4_es_must_keep(&newes))
>  		err = 0;
>  
>  	if (sbi->s_cluster_ratio > 1 && test_opt(inode->i_sb, DELALLOC) &&
> @@ -1706,11 +1718,8 @@ static int es_do_reclaim_extents(struct ext4_inode_info *ei, ext4_lblk_t end,
>  
>  		(*nr_to_scan)--;
>  		node = rb_next(&es->rb_node);
> -		/*
> -		 * We can't reclaim delayed extent from status tree because
> -		 * fiemap, bigallic, and seek_data/hole need to use it.
> -		 */
> -		if (ext4_es_is_delayed(es))
> +		/* We can't reclaim a must be kept extent from status tree. */

I guess we can just drop this comment. The function name explains enough...

> +		if (ext4_es_must_keep(es))
>  			goto next;
>  		if (ext4_es_is_referenced(es)) {
>  			ext4_es_clear_referenced(es);

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR