Re: [PATCH] btrfs: do not BUG_ON() on failure to update inode when setting xattr

[Anand Jain <anand.jain@oracle.com>]

On 21/04/2022 18:03, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We are doing a BUG_ON() if we fail to update an inode after setting (or
> clearing) a xattr, but there's really no reason to not instead simply
> abort the transaction and return the error to the caller. This should be
> a rare error because we have previously reserved enough metadata space to
> update the inode and the delayed inode should have already been setup, so
> an -ENOSPC or -ENOMEM, which are the possible errors, are very unlikely to
> happen.
> 
> So replace the BUG_ON()s with a transaction abort.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>   fs/btrfs/xattr.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index f9d22ff3567f..4a2a5cb1c202 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -262,7 +262,8 @@ int btrfs_setxattr_trans(struct inode *inode, const char *name,
>   	inode_inc_iversion(inode);
>   	inode->i_ctime = current_time(inode);
>   	ret = btrfs_update_inode(trans, root, BTRFS_I(inode));
> -	BUG_ON(ret);
> +	if (ret)
> +		btrfs_abort_transaction(trans, ret);
>   out:
>   	if (start_trans)
>   		btrfs_end_transaction(trans);
> @@ -401,7 +402,8 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
>   		inode_inc_iversion(inode);
>   		inode->i_ctime = current_time(inode);
>   		ret = btrfs_update_inode(trans, root, BTRFS_I(inode));
> -		BUG_ON(ret);
> +		if (ret)
> +			btrfs_abort_transaction(trans, ret);
>   	}
>   
>   	btrfs_end_transaction(trans);


Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand