Re: [PATCH 3/4] btrfs: replace single page bio_iter_iovec() usage

[Qu Wenruo <wqu@suse.com>]

在 2025/9/1 14:54, Qu Wenruo 写道:
> There are several functions inside btrfs calling bio_iter_iovec(),
> mostly to do a block-by-block iteration on a bio.
> 
> - btrfs_check_read_bio()
> - btrfs_decompress_buf2page()
> - index_one_bio() from raid56
> 
> However that helper is single page based, meaning it will never return a
> bv_len larger than PAGE_SIZE. For now it's fine as we only support bs <=
> ps at most.
> 
> But for the incoming bs > ps support, we want to get bv_len larger than
> PAGE_SIZE so that the bio_vec will cover a full block, not just part of
> the large folio of the block.
> 
> In fact the call site inside btrfs_check_read_bio() will trigger
> ASSERT() inside btrfs_data_csum_ok() when bs > ps support is enabled.
> As bio_iter_iovec() will return a bv_len == 4K, meanwhile the bs is
> larger than 4K, triggering the ASSERT().
> 
> Replace those call sites with mp_bvec_iter_bvec(), which will return the
> full length of from the bi_io_vec array.
> Currently all call sites are already doing extra loop inside the bvec
> range for bs < ps support, so they will be fine.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/bio.c         | 3 ++-
>   fs/btrfs/compression.c | 3 +--
>   fs/btrfs/raid56.c      | 2 +-
>   3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index ea7f7a17a3d5..f7aea4310dd6 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -277,8 +277,9 @@ static void btrfs_check_read_bio(struct btrfs_bio *bbio, struct btrfs_device *de
>   	bbio->bio.bi_status = BLK_STS_OK;
>   
>   	while (iter->bi_size) {
> -		struct bio_vec bv = bio_iter_iovec(&bbio->bio, *iter);
> +		struct bio_vec bv = mp_bvec_iter_bvec(bbio->bio.bi_io_vec, *iter);

This multi-page conversion is going to hit VM_BUG_ON() when 
btrfs_data_csum_ok() got a csum mismatch and has to call memzero_bvec(), 
which is a single page only helper.

I'm wondering what is the proper handling for mp bvec.


Since we're inside one mp vec, all the pages in the bvec should be 
physically contiguous. But will highmem sneak into a bvec?

If not, the memzero_page()'s check looks a little overkilled.

And if highmem page can sneak in, it means we will need a loop to 
map/zero/unmap...

Thanks,
Qu
>   
> +		ASSERT(bv.bv_len >= sectorsize && IS_ALIGNED(bv.bv_len, sectorsize));
>   		bv.bv_len = min(bv.bv_len, sectorsize);
>   		if (status || !btrfs_data_csum_ok(bbio, dev, offset, &bv))
>   			fbio = repair_one_sector(bbio, offset, &bv, fbio);
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 068339e86123..8b415c780ba8 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -1227,14 +1227,13 @@ int btrfs_decompress_buf2page(const char *buf, u32 buf_len,
>   	cur_offset = decompressed;
>   	/* The main loop to do the copy */
>   	while (cur_offset < decompressed + buf_len) {
> -		struct bio_vec bvec;
> +		struct bio_vec bvec = mp_bvec_iter_bvec(orig_bio->bi_io_vec, orig_bio->bi_iter);
>   		size_t copy_len;
>   		u32 copy_start;
>   		/* Offset inside the full decompressed extent */
>   		u32 bvec_offset;
>   		void *kaddr;
>   
> -		bvec = bio_iter_iovec(orig_bio, orig_bio->bi_iter);
>   		/*
>   		 * cb->start may underflow, but subtracting that value can still
>   		 * give us correct offset inside the full decompressed extent.
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 3ff2bedfb3a4..df48dd6c3f54 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1214,7 +1214,7 @@ static void index_one_bio(struct btrfs_raid_bio *rbio, struct bio *bio)
>   	while (iter.bi_size) {
>   		unsigned int index = (offset >> sectorsize_bits);
>   		struct sector_ptr *sector = &rbio->bio_sectors[index];
> -		struct bio_vec bv = bio_iter_iovec(bio, iter);
> +		struct bio_vec bv = mp_bvec_iter_bvec(bio->bi_io_vec, iter);
>   
>   		sector->has_paddr = true;
>   		sector->paddr = bvec_phys(&bv);