[PATCH RFC 0/3] btrfs: refactor how we handle btrfs_io_context and slightly reduce memory usage for both btrfs_bio and btrfs_io_context

[PATCH RFC 0/3] btrfs: refactor how we handle btrfs_io_context and slightly reduce memory usage for both btrfs_bio and btrfs_io_context

[Qu Wenruo]

Currently btrfs_io_context is utilized as both bio->bi_private for
mirrored stripes, and stripes mapping for RAID56.

This makes some members like btrfs_io_context::private to be there.

But in fact, since almost all bios in btrfs have btrfs_bio structure
appended, we don't really need to reuse bio::bi_private for mirrored
profiles.

So this patchset will:

- Introduce btrfs_bio::bioc member
  So that btrfs_io_context don't need to hold @private.
  This modification is still a net increase for memory usage, just
  a trade-off between btrfs_io_context and btrfs_bio.

- Replace btrfs_bio::device with btrfs_bio::stripe_num
  This reclaim the memory usage increase for btrfs_bio.

  But unfortunately, due to the short life time of btrfs_io_context,
  we don't have as good device status accounting as the old code.

  Now for csum error, we can no longer distinguish source and target
  device of dev-replace.

  This is the biggest blockage, and that's why it's RFC.

The result of the patchset is:

btrfs_bio:		size: 240 -> 240
btrfs_io_context:	size: 88 -> 80

Although to really reduce btrfs_bio, the main target should be
csum_inline.

Qu Wenruo (3):
  btrfs: add btrfs_bio::bioc pointer for further modification
  btrfs: remove redundant parameters for submit_stripe_bio()
  btrfs: replace btrfs_bio::device member with stripe_num

 fs/btrfs/compression.c |  6 ++----
 fs/btrfs/ctree.h       |  2 ++
 fs/btrfs/extent_io.c   |  2 --
 fs/btrfs/inode.c       | 30 +++++++++++++++++++++++++++---
 fs/btrfs/raid56.c      |  1 -
 fs/btrfs/volumes.c     | 23 +++++++++++++----------
 fs/btrfs/volumes.h     | 11 ++++++++---
 7 files changed, 52 insertions(+), 23 deletions(-)

-- 
2.33.0

[PATCH RFC 1/3] btrfs: add btrfs_bio::bioc pointer for further modification

[Qu Wenruo]

Currently we use btrfs_io_context for dual purposes:

- As bio->private for mirror based bio submission
  For those profiles, we assign bio->private to btrfs_io_context and
  save the old private/endio, and utilize bioc::stripes_pending.

- As pure stripe maps for RAID56
  For RAID56, btrfs will assemble its own raid_bio for physical
  submission. In that case, btrfs_io_context only provides stripe
  mapping, thus no need to utilize things like
  end_io/private/stripes_pending.

To make future members modifications, here we do a small change, by
introducing btrfs_bio::bioc pointer.

This modification will increase memory usage for btrfs_bio by 8 bytes,
but reduces btrfs_io_context by 8 bytes.
Overall it's still a net increase as btrfs_bio will be created for each
stripe, while btrfs_io_context exists once for all those involved
stripes.

This memory usage will be reduced by later commits.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d7cc24ed9620..86ff268369ec 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6557,16 +6557,16 @@ int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 
 static inline void btrfs_end_bioc(struct btrfs_io_context *bioc, struct bio *bio)
 {
-	bio->bi_private = bioc->private;
 	bio->bi_end_io = bioc->end_io;
 	bio_endio(bio);
+	btrfs_bio(bio)->bioc = NULL;
 
 	btrfs_put_bioc(bioc);
 }
 
 static void btrfs_end_bio(struct bio *bio)
 {
-	struct btrfs_io_context *bioc = bio->bi_private;
+	struct btrfs_io_context *bioc = btrfs_bio(bio)->bioc;
 	int is_orig_bio = 0;
 
 	if (bio->bi_status) {
@@ -6624,7 +6624,7 @@ static void submit_stripe_bio(struct btrfs_io_context *bioc, struct bio *bio,
 {
 	struct btrfs_fs_info *fs_info = bioc->fs_info;
 
-	bio->bi_private = bioc;
+	btrfs_bio(bio)->bioc = bioc;
 	btrfs_bio(bio)->device = dev;
 	bio->bi_end_io = btrfs_end_bio;
 	bio->bi_iter.bi_sector = physical >> 9;
@@ -6697,7 +6697,6 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 
 	total_devs = bioc->num_stripes;
 	bioc->orig_bio = first_bio;
-	bioc->private = first_bio->bi_private;
 	bioc->end_io = first_bio->bi_end_io;
 	bioc->fs_info = fs_info;
 	atomic_set(&bioc->stripes_pending, bioc->num_stripes);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 83075d6855db..384c483d2cef 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -311,6 +311,8 @@ struct btrfs_fs_devices {
 struct btrfs_bio {
 	unsigned int mirror_num;
 
+	struct btrfs_io_context *bioc;
+
 	/* @device is for stripe IO submission. */
 	struct btrfs_device *device;
 	u64 logical;
@@ -367,7 +369,6 @@ struct btrfs_io_context {
 	u64 map_type; /* get from map_lookup->type */
 	bio_end_io_t *end_io;
 	struct bio *orig_bio;
-	void *private;
 	atomic_t error;
 	int max_errors;
 	int num_stripes;
-- 
2.33.0

[PATCH RFC 2/3] btrfs: remove redundant parameters for submit_stripe_bio()

[Qu Wenruo]

Function submit_stripe_bio() is to submit bio using provided
btrfs_io_context, which will map the logical address to physical
address, and set the target device.

All the required info is already in bioc, including the device and
physical address, we only need to know which stripe we're targeting at.

This patch will replace @physical and @dev parameters with @stripe_nr.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 86ff268369ec..0c907a3eb3a7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6620,9 +6620,11 @@ static void btrfs_end_bio(struct bio *bio)
 }
 
 static void submit_stripe_bio(struct btrfs_io_context *bioc, struct bio *bio,
-			      u64 physical, struct btrfs_device *dev)
+			      unsigned int stripe_nr)
 {
 	struct btrfs_fs_info *fs_info = bioc->fs_info;
+	struct btrfs_device *dev = bioc->stripes[stripe_nr].dev;
+	const u64 physical = bioc->stripes[stripe_nr].physical;
 
 	btrfs_bio(bio)->bioc = bioc;
 	btrfs_bio(bio)->device = dev;
@@ -6674,7 +6676,6 @@ static void bioc_error(struct btrfs_io_context *bioc, struct bio *bio, u64 logic
 blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 			   int mirror_num)
 {
-	struct btrfs_device *dev;
 	struct bio *first_bio = bio;
 	u64 logical = bio->bi_iter.bi_sector << 9;
 	u64 length = 0;
@@ -6725,7 +6726,8 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 	}
 
 	for (dev_nr = 0; dev_nr < total_devs; dev_nr++) {
-		dev = bioc->stripes[dev_nr].dev;
+		struct btrfs_device *dev = bioc->stripes[dev_nr].dev;
+
 		if (!dev || !dev->bdev || test_bit(BTRFS_DEV_STATE_MISSING,
 						   &dev->dev_state) ||
 		    (btrfs_op(first_bio) == BTRFS_MAP_WRITE &&
@@ -6739,7 +6741,7 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 		else
 			bio = first_bio;
 
-		submit_stripe_bio(bioc, bio, bioc->stripes[dev_nr].physical, dev);
+		submit_stripe_bio(bioc, bio, dev_nr);
 	}
 	btrfs_bio_counter_dec(fs_info);
 	return BLK_STS_OK;
-- 
2.33.0

[PATCH RFC 3/3] btrfs: replace btrfs_bio::device member with stripe_num

[Qu Wenruo]

Structure btrfs_bio uses @device member to record which the underlying
physical device is for each stripe.

However btrfs_bio::device is only utilized by two types of call sites:

- btrfs_end_bio()
- check_data_sum() and check_compressed_csum()

Both types of call sites are just to update device status.

For btrfs_end_bio(), in the context we can easily grab bioc from
btrfs_bio, and with the new @stripe_num member, we can easily grab the
stripe and its device.

This is as good as the original code.

For check_data_sum() and check_compressed_csum(), we have
btrfs_bio::mirror_num, and can afford to do a logical->physical mapping
lookup to grab the device.

Unfortunately this path is not as good as the original code, as we can't
distinguish target and source device for dev-replace.

But this should save us 8 bytes from btrfs_bio, thus I still think the
benefit can cover the shortcoming.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c |  6 ++----
 fs/btrfs/ctree.h       |  2 ++
 fs/btrfs/extent_io.c   |  2 --
 fs/btrfs/inode.c       | 30 +++++++++++++++++++++++++++---
 fs/btrfs/raid56.c      |  1 -
 fs/btrfs/volumes.c     |  6 ++++--
 fs/btrfs/volumes.h     | 10 +++++++---
 7 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 2a86a2a1494b..28c35f58c2cd 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -179,10 +179,8 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
 			if (memcmp(&csum, cb_sum, csum_size) != 0) {
 				btrfs_print_data_csum_error(inode, disk_start,
 						csum, cb_sum, cb->mirror_num);
-				if (btrfs_bio(bio)->device)
-					btrfs_dev_stat_inc_and_print(
-						btrfs_bio(bio)->device,
-						BTRFS_DEV_STAT_CORRUPTION_ERRS);
+				btrfs_dev_stat_inc_and_print_by_bbio(fs_info,
+								btrfs_bio(bio));
 				return -EIO;
 			}
 			cb_sum += csum_size;
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 522ded06fd85..515750bb7a8e 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3141,6 +3141,8 @@ u64 btrfs_file_extent_end(const struct btrfs_path *path);
 /* inode.c */
 blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
 				   int mirror_num, unsigned long bio_flags);
+void btrfs_dev_stat_inc_and_print_by_bbio(struct btrfs_fs_info *fs_info,
+					  struct btrfs_bio *bbio);
 unsigned int btrfs_verify_data_csum(struct btrfs_bio *bbio,
 				    u32 bio_offset, struct page *page,
 				    u64 start, u64 end);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c56973f7daae..127885f5413a 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3331,8 +3331,6 @@ static int alloc_new_bio(struct btrfs_inode *inode,
 			ret = PTR_ERR(device);
 			goto error;
 		}
-
-		btrfs_bio(bio)->device = device;
 	}
 	return 0;
 error:
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a643be30c18a..29945ff4002c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3206,6 +3206,32 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
 				       finish_ordered_fn, uptodate);
 }
 
+/*
+ * This is a cold path for csum mismatch case, we can afford to a
+ * logical->physical address lookup to locate the device using mirror_num.
+ */
+void btrfs_dev_stat_inc_and_print_by_bbio(struct btrfs_fs_info *fs_info,
+					  struct btrfs_bio *bbio)
+{
+	struct btrfs_io_context *bioc;
+	struct btrfs_device *dev;
+	u64 logical = bbio->logical;
+	u64 length = fs_info->sectorsize;
+	unsigned int mirror_num = bbio->mirror_num;
+	int ret;
+
+	ret = btrfs_map_block(fs_info, BTRFS_MAP_READ, logical, &length, &bioc,
+			      mirror_num);
+	/* Failed to do such map, just skip the dev status update */
+	if (ret < 0)
+		return;
+	dev = bioc->stripes[0].dev;
+	btrfs_put_bioc(bioc);
+	if (dev)
+		btrfs_dev_stat_inc_and_print(dev,
+					     BTRFS_DEV_STAT_CORRUPTION_ERRS);
+}
+
 /*
  * check_data_csum - verify checksum of one sector of uncompressed data
  * @inode:	inode
@@ -3248,9 +3274,7 @@ static int check_data_csum(struct inode *inode, struct btrfs_bio *bbio,
 zeroit:
 	btrfs_print_data_csum_error(BTRFS_I(inode), start, csum, csum_expected,
 				    bbio->mirror_num);
-	if (bbio->device)
-		btrfs_dev_stat_inc_and_print(bbio->device,
-					     BTRFS_DEV_STAT_CORRUPTION_ERRS);
+	btrfs_dev_stat_inc_and_print_by_bbio(fs_info, bbio);
 	memset(kaddr + pgoff, 1, len);
 	flush_dcache_page(page);
 	kunmap_atomic(kaddr);
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 02aa3fbb8108..28ef777ce51f 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1105,7 +1105,6 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,
 
 	/* put a new bio on the list */
 	bio = btrfs_bio_alloc(bio_max_len >> PAGE_SHIFT ?: 1);
-	btrfs_bio(bio)->device = stripe->dev;
 	bio->bi_iter.bi_size = 0;
 	bio_set_dev(bio, stripe->dev->bdev);
 	bio->bi_iter.bi_sector = disk_start >> 9;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0c907a3eb3a7..b66e336dbe5e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6573,8 +6573,10 @@ static void btrfs_end_bio(struct bio *bio)
 		atomic_inc(&bioc->error);
 		if (bio->bi_status == BLK_STS_IOERR ||
 		    bio->bi_status == BLK_STS_TARGET) {
-			struct btrfs_device *dev = btrfs_bio(bio)->device;
+			struct btrfs_bio *bbio = btrfs_bio(bio);
+			struct btrfs_device *dev;
 
+			dev = bioc->stripes[bbio->stripe_num].dev;
 			ASSERT(dev->bdev);
 			if (btrfs_op(bio) == BTRFS_MAP_WRITE)
 				btrfs_dev_stat_inc_and_print(dev,
@@ -6627,7 +6629,7 @@ static void submit_stripe_bio(struct btrfs_io_context *bioc, struct bio *bio,
 	const u64 physical = bioc->stripes[stripe_nr].physical;
 
 	btrfs_bio(bio)->bioc = bioc;
-	btrfs_bio(bio)->device = dev;
+	btrfs_bio(bio)->stripe_num = stripe_nr;
 	bio->bi_end_io = btrfs_end_bio;
 	bio->bi_iter.bi_sector = physical >> 9;
 	/*
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 384c483d2cef..5bae94843e49 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -310,11 +310,15 @@ struct btrfs_fs_devices {
  */
 struct btrfs_bio {
 	unsigned int mirror_num;
+	/*
+	 * @stripe_num is for stripe IO submission, under most case it should
+	 * be the same as @mirror_num, but for dev replace case it can be
+	 * different as we need to submit the bio to both source and target
+	 * device.
+	 */
+	unsigned int stripe_num;
 
 	struct btrfs_io_context *bioc;
-
-	/* @device is for stripe IO submission. */
-	struct btrfs_device *device;
 	u64 logical;
 	u8 *csum;
 	u8 csum_inline[BTRFS_BIO_INLINE_CSUM_SIZE];
-- 
2.33.0

Re: [PATCH RFC 0/3] btrfs: refactor how we handle btrfs_io_context and slightly reduce memory usage for both btrfs_bio and btrfs_io_context

[Qu Wenruo]

On 2021/9/22 16:27, Qu Wenruo wrote:
> Currently btrfs_io_context is utilized as both bio->bi_private for
> mirrored stripes, and stripes mapping for RAID56.
> 
> This makes some members like btrfs_io_context::private to be there.
> 
> But in fact, since almost all bios in btrfs have btrfs_bio structure
> appended, we don't really need to reuse bio::bi_private for mirrored
> profiles.
> 
> So this patchset will:
> 
> - Introduce btrfs_bio::bioc member
>    So that btrfs_io_context don't need to hold @private.
>    This modification is still a net increase for memory usage, just
>    a trade-off between btrfs_io_context and btrfs_bio.
> 
> - Replace btrfs_bio::device with btrfs_bio::stripe_num
>    This reclaim the memory usage increase for btrfs_bio.
> 
>    But unfortunately, due to the short life time of btrfs_io_context,
>    we don't have as good device status accounting as the old code.
> 
>    Now for csum error, we can no longer distinguish source and target
>    device of dev-replace.
> 
>    This is the biggest blockage, and that's why it's RFC.

OK, here I'm over-thinking.

The truth is, dev-replace only affects two types of operations, WRITE 
and GET_READ_MIRRORS.

The former case won't need to bother csum mismatch, while the latter 
case won't result a bio used directly for read.

Thus it brings no behavior change.

Thanks,
Qu
> 
> The result of the patchset is:
> 
> btrfs_bio:		size: 240 -> 240
> btrfs_io_context:	size: 88 -> 80
> 
> Although to really reduce btrfs_bio, the main target should be
> csum_inline.
> 
> Qu Wenruo (3):
>    btrfs: add btrfs_bio::bioc pointer for further modification
>    btrfs: remove redundant parameters for submit_stripe_bio()
>    btrfs: replace btrfs_bio::device member with stripe_num
> 
>   fs/btrfs/compression.c |  6 ++----
>   fs/btrfs/ctree.h       |  2 ++
>   fs/btrfs/extent_io.c   |  2 --
>   fs/btrfs/inode.c       | 30 +++++++++++++++++++++++++++---
>   fs/btrfs/raid56.c      |  1 -
>   fs/btrfs/volumes.c     | 23 +++++++++++++----------
>   fs/btrfs/volumes.h     | 11 ++++++++---
>   7 files changed, 52 insertions(+), 23 deletions(-)
> 

Re: [PATCH RFC 0/3] btrfs: refactor how we handle btrfs_io_context and slightly reduce memory usage for both btrfs_bio and btrfs_io_context

[David Sterba]

On Wed, Sep 22, 2021 at 04:27:03PM +0800, Qu Wenruo wrote:
> Currently btrfs_io_context is utilized as both bio->bi_private for
> mirrored stripes, and stripes mapping for RAID56.
> 
> This makes some members like btrfs_io_context::private to be there.
> 
> But in fact, since almost all bios in btrfs have btrfs_bio structure
> appended, we don't really need to reuse bio::bi_private for mirrored
> profiles.
> 
> So this patchset will:
> 
> - Introduce btrfs_bio::bioc member
>   So that btrfs_io_context don't need to hold @private.
>   This modification is still a net increase for memory usage, just
>   a trade-off between btrfs_io_context and btrfs_bio.
> 
> - Replace btrfs_bio::device with btrfs_bio::stripe_num
>   This reclaim the memory usage increase for btrfs_bio.
> 
>   But unfortunately, due to the short life time of btrfs_io_context,
>   we don't have as good device status accounting as the old code.
> 
>   Now for csum error, we can no longer distinguish source and target
>   device of dev-replace.
> 
>   This is the biggest blockage, and that's why it's RFC.
> 
> The result of the patchset is:
> 
> btrfs_bio:		size: 240 -> 240
> btrfs_io_context:	size: 88 -> 80
> 
> Although to really reduce btrfs_bio, the main target should be
> csum_inline.
> 
> Qu Wenruo (3):
>   btrfs: add btrfs_bio::bioc pointer for further modification
>   btrfs: remove redundant parameters for submit_stripe_bio()
>   btrfs: replace btrfs_bio::device member with stripe_num

Can you please refresh the patchset on top current misc-next?

Re: [PATCH RFC 0/3] btrfs: refactor how we handle btrfs_io_context and slightly reduce memory usage for both btrfs_bio and btrfs_io_context

[Qu Wenruo]

On 2021/10/7 03:38, David Sterba wrote:
> On Wed, Sep 22, 2021 at 04:27:03PM +0800, Qu Wenruo wrote:
>> Currently btrfs_io_context is utilized as both bio->bi_private for
>> mirrored stripes, and stripes mapping for RAID56.
>>
>> This makes some members like btrfs_io_context::private to be there.
>>
>> But in fact, since almost all bios in btrfs have btrfs_bio structure
>> appended, we don't really need to reuse bio::bi_private for mirrored
>> profiles.
>>
>> So this patchset will:
>>
>> - Introduce btrfs_bio::bioc member
>>    So that btrfs_io_context don't need to hold @private.
>>    This modification is still a net increase for memory usage, just
>>    a trade-off between btrfs_io_context and btrfs_bio.
>>
>> - Replace btrfs_bio::device with btrfs_bio::stripe_num
>>    This reclaim the memory usage increase for btrfs_bio.
>>
>>    But unfortunately, due to the short life time of btrfs_io_context,
>>    we don't have as good device status accounting as the old code.
>>
>>    Now for csum error, we can no longer distinguish source and target
>>    device of dev-replace.
>>
>>    This is the biggest blockage, and that's why it's RFC.
>>
>> The result of the patchset is:
>>
>> btrfs_bio:		size: 240 -> 240
>> btrfs_io_context:	size: 88 -> 80
>>
>> Although to really reduce btrfs_bio, the main target should be
>> csum_inline.
>>
>> Qu Wenruo (3):
>>    btrfs: add btrfs_bio::bioc pointer for further modification
>>    btrfs: remove redundant parameters for submit_stripe_bio()
>>    btrfs: replace btrfs_bio::device member with stripe_num
>
> Can you please refresh the patchset on top current misc-next?
>

Please discard the patchset for now.

Unfortunately I have found one critical member abuses making such
refactor unfeasible (at least for now).

- btrfs_bio::logical abuse
   Direct IO uses btrfs_bio::logical as file_offset, while no other
   call sites really utilize btrfs_bio::logical at all.

   This means, we can't simply rely on btrfs_map_bio() to verify if
   btrfs_bio::logical is correctly initialized.

   This is a big alert, if we can't do ASSERT() to verify if one member
   is properly initialized, then it's just going to happen.

   This also means, for direct IO, we can't use @mirror_num with @logical
   to grab the device with IO failure.

So for now, although it may save 8 bytes, unless we solve the DIO mess,
we can't continue.

Thanks,
Qu

Re: [PATCH RFC 0/3] btrfs: refactor how we handle btrfs_io_context and slightly reduce memory usage for both btrfs_bio and btrfs_io_context

[David Sterba]

On Thu, Oct 07, 2021 at 10:24:27AM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/10/7 03:38, David Sterba wrote:
> > On Wed, Sep 22, 2021 at 04:27:03PM +0800, Qu Wenruo wrote:
> >> Currently btrfs_io_context is utilized as both bio->bi_private for
> >> mirrored stripes, and stripes mapping for RAID56.
> >>
> >> This makes some members like btrfs_io_context::private to be there.
> >>
> >> But in fact, since almost all bios in btrfs have btrfs_bio structure
> >> appended, we don't really need to reuse bio::bi_private for mirrored
> >> profiles.
> >>
> >> So this patchset will:
> >>
> >> - Introduce btrfs_bio::bioc member
> >>    So that btrfs_io_context don't need to hold @private.
> >>    This modification is still a net increase for memory usage, just
> >>    a trade-off between btrfs_io_context and btrfs_bio.
> >>
> >> - Replace btrfs_bio::device with btrfs_bio::stripe_num
> >>    This reclaim the memory usage increase for btrfs_bio.
> >>
> >>    But unfortunately, due to the short life time of btrfs_io_context,
> >>    we don't have as good device status accounting as the old code.
> >>
> >>    Now for csum error, we can no longer distinguish source and target
> >>    device of dev-replace.
> >>
> >>    This is the biggest blockage, and that's why it's RFC.
> >>
> >> The result of the patchset is:
> >>
> >> btrfs_bio:		size: 240 -> 240
> >> btrfs_io_context:	size: 88 -> 80
> >>
> >> Although to really reduce btrfs_bio, the main target should be
> >> csum_inline.
> >>
> >> Qu Wenruo (3):
> >>    btrfs: add btrfs_bio::bioc pointer for further modification
> >>    btrfs: remove redundant parameters for submit_stripe_bio()
> >>    btrfs: replace btrfs_bio::device member with stripe_num
> >
> > Can you please refresh the patchset on top current misc-next?
> >
> 
> Please discard the patchset for now.
> 
> Unfortunately I have found one critical member abuses making such
> refactor unfeasible (at least for now).
> 
> - btrfs_bio::logical abuse
>    Direct IO uses btrfs_bio::logical as file_offset, while no other
>    call sites really utilize btrfs_bio::logical at all.
> 
>    This means, we can't simply rely on btrfs_map_bio() to verify if
>    btrfs_bio::logical is correctly initialized.
> 
>    This is a big alert, if we can't do ASSERT() to verify if one member
>    is properly initialized, then it's just going to happen.
> 
>    This also means, for direct IO, we can't use @mirror_num with @logical
>    to grab the device with IO failure.
> 
> So for now, although it may save 8 bytes, unless we solve the DIO mess,
> we can't continue.

Ok, thanks.