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

[Qu Wenruo <wqu@suse.com>]

在 2025/4/7 15:48, Christoph Hellwig 写道:
> On Mon, Mar 17, 2025 at 05:40:53PM +1030, Qu Wenruo wrote:
>> 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).
>>
>> With these changes, now btrfs_end_repair_bio() is able to handle larger
>> folios properly.
> 
> Please stop messing with internals like bv_offset and bv_page entirely
> if you can, as that makes the pending conversion of the bio_vec to store
> a physical address much harder.

Well, I know this version is bad and it is already causing bugs when I 
enabled large folios.

I have a better solution in the latest version to calculate the offset 
inside the folio, but not sure if it still counts as messing with internals:

	struct page *real_page = bv->bv_page +
				 (bv->bv_offset >> PAGE_SHIFT);
	struct folio *folio = page_folio(real_page);

	return (folio_page_idx(folio, real_page) << PAGE_SHIFT) +
		offset_in_page(bv->bv_offset);

The latest version can be found here just for reference, with the corner 
case taken into consideration.

https://lore.kernel.org/linux-btrfs/f679cbeedbb0132cc657b4db7a1d3d70ff2261f0.1744005845.git.wqu@suse.com/T/#u

> 
> Looking at the code the best way to handle this would be to simply
> split btrfs_repair_io_failure so that there is a helper for the code
> before the bio_init call.  btrfs_repair_eb_io_failure (or a new helper
> called by it) then open codes the existing logic using bio_add_folio
> (it could in fact use bio_add_folio_nofail instead), while
> btrfs_end_repair_bio should just copy the existing bio_vec over
> using an assignment or mempcy.

I guess "btrfs_repair_eb_io_failure" here is just a typo, as we're only 
talking about data read-repair.

Mind to give some pseudo code example? As I still didn't catch all the 
points.

There are quite some direct bv_page/bv_len/bv_offset usage inside 
fs/btrfs/bio.c, and to my uneducated eyes it looks like we will need a 
way to convert bio_vec to folio+offset anyway, as long as we're still 
using bio_vec as btrfs_bio::saved_iter.

Thanks,
Qu