Re: [PATCH] btrfs: check error value from btrfs_update_inode in tree log

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/5/19 下午11:26, Josef Bacik wrote:
> Error injection testing uncovered a case where we ended up with invalid
> link counts on an inode.  This happened because we failed to notice an
> error when updating the inode while replaying the tree log, and
> committed the transaction with an invalid file system.  Fix this by
> checking the return value of btrfs_update_inode.  This resolved the link
> count errors I was seeing, and we already properly handle passing up the
> error values in these paths.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

But one thing unrelated to notice, inlined below.

> ---
>   fs/btrfs/tree-log.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 326be57f2828..4dc74949040d 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -1574,7 +1574,9 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
>   			if (ret)
>   				goto out;
>
> -			btrfs_update_inode(trans, root, BTRFS_I(inode));

I did a quick grep and found that we have other locations where we call
btrfs_uppdate_inode() without catching the return value:

$ grep -IRe "^\s\+btrfs_update_inode(" fs/btrfs/
fs/btrfs/free-space-cache.c:    btrfs_update_inode(trans, root,
BTRFS_I(inode));
fs/btrfs/free-space-cache.c:    btrfs_update_inode(trans, root,
BTRFS_I(inode));
fs/btrfs/inode.c:               btrfs_update_inode(trans, root, inode);
fs/btrfs/inode.c:       btrfs_update_inode(trans, root, BTRFS_I(inode));

Maybe it's better to make btrfs_update_inode() to have __must_check prefix?

Thanks,
Qu


> +			ret = btrfs_update_inode(trans, root, BTRFS_I(inode));
> +			if (ret)
> +				goto out;
>   		}
>
>   		ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen;
> @@ -1749,7 +1751,9 @@ static noinline int fixup_inode_link_count(struct btrfs_trans_handle *trans,
>
>   	if (nlink != inode->i_nlink) {
>   		set_nlink(inode, nlink);
> -		btrfs_update_inode(trans, root, BTRFS_I(inode));
> +		ret = btrfs_update_inode(trans, root, BTRFS_I(inode));
> +		if (ret)
> +			goto out;
>   	}
>   	BTRFS_I(inode)->index_cnt = (u64)-1;
>
>