Re: [PATCH 03/34] btrfs: add a btrfs_inode pointer to struct btrfs_bio

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Qu Wenruo <wqu@suse.com>,
	linux-btrfs@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 03/34] btrfs: add a btrfs_inode pointer to struct btrfs_bio
Date: Thu, 9 Mar 2023 08:08:34 +0800	[thread overview]
Message-ID: <9c59ce30-f217-568e-a3a0-f5a8fd1ac107@gmx.com> (raw)
In-Reply-To: <20230308142817.GA14929@lst.de>

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]



On 2023/3/8 22:28, Christoph Hellwig wrote:
> On Wed, Mar 08, 2023 at 02:04:26PM +0800, Qu Wenruo wrote:
>> BTW, I also checked if I can craft a scrub specific version of
>> btrfs_submit_bio().
>>
>> The result doesn't look good at all.
>>
>> Without a btrfs_bio structure, it's already pretty hard to properly put
>> bioc, decrease the bio counter.
>>
>> Or I need to create a scrub_bio, and re-implement all the needed endio
>> function handling.
>>
>> So please really consider the simplest case, one just wants to read/write
>> some data using logical + mirror_num, without any btrfs inode nor csum
>> verification.
> 
> As said before with a little more work we could get away without the
> inode.  But the above sounds a little strange to me.  Can you share
> your current code?  Maybe I can come up with some better ideas.

My current one is a new btrfs_submit_scrub_read() helper, getting rid of 
features I don't need and slightly modify the endio functions to avoid 
any checks if no bbio->inode. AKA, most of your idea.

So that would be mostly fine.


But what I'm exploring is to completely get rid of btrfs_ino, using 
plain bio.

Things like bio counter is not a big deal as the only caller is 
scrub/dev-replace, thus there would be no race to change dev-replace 
half-way.
So is bioc for non-RAID56 profiles, as we only need to grab the 
dev/physical and can release it immediately.

But for RAID56, the bioc has to live long enough for raid56 work to 
finish, thus has to go btrfs_raid56_end_io() and rely on the extra 
bbio->end_io().

I guess I have to accept the defeat and just go btrfs_bio.

Thanks,
Qu

[-- Attachment #2: 0002-btrfs-introduce-a-new-helper-to-submit-bio-for-scrub.patch --]
[-- Type: text/x-patch, Size: 4002 bytes --]

From 85abc1d8b0d463f9db6d74a5809b10487ae21de8 Mon Sep 17 00:00:00 2001
Message-Id: <85abc1d8b0d463f9db6d74a5809b10487ae21de8.1678261377.git.wqu@suse.com>
In-Reply-To: <cover.1678261377.git.wqu@suse.com>
References: <cover.1678261377.git.wqu@suse.com>
From: Qu Wenruo <wqu@suse.com>
Date: Wed, 8 Mar 2023 14:22:10 +0800
Subject: [PATCH 02/10] btrfs: introduce a new helper to submit bio for scrub

The new helper, btrfs_submit_scrub_read(), would be mostly a subset of
btrfs_submit_bio(), with the following limitations:

- Only supports read
- @mirror_num must be > 0
- No read-time repair nor checksum verification
- The @bbio must not cross stripe boundary

This would provide the basis for unified read repair for scrub, as we no
longer needs to handle RAID56 recovery all by scrub, and RAID56 data
stripes scrub can share the same code of read and repair.

The repair part would be the same as non-RAID56, as we only need to try
the next mirror.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/bio.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 fs/btrfs/bio.h |  2 ++
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index 726592868e9c..966517ad81ce 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -305,8 +305,8 @@ static void btrfs_end_bio_work(struct work_struct *work)
 {
 	struct btrfs_bio *bbio = container_of(work, struct btrfs_bio, end_io_work);
 
-	/* Metadata reads are checked and repaired by the submitter. */
-	if (bbio->bio.bi_opf & REQ_META)
+	/* Metadata or scrub reads are checked and repaired by the submitter. */
+	if (bbio->bio.bi_opf & REQ_META || !bbio->inode)
 		bbio->end_io(bbio);
 	else
 		btrfs_check_read_bio(bbio, bbio->bio.bi_private);
@@ -340,7 +340,8 @@ static void btrfs_raid56_end_io(struct bio *bio)
 
 	btrfs_bio_counter_dec(bioc->fs_info);
 	bbio->mirror_num = bioc->mirror_num;
-	if (bio_op(bio) == REQ_OP_READ && !(bbio->bio.bi_opf & REQ_META))
+	if (bio_op(bio) == REQ_OP_READ && bbio->inode &&
+	    !(bbio->bio.bi_opf & REQ_META))
 		btrfs_check_read_bio(bbio, NULL);
 	else
 		btrfs_orig_bbio_end_io(bbio);
@@ -686,6 +687,42 @@ static bool btrfs_submit_chunk(struct bio *bio, int mirror_num)
 	return true;
 }
 
+/*
+ * Scrub read special version, with extra limits:
+ *
+ * - Only support read for scrub usage
+ * - @mirror_num must be >0
+ * - No read-time repair nor checksum verification.
+ * - The @bbio must not cross stripe boundary.
+ */
+void btrfs_submit_scrub_read(struct btrfs_fs_info *fs_info,
+			     struct btrfs_bio *bbio, int mirror_num)
+{
+	struct btrfs_bio *orig_bbio = bbio;
+	u64 logical = bbio->bio.bi_iter.bi_sector << SECTOR_SHIFT;
+	u64 length = bbio->bio.bi_iter.bi_size;
+	u64 map_length = length;
+	struct btrfs_io_context *bioc = NULL;
+	struct btrfs_io_stripe smap;
+	int ret;
+
+	ASSERT(mirror_num > 0);
+	ASSERT(btrfs_op(&bbio->bio) == BTRFS_MAP_READ);
+	btrfs_bio_counter_inc_blocked(fs_info);
+	ret = __btrfs_map_block(fs_info, btrfs_op(&bbio->bio), logical,
+				&map_length, &bioc, &smap, &mirror_num, 1);
+	if (ret)
+		goto fail;
+
+	ASSERT(map_length == length);
+	__btrfs_submit_bio(&bbio->bio, bioc, &smap, mirror_num);
+	return;
+
+fail:
+	btrfs_bio_counter_dec(fs_info);
+	btrfs_bio_end_io(orig_bbio, ret);
+}
+
 void btrfs_submit_bio(struct bio *bio, int mirror_num)
 {
 	while (!btrfs_submit_chunk(bio, mirror_num))
diff --git a/fs/btrfs/bio.h b/fs/btrfs/bio.h
index 873ff85817f0..341d579bd0b9 100644
--- a/fs/btrfs/bio.h
+++ b/fs/btrfs/bio.h
@@ -89,6 +89,8 @@ static inline void btrfs_bio_end_io(struct btrfs_bio *bbio, blk_status_t status)
 #define REQ_BTRFS_ONE_ORDERED			REQ_DRV
 
 void btrfs_submit_bio(struct bio *bio, int mirror_num);
+void btrfs_submit_scrub_read(struct btrfs_fs_info *fs_info,
+			     struct btrfs_bio *bbio, int mirror_num);
 int btrfs_repair_io_failure(struct btrfs_fs_info *fs_info, u64 ino, u64 start,
 			    u64 length, u64 logical, struct page *page,
 			    unsigned int pg_offset, int mirror_num);
-- 
2.39.1

