diff mbox series

[05/15] btrfs: add a helper to iterate through a btrfs_bio with sector sized chunks

Message ID 20220517145039.3202184-6-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [01/15] btrfs: introduce a pure data checksum checking helper | expand

Commit Message

Christoph Hellwig May 17, 2022, 2:50 p.m. UTC 
From: Qu Wenruo <wqu@suse.com>

Add a helper that works similar to __bio_for_each_segment, but instead of
iterating over PAGE_SIZE chunks it iterates over each sector.

Signed-off-by: Qu Wenruo <wqu@suse.com>
[hch: split from a larger patch, and iterate over the offset instead of
      the offset bits]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/volumes.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Johannes Thumshirn May 17, 2022, 3:27 p.m. UTC | #1 
On 17/05/2022 16:51, Christoph Hellwig wrote:
> +/*
> + * Iterate through a btrfs_bio (@bbio) on a per-sector basis.
> + */
> +#define btrfs_bio_for_each_sector(fs_info, bvl, bbio, iter, bio_offset)	\
> +	for ((iter) = (bbio)->iter, (bio_offset) = 0;			\
> +	     (iter).bi_size &&					\
> +	     (((bvl) = bio_iter_iovec((&(bbio)->bio), (iter))), 1);	\
> +	     (bio_offset) += fs_info->sectorsize,			\
> +	     bio_advance_iter_single(&(bbio)->bio, &(iter),		\
> +	     (fs_info)->sectorsize))
> +
> +

This is first used in patch 12 why not move there?
Christoph Hellwig May 18, 2022, 8:46 a.m. UTC | #2 
On Tue, May 17, 2022 at 03:27:35PM +0000, Johannes Thumshirn wrote:
> On 17/05/2022 16:51, Christoph Hellwig wrote:
> > +/*
> > + * Iterate through a btrfs_bio (@bbio) on a per-sector basis.
> > + */
> > +#define btrfs_bio_for_each_sector(fs_info, bvl, bbio, iter, bio_offset)	\
> > +	for ((iter) = (bbio)->iter, (bio_offset) = 0;			\
> > +	     (iter).bi_size &&					\
> > +	     (((bvl) = bio_iter_iovec((&(bbio)->bio), (iter))), 1);	\
> > +	     (bio_offset) += fs_info->sectorsize,			\
> > +	     bio_advance_iter_single(&(bbio)->bio, &(iter),		\
> > +	     (fs_info)->sectorsize))
> > +
> > +
> 
> This is first used in patch 12 why not move there?

Because it is a logically separate change that doesn't really have
anything to do with the repair code, except that it happens to be the
first user?
Qu Wenruo May 18, 2022, 10:07 a.m. UTC | #3 
On 2022/5/18 16:46, Christoph Hellwig wrote:
> On Tue, May 17, 2022 at 03:27:35PM +0000, Johannes Thumshirn wrote:
>> On 17/05/2022 16:51, Christoph Hellwig wrote:
>>> +/*
>>> + * Iterate through a btrfs_bio (@bbio) on a per-sector basis.
>>> + */
>>> +#define btrfs_bio_for_each_sector(fs_info, bvl, bbio, iter, bio_offset)	\
>>> +	for ((iter) = (bbio)->iter, (bio_offset) = 0;			\
>>> +	     (iter).bi_size &&					\
>>> +	     (((bvl) = bio_iter_iovec((&(bbio)->bio), (iter))), 1);	\
>>> +	     (bio_offset) += fs_info->sectorsize,			\
>>> +	     bio_advance_iter_single(&(bbio)->bio, &(iter),		\
>>> +	     (fs_info)->sectorsize))
>>> +
>>> +
>>
>> This is first used in patch 12 why not move there?
>
> Because it is a logically separate change that doesn't really have
> anything to do with the repair code, except that it happens to be the
> first user?

In fact, there are some other call sites can benefit from the helper,
like btrfs_csum_one_bio().

Thus if you can convert those call sites, there will be no question on
introducing the helper in one patch.

Thanks,
Qu
Christoph Hellwig May 20, 2022, 4:27 p.m. UTC | #4 
On Wed, May 18, 2022 at 06:07:38PM +0800, Qu Wenruo wrote:
>> Because it is a logically separate change that doesn't really have
>> anything to do with the repair code, except that it happens to be the
>> first user?
>
> In fact, there are some other call sites can benefit from the helper,
> like btrfs_csum_one_bio().
>
> Thus if you can convert those call sites, there will be no question on
> introducing the helper in one patch.

Yes, there is a bunch of places where it would be pretty useful.  But
the series is already large enough as is, so for now I'd rather
keep that for another round.
Qu Wenruo May 21, 2022, 1:16 a.m. UTC | #5 
On 2022/5/21 00:27, Christoph Hellwig wrote:
> On Wed, May 18, 2022 at 06:07:38PM +0800, Qu Wenruo wrote:
>>> Because it is a logically separate change that doesn't really have
>>> anything to do with the repair code, except that it happens to be the
>>> first user?
>>
>> In fact, there are some other call sites can benefit from the helper,
>> like btrfs_csum_one_bio().
>>
>> Thus if you can convert those call sites, there will be no question on
>> introducing the helper in one patch.
>
> Yes, there is a bunch of places where it would be pretty useful.  But
> the series is already large enough as is, so for now I'd rather
> keep that for another round.

After more thought (refreshing bike riding), considering there are
already quite some independent fixes and cleanups, it looks reasonable
to me to split the patchset into two parts:

1. Independent cleanups/fixes/refactors
    Those can be queued for v5.20.
    And especially for this helper, I'd like to rework a lot of call
    sites to use this helpers to simplify the code.

2. Real read-time refactor
    In fact I also want to revive the old initial RFC version, and
    compare with different versions.

Does this sound OK?

Thanks,
Qu
diff mbox series

Patch

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 12b2af9260e92..6f784d4f54664 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -391,6 +391,18 @@  static inline void btrfs_bio_free_csum(struct btrfs_bio *bbio)
 	}
 }
 
+/*
+ * Iterate through a btrfs_bio (@bbio) on a per-sector basis.
+ */
+#define btrfs_bio_for_each_sector(fs_info, bvl, bbio, iter, bio_offset)	\
+	for ((iter) = (bbio)->iter, (bio_offset) = 0;			\
+	     (iter).bi_size &&					\
+	     (((bvl) = bio_iter_iovec((&(bbio)->bio), (iter))), 1);	\
+	     (bio_offset) += fs_info->sectorsize,			\
+	     bio_advance_iter_single(&(bbio)->bio, &(iter),		\
+	     (fs_info)->sectorsize))
+
+
 struct btrfs_io_stripe {
 	struct btrfs_device *dev;
 	u64 physical;