Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Date:   Wed, 13 Dec 2023 00:58:38 -0800
From:   Christoph Hellwig <hch@infradead.org>
To:     Johannes Thumshirn <johannes.thumshirn@wdc.com>
Cc:     Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>, linux-btrfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 11/13] btrfs: open code set_io_stripe for RAID56
Message-ID: <ZXlyPqtXO+j90vJb@infradead.org>
References: <20231212-btrfs_map_block-cleanup-v1-0-b2d954d9a55b@wdc.com>
 <20231212-btrfs_map_block-cleanup-v1-11-b2d954d9a55b@wdc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231212-btrfs_map_block-cleanup-v1-11-b2d954d9a55b@wdc.com>
Precedence: bulk

On Tue, Dec 12, 2023 at 04:38:09AM -0800, Johannes Thumshirn wrote:
> Open code set_io_stripe() for RAID56, as it a) uses a different method to
> calculate the stripe_index and b) doesn't need to go through raid-stripe-tree
> mapping code.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

I think raid stripe tree handling also really should move out of
set_io_stripe.  Below is the latest I have, although it probably won't
apply to your tree:

---
From ac208da48d7f9d11eef8a01ac0c6fbf9681665b5 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 22 Jun 2023 05:53:13 +0200
Subject: btrfs: move raid-stripe-tree handling out of set_io_stripe

set_io_stripe gets a little too complicated with the raid-stripe-tree
handling.  Move it out into the only callers that actually needs it.

The only reads with more than a single stripe is the parity raid recovery
case thast will need very special handling anyway once implemented.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/volumes.c | 61 ++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 30ee5d1670d034..e32eefa242b0a4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6233,22 +6233,12 @@ static u64 btrfs_max_io_len(struct map_lookup *map, enum btrfs_map_op op,
 	return U64_MAX;
 }
 
-static int set_io_stripe(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
-		      u64 logical, u64 *length, struct btrfs_io_stripe *dst,
-		      struct map_lookup *map, u32 stripe_index,
-		      u64 stripe_offset, u64 stripe_nr)
+static void set_io_stripe(struct btrfs_io_stripe *dst, const struct map_lookup *map,
+			  u32 stripe_index, u64 stripe_offset, u32 stripe_nr)
 {
 	dst->dev = map->stripes[stripe_index].dev;
-
-	if (op == BTRFS_MAP_READ &&
-	    btrfs_use_raid_stripe_tree(fs_info, map->type))
-		return btrfs_get_raid_extent_offset(fs_info, logical, length,
-						    map->type, stripe_index,
-						    dst);
-
 	dst->physical = map->stripes[stripe_index].physical +
 			stripe_offset + ((u64)stripe_nr << BTRFS_STRIPE_LEN_SHIFT);
-	return 0;
 }
 
 int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
@@ -6423,15 +6413,24 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	 * physical block information on the stack instead of allocating an
 	 * I/O context structure.
 	 */
-	if (smap && num_alloc_stripes == 1 &&
-	    !(btrfs_use_raid_stripe_tree(fs_info, map->type) &&
-	      op != BTRFS_MAP_READ) &&
-	    !((map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) && mirror_num > 1)) {
-		ret = set_io_stripe(fs_info, op, logical, length, smap, map,
-				    stripe_index, stripe_offset, stripe_nr);
-		*mirror_num_ret = mirror_num;
-		*bioc_ret = NULL;
-		goto out;
+	if (smap && num_alloc_stripes == 1) {
+		if (op == BTRFS_MAP_READ &&
+		    btrfs_use_raid_stripe_tree(fs_info, map->type)) {
+			ret = btrfs_get_raid_extent_offset(fs_info, logical,
+							   length, map->type,
+							   stripe_index, smap);
+			*mirror_num_ret = mirror_num;
+			*bioc_ret = NULL;
+			goto out;
+		} else if (!(map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) ||
+			   mirror_num == 0) {
+			set_io_stripe(smap, map, stripe_index, stripe_offset,
+				      stripe_nr);
+			*mirror_num_ret = mirror_num;
+			*bioc_ret = NULL;
+			ret = 0;
+			goto out;
+		}
 	}
 
 	bioc = alloc_btrfs_io_context(fs_info, logical, num_alloc_stripes);
@@ -6448,6 +6447,8 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	 *
 	 * It's still mostly the same as other profiles, just with extra rotation.
 	 */
+	ASSERT(op != BTRFS_MAP_READ ||
+	       btrfs_use_raid_stripe_tree(fs_info, map->type));
 	if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK && need_raid_map &&
 	    (op != BTRFS_MAP_READ || mirror_num > 1)) {
 		/*
@@ -6461,29 +6462,21 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		bioc->full_stripe_logical = em->start +
 			((stripe_nr * data_stripes) << BTRFS_STRIPE_LEN_SHIFT);
 		for (i = 0; i < num_stripes; i++)
-			ret = set_io_stripe(fs_info, op, logical, length,
-					    &bioc->stripes[i], map,
-					    (i + stripe_nr) % num_stripes,
-					    stripe_offset, stripe_nr);
+			set_io_stripe(&bioc->stripes[i], map,
+				      (i + stripe_nr) % num_stripes,
+				      stripe_offset, stripe_nr);
 	} else {
 		/*
 		 * For all other non-RAID56 profiles, just copy the target
 		 * stripe into the bioc.
 		 */
 		for (i = 0; i < num_stripes; i++) {
-			ret = set_io_stripe(fs_info, op, logical, length,
-					    &bioc->stripes[i], map, stripe_index,
-					    stripe_offset, stripe_nr);
+			set_io_stripe(&bioc->stripes[i], map, stripe_index,
+				      stripe_offset, stripe_nr);
 			stripe_index++;
 		}
 	}
 
-	if (ret) {
-		*bioc_ret = NULL;
-		btrfs_put_bioc(bioc);
-		goto out;
-	}
-
 	if (op != BTRFS_MAP_READ)
 		max_errors = btrfs_chunk_max_errors(map);
 
-- 
2.39.2