Re: [PATCH v2 4/5] btrfs: introduce btrfs_bio_for_each_block_all() helper

[David Sterba <dsterba@suse.cz>]

On Tue, Sep 02, 2025 at 06:32:15PM +0930, Qu Wenruo wrote:
> Currently if we want to iterate all blocks inside a bio, we do something
> like this:
> 
> 	bio_for_each_segment_all(bvec, bio, iter_all) {
> 		for (off = 0; off < bvec->bv_len; off += sectorsize) {
> 			/* Iterate blocks using bv + off */
> 		}
> 	}
> 
> That's fine for now, but it will not handle future bs > ps, as
> bio_for_each_segment_all() is a single-page iterator, it will always
> return a bvec that's no larger than a page.
> 
> But for bs > ps cases, we need a full folio (which covers at least one
> block) so that we can work on the block.
> 
> To address this problem and handle future bs > ps cases better:
> 
> - Introduce a helper btrfs_bio_for_each_block_all()
>   This helper will create a local bvec_iter, which has the size of the
>   target bio. Then grab the current physical address of the current
>   location, then advance the iterator by block size.
> 
> - Use btrfs_bio_for_each_block_all() to replace existing call sites
>   Including:
> 
>   * set_bio_pages_uptodate() in raid56
>   * verify_bio_data_sectors() in raid56
> 
>   Both will result much easier to read code.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/misc.h   | 24 +++++++++++++++++++++++
>  fs/btrfs/raid56.c | 49 +++++++++++++++++++----------------------------
>  2 files changed, 44 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/btrfs/misc.h b/fs/btrfs/misc.h
> index f210f808311f..2352ab56dbb3 100644
> --- a/fs/btrfs/misc.h
> +++ b/fs/btrfs/misc.h
> @@ -45,6 +45,30 @@ static inline phys_addr_t bio_iter_phys(struct bio *bio, struct bvec_iter *iter)
>  	     (paddr = bio_iter_phys((bio), (iter)), 1);			\
>  	     bio_advance_iter_single((bio), (iter), (blocksize)))
>  
> +/* Helper to initialize a bvec_iter to the size of the specified bio. */

Please drop 'Helper to ...'

> +static inline struct bvec_iter init_bvec_iter_for_bio(struct bio *bio)
> +{
> +	struct bio_vec *bvec;
> +	u32 bio_size = 0;
> +	int i;
> +
> +	bio_for_each_bvec_all(bvec, bio, i)
> +		bio_size += bvec->bv_len;
> +
> +	return (struct bvec_iter) {
> +		.bi_sector = 0,
> +		.bi_size = bio_size,
> +		.bi_idx = 0,
> +		.bi_bvec_done = 0,
> +	};

We don't use this kind of initializers, usually the structure is passed
as parameter, but I can see how it makes it convenient in the for()
initialization. The parameter way would work but would also look strange
so I think it's acceptable.

> +}
> +
> +#define btrfs_bio_for_each_block_all(paddr, bio, blocksize)		\
> +	for (struct bvec_iter iter = init_bvec_iter_for_bio(bio);	\
> +	     (iter).bi_size &&						\
> +	     (paddr = bio_iter_phys((bio), &(iter)), 1);		\
> +	     bio_advance_iter_single((bio), &(iter), (blocksize)))
> +
>  static inline void cond_wake_up(struct wait_queue_head *wq)
>  {
>  	/*