Re: [PATCH 1/3] btrfs: remove the unnecessary path parameter for scrub_raid56_parity()

[Qu Wenruo <wqu@suse.com>]

On 2021/12/13 21:10, Qu Wenruo wrote:
> In function scrub_stripe() we allocated two btrfs_path, one @path for
> extent tree search and another @ppath for full stripe extent tree search
> for RAID56.
> 
> This is totally uncessary, as the @ppath usage is completely inside
> scrub_raid56_parity(), thus we can move the path allocation into
> scrub_raid56_parity() completely.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/scrub.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 15a123e67108..a3bd3dfd5e8b 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -2892,7 +2892,6 @@ static void scrub_parity_put(struct scrub_parity *sparity)
>   static noinline_for_stack int scrub_raid56_parity(struct scrub_ctx *sctx,
>   						  struct map_lookup *map,
>   						  struct btrfs_device *sdev,
> -						  struct btrfs_path *path,
>   						  u64 logic_start,
>   						  u64 logic_end)
>   {
> @@ -2901,6 +2900,7 @@ static noinline_for_stack int scrub_raid56_parity(struct scrub_ctx *sctx,
>   	struct btrfs_root *csum_root;
>   	struct btrfs_extent_item *extent;
>   	struct btrfs_io_context *bioc = NULL;
> +	struct btrfs_path *path;
>   	u64 flags;
>   	int ret;
>   	int slot;
> @@ -2919,6 +2919,14 @@ static noinline_for_stack int scrub_raid56_parity(struct scrub_ctx *sctx,
>   	int extent_mirror_num;
>   	int stop_loop = 0;
>   
> +	path = btrfs_alloc_path();
> +	if (!path) {
> +		spin_lock(&sctx->stat_lock);
> +		sctx->stat.malloc_errors++;
> +		spin_unlock(&sctx->stat_lock);
> +		return -ENOMEM;
> +	}
> +
>   	ASSERT(map->stripe_len <= U32_MAX);
>   	nsectors = map->stripe_len >> fs_info->sectorsize_bits;
>   	bitmap_len = scrub_calc_parity_bitmap_len(nsectors);
> @@ -2928,6 +2936,7 @@ static noinline_for_stack int scrub_raid56_parity(struct scrub_ctx *sctx,
>   		spin_lock(&sctx->stat_lock);
>   		sctx->stat.malloc_errors++;
>   		spin_unlock(&sctx->stat_lock);
> +		btrfs_free_path(path);
>   		return -ENOMEM;
>   	}
>   
> @@ -3117,7 +3126,7 @@ static noinline_for_stack int scrub_raid56_parity(struct scrub_ctx *sctx,
>   	scrub_wr_submit(sctx);
>   	mutex_unlock(&sctx->wr_lock);
>   
> -	btrfs_release_path(path);
> +	btrfs_free_path(path);
>   	return ret < 0 ? ret : 0;
>   }
>   
> @@ -3167,7 +3176,7 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>   					   int num, u64 base, u64 length,
>   					   struct btrfs_block_group *cache)
>   {
> -	struct btrfs_path *path, *ppath;
> +	struct btrfs_path *path;
>   	struct btrfs_fs_info *fs_info = sctx->fs_info;
>   	struct btrfs_root *root;
>   	struct btrfs_root *csum_root;
> @@ -3229,12 +3238,6 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>   	if (!path)
>   		return -ENOMEM;
>   
> -	ppath = btrfs_alloc_path();
> -	if (!ppath) {
> -		btrfs_free_path(path);
> -		return -ENOMEM;
> -	}
> -
>   	/*
>   	 * work on commit root. The related disk blocks are static as
>   	 * long as COW is applied. This means, it is save to rewrite
> @@ -3243,8 +3246,6 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>   	path->search_commit_root = 1;
>   	path->skip_locking = 1;
>   
> -	ppath->search_commit_root = 1;
> -	ppath->skip_locking = 1;

The new path in the raid56 functions doesn't have this.

This is causing false alerts in raid56 dev-replace test cases, like 
btrfs/072.
Which reports csum mismatch for metadata.

Would fix it in next update.

Thanks,
Qu

>   	/*
>   	 * trigger the readahead for extent tree csum tree and wait for
>   	 * completion. During readahead, the scrub is officially paused
> @@ -3347,7 +3348,7 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>   				stripe_logical += base;
>   				stripe_end = stripe_logical + increment;
>   				ret = scrub_raid56_parity(sctx, map, scrub_dev,
> -							  ppath, stripe_logical,
> +							  stripe_logical,
>   							  stripe_end);
>   				if (ret)
>   					goto out;
> @@ -3518,7 +3519,7 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>   						stripe_end = stripe_logical +
>   								increment;
>   						ret = scrub_raid56_parity(sctx,
> -							map, scrub_dev, ppath,
> +							map, scrub_dev,
>   							stripe_logical,
>   							stripe_end);
>   						if (ret)
> @@ -3565,7 +3566,6 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>   
>   	blk_finish_plug(&plug);
>   	btrfs_free_path(path);
> -	btrfs_free_path(ppath);
>   
>   	if (sctx->is_dev_replace && ret >= 0) {
>   		int ret2;