Re: [PATCH 03/13] btrfs-progs: lowmem: fix false alert if extent item has been repaired

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

On 2018/10/23 下午5:41, Su Yue wrote:
> Previously, @err are assigned immediately after check but before
> repair.
> repair_extent_item()'s return value also confuses the caller. If
> error has been repaired and returns 0, check_extent_item() will try
> to continue check the nonexistent and cause flase alerts.
> 
> Here make repair_extent_item()'s return codes only represents status
> of the extent item, error bits are passed by pointer.
> Move the change of @err after repair.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>  check/mode-lowmem.c | 106 ++++++++++++++++++++++++++++----------------
>  1 file changed, 68 insertions(+), 38 deletions(-)
> 
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 76e7be81ceb1..769b3141de8b 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -3788,35 +3788,35 @@ out:
>  /*
>   * Only delete backref if REFERENCER_MISSING now
>   *
> - * Returns <0   the extent was deleted
> - * Returns >0   the backref was deleted but extent still exists, returned value
> - *               means error after repair
> + * Returns <0   the whole extent item was deleted
> + * Returns >0   the backref was deleted but extent still exists, @err_ret
> + *		will be set to be error bits after repair.

Well, I have to admit the parameter list is really human friendly from
the initial design.

In fact, the only real user of @err is just that "if ((err &
(REFERENCER_MISSING | REFERENCER_MISMATCH)) == 0)" line.

We can in fact get rid of that @err parameter and do that check at the
only caller of repair_extent_item().


And then redefine the return value.

Like "Return < 0 if we failed to repair backref for the extent.
Return 0 if we repaired the backref for the extent".

[snip]
>  
> -	if (err && repair) {
> +	if (tmp_err && repair) {
>  		ret = repair_extent_item(fs_info->extent_root, path,
>  			 key.objectid, num_bytes, parent, root_objectid,
> -			 owner, owner_offset, ret);
> -		if (ret < 0)

If follow my above method, here the code can be a little easier:
	if (tmp_err && repair) {
		if (!(tmp_err & (REFERENCER_MISSING |
		    REFENCER_MISMATCH)) {
			err |= tmp_err;
			goto next;
		}
		ret = repair_extent_item(...);
		if (ret < 0)
			err |= tmp_err;
	} else if (tmp_err) {
		err |= tmp_err;
	}

Without the need to passing @err into repair_extent_item() nor modifying
@err in that function.

Thanks,
Qu

> +			 owner, owner_offset, &tmp_err);
> +		err |= tmp_err;
> +
> +		if (tmp_err & FATAL_ERROR || ret < 0)
>  			goto out;
> -		if (ret) {
> +		/*
> +		 * the error has been repaired which means the extent item
> +		 * is still existed with other backrefs, go to check next.
> +		 */> +		if (ret > 0) {
> +			eb = path->nodes[0];
> +			slot = path->slots[0];
> +			ei = btrfs_item_ptr(eb, slot, struct btrfs_extent_item);
> +			item_size = btrfs_item_size_nr(eb, slot);
>  			goto next;
> -			err = ret;
>  		}
> +	} else {
> +		err |= tmp_err;
>  	}
>  
> -	ptr += btrfs_extent_inline_ref_size(type);
> +	ptr_offset += btrfs_extent_inline_ref_size(type);
>  	goto next;
>  
>  out:
>