Re: [PATCH v3 8/9] btrfs: prepare btrfs_end_repair_bio() for larger data folios

[Qu Wenruo <wqu@suse.com>]

在 2025/3/17 17:40, Qu Wenruo 写道:
> The function btrfs_end_repair_bio() has an ASSERT() making sure the
> folio is page sized.
> 
> The reason is mostly related to the fact that later we pass a folio and
> its offset into btrfs_repair_io_failure().
> If we have larger folios passed in, later calculation of the folio and
> its offset can go wrong, as we have extra offset to the bv_page.
> 
> Change the behavior by:
> 
> - Doing a proper folio grab
>    Instead of just page_folio(bv_page), we should get the real page (as the
>    bv_offset can be larger than page size), then call page_folio().
> 
> - Do extra folio offset calculation
>    We can have the following cases of a bio_vec (this bv is moved
>    forward by btrfs read verification):
> 
>    bv_page             bv_offset
>    |                   |
>    | | | | | | | | | | | | | | | | |
>    |<-  folio_a  ->|<-  folio_b  ->|
> 
>    | | = a page.
> 
> In above case, the real folio should be folio_b, and offset inside that
> folio should be:
> 
>    bv_offset - ((&folio_b->page - &folio_a->page) << PAGE_SHIFT).

This part is a little confusing and resulted an incorrect calculation 
and ASSERT().

The more accurate (with more corner cases) example to calculate the 
offset would be something like this:

                             real_page
           bv_page           |   bv_offset (10K)
           |                 |   |
           v                 v   v
           |        |        |       |
           |<- F1 ->|<--- Folio 2 -->|
                    | result off |

In that case, we only need to care about one page pointer, the 
@real_page, which is (bv_page + (bv_offset >> PAGE_SHIFT)).

Meanwhile the offset inside the folio 2 is consisted of two part:

- The offset between folio2's head page and real_page
   We have an existing helper, folio_page_idx(), to do the calculation.
   So this part is just (folio_page_idx() << PAGE_SHIFT).

- The offset inside that @real_page
   Since it's now page based, we do not need to bother folio size, just
   offset_in_page() will do the work.

So the resulted value should be:
   (folio_page_idx() << PAGE_SHIFT) + offset_in_page(bv_offset)

Furthermore, the above cases shows that we can not use offset_in_folio() 
either, as the previous folio is no in the same order.
(offset_in_folio() will result 2K, not the correct 6K).

I'll send out an updated series with this fixed, and enable large folios 
for testing.

Thanks,
Qu

> 
> With these changes, now btrfs_end_repair_bio() is able to handle larger
> folios properly.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/bio.c | 28 +++++++++++++++++++++-------
>   1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index 8c2eee1f1878..292c79e0855f 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -156,6 +156,25 @@ static void btrfs_repair_done(struct btrfs_failed_bio *fbio)
>   	}
>   }
>   
> +static struct folio *bio_vec_get_folio(const struct bio_vec *bv)
> +{
> +	return page_folio(bv->bv_page + (bv->bv_offset >> PAGE_SHIFT));
> +}
> +
> +static unsigned long bio_vec_get_folio_offset(const struct bio_vec *bv)
> +{
> +	struct folio *folio = bio_vec_get_folio(bv);
> +
> +	/*
> +	 * There can be multiple physically contiguous folios queued
> +	 * into the bio_vec.
> +	 * Thus the first page of our folio should be at or beyond
> +	 * the first page of the bio_vec.
> +	 */
> +	ASSERT(&folio->page >= bv->bv_page);
> +	return bv->bv_offset - ((&folio->page - bv->bv_page) << PAGE_SHIFT);
> +}
> +
>   static void btrfs_end_repair_bio(struct btrfs_bio *repair_bbio,
>   				 struct btrfs_device *dev)
>   {
> @@ -165,12 +184,6 @@ static void btrfs_end_repair_bio(struct btrfs_bio *repair_bbio,
>   	struct bio_vec *bv = bio_first_bvec_all(&repair_bbio->bio);
>   	int mirror = repair_bbio->mirror_num;
>   
> -	/*
> -	 * We can only trigger this for data bio, which doesn't support larger
> -	 * folios yet.
> -	 */
> -	ASSERT(folio_order(page_folio(bv->bv_page)) == 0);
> -
>   	if (repair_bbio->bio.bi_status ||
>   	    !btrfs_data_csum_ok(repair_bbio, dev, 0, bv)) {
>   		bio_reset(&repair_bbio->bio, NULL, REQ_OP_READ);
> @@ -192,7 +205,8 @@ static void btrfs_end_repair_bio(struct btrfs_bio *repair_bbio,
>   		btrfs_repair_io_failure(fs_info, btrfs_ino(inode),
>   				  repair_bbio->file_offset, fs_info->sectorsize,
>   				  repair_bbio->saved_iter.bi_sector << SECTOR_SHIFT,
> -				  page_folio(bv->bv_page), bv->bv_offset, mirror);
> +				  bio_vec_get_folio(bv), bio_vec_get_folio_offset(bv),
> +				  mirror);
>   	} while (mirror != fbio->bbio->mirror_num);
>   
>   done: