* [PATCH RFC 1/3] btrfs: add btrfs_bio::bioc pointer for further modification
2021-09-22 8:27 [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
@ 2021-09-22 8:27 ` Qu Wenruo
2021-09-22 8:27 ` [PATCH RFC 2/3] btrfs: remove redundant parameters for submit_stripe_bio() Qu Wenruo
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Qu Wenruo @ 2021-09-22 8:27 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH RFC 2/3] btrfs: remove redundant parameters for submit_stripe_bio()
2021-09-22 8:27 [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
2021-09-22 8:27 ` [PATCH RFC 1/3] btrfs: add btrfs_bio::bioc pointer for further modification Qu Wenruo
@ 2021-09-22 8:27 ` Qu Wenruo
2021-09-22 8:27 ` [PATCH RFC 3/3] btrfs: replace btrfs_bio::device member with stripe_num Qu Wenruo
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Qu Wenruo @ 2021-09-22 8:27 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH RFC 3/3] btrfs: replace btrfs_bio::device member with stripe_num
2021-09-22 8:27 [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
2021-09-22 8:27 ` [PATCH RFC 1/3] btrfs: add btrfs_bio::bioc pointer for further modification Qu Wenruo
2021-09-22 8:27 ` [PATCH RFC 2/3] btrfs: remove redundant parameters for submit_stripe_bio() Qu Wenruo
@ 2021-09-22 8:27 ` Qu Wenruo
2021-09-22 9:57 ` [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
2021-10-06 19:38 ` David Sterba
4 siblings, 0 replies; 8+ messages in thread
From: Qu Wenruo @ 2021-09-22 8:27 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* 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
2021-09-22 8:27 [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
` (2 preceding siblings ...)
2021-09-22 8:27 ` [PATCH RFC 3/3] btrfs: replace btrfs_bio::device member with stripe_num Qu Wenruo
@ 2021-09-22 9:57 ` Qu Wenruo
2021-10-06 19:38 ` David Sterba
4 siblings, 0 replies; 8+ messages in thread
From: Qu Wenruo @ 2021-09-22 9:57 UTC (permalink / raw)
To: linux-btrfs
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(-)
>
^ permalink raw reply [flat|nested] 8+ messages in thread* 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
2021-09-22 8:27 [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
` (3 preceding siblings ...)
2021-09-22 9:57 ` [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
@ 2021-10-06 19:38 ` David Sterba
2021-10-07 2:24 ` Qu Wenruo
4 siblings, 1 reply; 8+ messages in thread
From: David Sterba @ 2021-10-06 19:38 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
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?
^ permalink raw reply [flat|nested] 8+ messages in thread* 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
2021-10-06 19:38 ` David Sterba
@ 2021-10-07 2:24 ` Qu Wenruo
2021-10-07 11:04 ` David Sterba
0 siblings, 1 reply; 8+ messages in thread
From: Qu Wenruo @ 2021-10-07 2:24 UTC (permalink / raw)
To: dsterba, Qu Wenruo, linux-btrfs
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* 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
2021-10-07 2:24 ` Qu Wenruo
@ 2021-10-07 11:04 ` David Sterba
0 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2021-10-07 11:04 UTC (permalink / raw)
To: Qu Wenruo; +Cc: dsterba, Qu Wenruo, linux-btrfs
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.
^ permalink raw reply [flat|nested] 8+ messages in thread