[PATCH 1/2] btrfs: cleanup the argument list of scrub_chunk()

[PATCH 1/2] btrfs: cleanup the argument list of scrub_chunk()

[Qu Wenruo]

The argument list of scrub_chunk() has the following problems:

- Duplicated @chunk_offset
  It is the same as btrfs_block_group::start.

- Confusing @length
  The most instinctive guess is chunk length, and one may want to delete
  it, but the truth is, it's device extent length.

Fix this by:

- Remove @chunk_offset
  Use btrfs_block_group::start instead.

- Rename @length to @dev_ext_len
  Also rename the caller to remove the ambiguous naming.

- Rename @cache to @bg
  The "_cache" suffix for btrfs_block_group is removed for a while.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/scrub.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 5e59e27e6bcd..ffbaf6fbf71c 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3548,10 +3548,10 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 }
 
 static noinline_for_stack int scrub_chunk(struct scrub_ctx *sctx,
+					  struct btrfs_block_group *bg,
 					  struct btrfs_device *scrub_dev,
-					  u64 chunk_offset, u64 length,
 					  u64 dev_offset,
-					  struct btrfs_block_group *cache)
+					  u64 dev_ext_len)
 {
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
 	struct extent_map_tree *map_tree = &fs_info->mapping_tree;
@@ -3561,7 +3561,7 @@ static noinline_for_stack int scrub_chunk(struct scrub_ctx *sctx,
 	int ret = 0;
 
 	read_lock(&map_tree->lock);
-	em = lookup_extent_mapping(map_tree, chunk_offset, 1);
+	em = lookup_extent_mapping(map_tree, bg->start, bg->length);
 	read_unlock(&map_tree->lock);
 
 	if (!em) {
@@ -3569,26 +3569,24 @@ static noinline_for_stack int scrub_chunk(struct scrub_ctx *sctx,
 		 * Might have been an unused block group deleted by the cleaner
 		 * kthread or relocation.
 		 */
-		spin_lock(&cache->lock);
-		if (!cache->removed)
+		spin_lock(&bg->lock);
+		if (!bg->removed)
 			ret = -EINVAL;
-		spin_unlock(&cache->lock);
+		spin_unlock(&bg->lock);
 
 		return ret;
 	}
-
-	map = em->map_lookup;
-	if (em->start != chunk_offset)
+	if (em->start != bg->start)
 		goto out;
-
-	if (em->len < length)
+	if (em->len < dev_ext_len)
 		goto out;
 
+	map = em->map_lookup;
 	for (i = 0; i < map->num_stripes; ++i) {
 		if (map->stripes[i].dev->bdev == scrub_dev->bdev &&
 		    map->stripes[i].physical == dev_offset) {
 			ret = scrub_stripe(sctx, map, scrub_dev, i,
-					   chunk_offset, length, cache);
+					   bg->start, dev_ext_len, bg);
 			if (ret)
 				goto out;
 		}
@@ -3626,7 +3624,6 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 	struct btrfs_path *path;
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
 	struct btrfs_root *root = fs_info->dev_root;
-	u64 length;
 	u64 chunk_offset;
 	int ret = 0;
 	int ro_set;
@@ -3650,6 +3647,8 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 	key.type = BTRFS_DEV_EXTENT_KEY;
 
 	while (1) {
+		u64 dev_ext_len;
+
 		ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
 		if (ret < 0)
 			break;
@@ -3686,9 +3685,9 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 			break;
 
 		dev_extent = btrfs_item_ptr(l, slot, struct btrfs_dev_extent);
-		length = btrfs_dev_extent_length(l, dev_extent);
+		dev_ext_len = btrfs_dev_extent_length(l, dev_extent);
 
-		if (found_key.offset + length <= start)
+		if (found_key.offset + dev_ext_len <= start)
 			goto skip;
 
 		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
@@ -3822,13 +3821,14 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 
 		scrub_pause_off(fs_info);
 		down_write(&dev_replace->rwsem);
-		dev_replace->cursor_right = found_key.offset + length;
+		dev_replace->cursor_right = found_key.offset + dev_ext_len;
 		dev_replace->cursor_left = found_key.offset;
 		dev_replace->item_needs_writeback = 1;
 		up_write(&dev_replace->rwsem);
 
-		ret = scrub_chunk(sctx, scrub_dev, chunk_offset, length,
-				  found_key.offset, cache);
+		ASSERT(cache->start == chunk_offset);
+		ret = scrub_chunk(sctx, cache, scrub_dev, found_key.offset,
+				  dev_ext_len);
 
 		/*
 		 * flush, submit all pending read and write bios, afterwards
@@ -3909,7 +3909,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 			break;
 		}
 skip:
-		key.offset = found_key.offset + length;
+		key.offset = found_key.offset + dev_ext_len;
 		btrfs_release_path(path);
 	}
 
-- 
2.34.1

[PATCH 2/2] btrfs: cleanup the argument list of scrub_stripe()

[Qu Wenruo]

The argument list of btrfs_stripe() has similar problems of
scrub_chunk():

- Duplicated and ambiguous @base argument
  Can be fetched from btrfs_block_group::bg.

- Ambiguous argument @length
  It's again device extent length

- Ambiguous argument @num
  The instinctive guess would be mirror number, but in fact it's stripe
  index.

Fix it by:

- Remove @base parameter

- Rename @length to @dev_ext_len

- Rename @num to @stripe_index

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/scrub.c | 67 +++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index ffbaf6fbf71c..f20ad60dcc0e 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3173,10 +3173,10 @@ static int sync_write_pointer_for_zoned(struct scrub_ctx *sctx, u64 logical,
 }
 
 static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
+					   struct btrfs_block_group *bg,
 					   struct map_lookup *map,
 					   struct btrfs_device *scrub_dev,
-					   int num, u64 base, u64 length,
-					   struct btrfs_block_group *cache)
+					   int stripe_index, u64 dev_ext_len)
 {
 	struct btrfs_path *path;
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
@@ -3184,6 +3184,7 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 	struct btrfs_root *csum_root;
 	struct btrfs_extent_item *extent;
 	struct blk_plug plug;
+	const u64 chunk_logical = bg->start;
 	u64 flags;
 	int ret;
 	int slot;
@@ -3211,25 +3212,26 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 	int extent_mirror_num;
 	int stop_loop = 0;
 
-	physical = map->stripes[num].physical;
+	physical = map->stripes[stripe_index].physical;
 	offset = 0;
-	nstripes = div64_u64(length, map->stripe_len);
+	nstripes = div64_u64(dev_ext_len, map->stripe_len);
 	mirror_num = 1;
 	increment = map->stripe_len;
 	if (map->type & BTRFS_BLOCK_GROUP_RAID0) {
-		offset = map->stripe_len * num;
+		offset = map->stripe_len * stripe_index;
 		increment = map->stripe_len * map->num_stripes;
 	} else if (map->type & BTRFS_BLOCK_GROUP_RAID10) {
 		int factor = map->num_stripes / map->sub_stripes;
-		offset = map->stripe_len * (num / map->sub_stripes);
+		offset = map->stripe_len * (stripe_index / map->sub_stripes);
 		increment = map->stripe_len * factor;
-		mirror_num = num % map->sub_stripes + 1;
+		mirror_num = stripe_index % map->sub_stripes + 1;
 	} else if (map->type & BTRFS_BLOCK_GROUP_RAID1_MASK) {
-		mirror_num = num % map->num_stripes + 1;
+		mirror_num = stripe_index % map->num_stripes + 1;
 	} else if (map->type & BTRFS_BLOCK_GROUP_DUP) {
-		mirror_num = num % map->num_stripes + 1;
+		mirror_num = stripe_index % map->num_stripes + 1;
 	} else if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
-		get_raid56_logic_offset(physical, num, map, &offset, NULL);
+		get_raid56_logic_offset(physical, stripe_index, map, &offset,
+					NULL);
 		increment = map->stripe_len * nr_data_stripes(map);
 	}
 
@@ -3246,12 +3248,12 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 	path->skip_locking = 1;
 	path->reada = READA_FORWARD;
 
-	logical = base + offset;
+	logical = chunk_logical + offset;
 	physical_end = physical + nstripes * map->stripe_len;
 	if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
-		get_raid56_logic_offset(physical_end, num,
+		get_raid56_logic_offset(physical_end, stripe_index,
 					map, &logic_end, NULL);
-		logic_end += base;
+		logic_end += chunk_logical;
 	} else {
 		logic_end = logical + increment * nstripes;
 	}
@@ -3306,13 +3308,13 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 		}
 
 		if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
-			ret = get_raid56_logic_offset(physical, num, map,
-						      &logical,
+			ret = get_raid56_logic_offset(physical, stripe_index,
+						      map, &logical,
 						      &stripe_logical);
-			logical += base;
+			logical += chunk_logical;
 			if (ret) {
 				/* it is parity strip */
-				stripe_logical += base;
+				stripe_logical += chunk_logical;
 				stripe_end = stripe_logical + increment;
 				ret = scrub_raid56_parity(sctx, map, scrub_dev,
 							  stripe_logical,
@@ -3392,13 +3394,13 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 			 * Continuing would prevent reusing its device extents
 			 * for new block groups for a long time.
 			 */
-			spin_lock(&cache->lock);
-			if (cache->removed) {
-				spin_unlock(&cache->lock);
+			spin_lock(&bg->lock);
+			if (bg->removed) {
+				spin_unlock(&bg->lock);
 				ret = 0;
 				goto out;
 			}
-			spin_unlock(&cache->lock);
+			spin_unlock(&bg->lock);
 
 			extent = btrfs_item_ptr(l, slot,
 						struct btrfs_extent_item);
@@ -3477,12 +3479,12 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 loop:
 					physical += map->stripe_len;
 					ret = get_raid56_logic_offset(physical,
-							num, map, &logical,
-							&stripe_logical);
-					logical += base;
+							stripe_index, map,
+							&logical, &stripe_logical);
+					logical += chunk_logical;
 
 					if (ret && physical < physical_end) {
-						stripe_logical += base;
+						stripe_logical += chunk_logical;
 						stripe_end = stripe_logical +
 								increment;
 						ret = scrub_raid56_parity(sctx,
@@ -3516,8 +3518,8 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 		physical += map->stripe_len;
 		spin_lock(&sctx->stat_lock);
 		if (stop_loop)
-			sctx->stat.last_physical = map->stripes[num].physical +
-						   length;
+			sctx->stat.last_physical = map->stripes[stripe_index].physical +
+						   dev_ext_len;
 		else
 			sctx->stat.last_physical = physical;
 		spin_unlock(&sctx->stat_lock);
@@ -3537,9 +3539,10 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 	if (sctx->is_dev_replace && ret >= 0) {
 		int ret2;
 
-		ret2 = sync_write_pointer_for_zoned(sctx, base + offset,
-						    map->stripes[num].physical,
-						    physical_end);
+		ret2 = sync_write_pointer_for_zoned(sctx,
+				chunk_logical + offset,
+				map->stripes[stripe_index].physical,
+				physical_end);
 		if (ret2)
 			ret = ret2;
 	}
@@ -3585,8 +3588,8 @@ static noinline_for_stack int scrub_chunk(struct scrub_ctx *sctx,
 	for (i = 0; i < map->num_stripes; ++i) {
 		if (map->stripes[i].dev->bdev == scrub_dev->bdev &&
 		    map->stripes[i].physical == dev_offset) {
-			ret = scrub_stripe(sctx, map, scrub_dev, i,
-					   bg->start, dev_ext_len, bg);
+			ret = scrub_stripe(sctx, bg, map, scrub_dev, i,
+					   dev_ext_len);
 			if (ret)
 				goto out;
 		}
-- 
2.34.1

Re: [PATCH 1/2] btrfs: cleanup the argument list of scrub_chunk()

[David Sterba]

On Wed, Dec 15, 2021 at 02:59:41PM +0800, Qu Wenruo wrote:
> The argument list of scrub_chunk() has the following problems:
> 
> - Duplicated @chunk_offset
>   It is the same as btrfs_block_group::start.
> 
> - Confusing @length
>   The most instinctive guess is chunk length, and one may want to delete
>   it, but the truth is, it's device extent length.
> 
> Fix this by:
> 
> - Remove @chunk_offset
>   Use btrfs_block_group::start instead.
> 
> - Rename @length to @dev_ext_len
>   Also rename the caller to remove the ambiguous naming.
> 
> - Rename @cache to @bg
>   The "_cache" suffix for btrfs_block_group is removed for a while.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Good cleanups, added to misc-next, thanks. I've renamed the dev_ext_len
to dev_extent_len, as it's close to the key name BTRFS_DEV_EXTENT_KEY
and sounds more familiar.