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, 3 May 2023 16:29:51 +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 v4 05/12] ext4: use pre-allocated es in
 __es_remove_extent()
Message-ID: <20230503142951.c5ugovaolwzawzun@quack3>
References: <20230424033846.4732-1-libaokun1@huawei.com>
 <20230424033846.4732-6-libaokun1@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230424033846.4732-6-libaokun1@huawei.com>
Precedence: bulk

On Mon 24-04-23 11:38:39, Baokun Li wrote:
> When splitting extent, if the second extent can not be dropped, we return
> -ENOMEM and use GFP_NOFAIL to preallocate an extent_status outside of
> i_es_lock and pass it to __es_remove_extent() to be used as the second
> extent. This ensures that __es_remove_extent() is executed successfully,
> thus ensuring consistency in the extent status tree. If the second extent
> is not undroppable, we simply drop it and return 0. Then retry is no longer
> necessary, remove it.
> 
> Now, __es_remove_extent() will always remove what it should, maybe more.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents_status.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index a6a62a744e83..7219116e0d68 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -147,7 +147,8 @@ static struct kmem_cache *ext4_pending_cachep;
>  static int __es_insert_extent(struct inode *inode, struct extent_status *newes,
>  			      struct extent_status *prealloc);
>  static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> -			      ext4_lblk_t end, int *reserved);
> +			      ext4_lblk_t end, int *reserved,
> +			      struct extent_status *prealloc);
>  static int es_reclaim_extents(struct ext4_inode_info *ei, int *nr_to_scan);
>  static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
>  		       struct ext4_inode_info *locked_ei);
> @@ -869,7 +870,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>  	ext4_es_insert_extent_check(inode, &newes);
>  
>  	write_lock(&EXT4_I(inode)->i_es_lock);
> -	err = __es_remove_extent(inode, lblk, end, NULL);
> +	err = __es_remove_extent(inode, lblk, end, NULL, NULL);
>  	if (err != 0)
>  		goto error;
>  retry:
> @@ -1315,6 +1316,7 @@ static unsigned int get_rsvd(struct inode *inode, ext4_lblk_t end,
>   * @lblk - first block in range
>   * @end - last block in range
>   * @reserved - number of cluster reservations released
> + * @prealloc - pre-allocated es to avoid memory allocation failures
>   *
>   * If @reserved is not NULL and delayed allocation is enabled, counts
>   * block/cluster reservations freed by removing range and if bigalloc
> @@ -1322,7 +1324,8 @@ static unsigned int get_rsvd(struct inode *inode, ext4_lblk_t end,
>   * error code on failure.
>   */
>  static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> -			      ext4_lblk_t end, int *reserved)
> +			      ext4_lblk_t end, int *reserved,
> +			      struct extent_status *prealloc)
>  {
>  	struct ext4_es_tree *tree = &EXT4_I(inode)->i_es_tree;
>  	struct rb_node *node;
> @@ -1330,14 +1333,12 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  	struct extent_status orig_es;
>  	ext4_lblk_t len1, len2;
>  	ext4_fsblk_t block;
> -	int err;
> +	int err = 0;
>  	bool count_reserved = true;
>  	struct rsvd_count rc;
>  
>  	if (reserved == NULL || !test_opt(inode->i_sb, DELALLOC))
>  		count_reserved = false;
> -retry:
> -	err = 0;
>  
>  	es = __es_tree_search(&tree->root, lblk);
>  	if (!es)
> @@ -1371,14 +1372,13 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  					orig_es.es_len - len2;
>  			ext4_es_store_pblock_status(&newes, block,
>  						    ext4_es_status(&orig_es));
> -			err = __es_insert_extent(inode, &newes, NULL);
> +			err = __es_insert_extent(inode, &newes, prealloc);
>  			if (err) {
> +				if (!ext4_es_must_keep(&newes))
> +					return 0;
> +
>  				es->es_lblk = orig_es.es_lblk;
>  				es->es_len = orig_es.es_len;
> -				if ((err == -ENOMEM) &&
> -				    __es_shrink(EXT4_SB(inode->i_sb),
> -							128, EXT4_I(inode)))
> -					goto retry;
>  				goto out;
>  			}
>  		} else {
> @@ -1478,7 +1478,7 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  	 * is reclaimed.
>  	 */
>  	write_lock(&EXT4_I(inode)->i_es_lock);
> -	err = __es_remove_extent(inode, lblk, end, &reserved);
> +	err = __es_remove_extent(inode, lblk, end, &reserved, NULL);
>  	write_unlock(&EXT4_I(inode)->i_es_lock);
>  	ext4_es_print_tree(inode);
>  	ext4_da_release_space(inode, reserved);
> @@ -2021,7 +2021,7 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
>  
>  	write_lock(&EXT4_I(inode)->i_es_lock);
>  
> -	err = __es_remove_extent(inode, lblk, lblk, NULL);
> +	err = __es_remove_extent(inode, lblk, lblk, NULL, NULL);
>  	if (err != 0)
>  		goto error;
>  retry:
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR