Received-SPF: pass (google.com: domain of linux-ext4+bounces-2334-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 6 May 2024 22:40:23 +0200
From: Jan Kara <jack@suse.cz>
To: libaokun@huaweicloud.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,
	Baokun Li <libaokun1@huawei.com>
Subject: Re: [PATCH 2/2] ext4: propagate errors from ext4_sb_bread() in
 ext4_xattr_block_cache_find()
Message-ID: <20240506204023.l74kejvjlinufuei@quack3>
References: <20240504075526.2254349-1-libaokun@huaweicloud.com>
 <20240504075526.2254349-3-libaokun@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240504075526.2254349-3-libaokun@huaweicloud.com>

On Sat 04-05-24 15:55:26, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> In ext4_xattr_block_cache_find(), when ext4_sb_bread() returns an error,
> we will either continue to find the next ea block or return NULL to try to
> insert a new ea block. But whether ext4_sb_bread() returns -EIO or -ENOMEM,
> the next operation is most likely to fail with the same error. So propagate
> the error returned by ext4_sb_bread() to make ext4_xattr_block_set() fail
> to reduce pointless operations.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/xattr.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 9fdd13422073..11742e1f16d7 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2059,8 +2059,13 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>  
>  inserted:
>  	if (!IS_LAST_ENTRY(s->first)) {
> -		new_bh = ext4_xattr_block_cache_find(inode, header(s->base),
> -						     &ce);
> +		new_bh = ext4_xattr_block_cache_find(inode, header(s->base), &ce);
> +		if (IS_ERR(new_bh)) {
> +			error = PTR_ERR(new_bh);
> +			new_bh = NULL;
> +			goto cleanup;
> +		}
> +
>  		if (new_bh) {
>  			/* We found an identical block in the cache. */
>  			if (new_bh == bs->bh)
> @@ -3090,8 +3095,8 @@ ext4_xattr_cmp(struct ext4_xattr_header *header1,
>   *
>   * Find an identical extended attribute block.
>   *
> - * Returns a pointer to the block found, or NULL if such a block was
> - * not found or an error occurred.
> + * Returns a pointer to the block found, or NULL if such a block was not
> + * found, or an error pointer if an error occurred while reading ea block.
>   */
>  static struct buffer_head *
>  ext4_xattr_block_cache_find(struct inode *inode,
> @@ -3113,13 +3118,11 @@ ext4_xattr_block_cache_find(struct inode *inode,
>  
>  		bh = ext4_sb_bread(inode->i_sb, ce->e_value, REQ_PRIO);
>  		if (IS_ERR(bh)) {
> -			if (PTR_ERR(bh) == -ENOMEM) {
> -				mb_cache_entry_put(ea_block_cache, ce);
> -				return NULL;
> -			}
> -			bh = NULL;
> -			EXT4_ERROR_INODE(inode, "block %lu read error",
> -					 (unsigned long)ce->e_value);
> +			if (PTR_ERR(bh) != -ENOMEM)
> +				EXT4_ERROR_INODE(inode, "block %lu read error",
> +						 (unsigned long)ce->e_value);
> +			mb_cache_entry_put(ea_block_cache, ce);
> +			return bh;
>  		} else if (ext4_xattr_cmp(header, BHDR(bh)) == 0) {
>  			*pce = ce;
>  			return bh;
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR