Proper way to copy de-compressed data into a bio, in folio style?

Proper way to copy de-compressed data into a bio, in folio style?

[Qu Wenruo]

Hi,

The seemingly easy question has some very interesting extra requirements:

1. The bio contains contig file map folios
    The folios may be large.
    So page_offset() on bv_page (using single-page bvec) is no longer
    reliable, one has to call page_pgoff() instead.

2. The data may not cover the bio range
    So we need some range comparison and skip if the data range doesn't
    cover the bio range.

3. The bio may have been advanced
    E.g. previous de-compressed range has been copied, but the remaining
    part still needs to be fulfilled.

    And we need to use the bv_page's file offset to calculate the real
    beginning of the range to copy.

The current btrfs code is doing single page bvec iteration, and handling
point 2 and 3 well.
(btrfs_decompress_buf2page() in fs/btrfs/compression.c)

Point 1 was not causing problem until the incoming large data folio
support, and can be easily fixed with page_pgoff() convertion.


But since we're here, I'm also wondering can we do it better with a
folio or multi-page bvec way?

The current folio bio iteration helper can only start from the beginning
of a bio (bio_for_each_folio_all() and bio_first_folio()), thus it's not
a good fit for point 3.

On the other hand, I'm having some internal code to convert a bio_vec
into a folio and offset inside the folio already.
Thus I'm wondering can we provide something like bio_for_each_folio()?
Or is it too niche that only certain fs can benefit from?

Thanks,
Qu

Re: Proper way to copy de-compressed data into a bio, in folio style?

[Matthew Wilcox]

On Mon, Mar 31, 2025 at 06:45:10PM +1030, Qu Wenruo wrote:
> Hi,
> 
> The seemingly easy question has some very interesting extra requirements:
> 
> 1. The bio contains contig file map folios
>    The folios may be large.
>    So page_offset() on bv_page (using single-page bvec) is no longer
>    reliable, one has to call page_pgoff() instead.

page_offset() is on my hitlist.  It actually is correct now (commit
12851bd921d4) but it's on its way out.  Don't use bv_page.

> 2. The data may not cover the bio range
>    So we need some range comparison and skip if the data range doesn't
>    cover the bio range.

I have no idea what this means.

> 3. The bio may have been advanced
>    E.g. previous de-compressed range has been copied, but the remaining
>    part still needs to be fulfilled.
> 
>    And we need to use the bv_page's file offset to calculate the real
>    beginning of the range to copy.
> 
> The current btrfs code is doing single page bvec iteration, and handling
> point 2 and 3 well.
> (btrfs_decompress_buf2page() in fs/btrfs/compression.c)
> 
> Point 1 was not causing problem until the incoming large data folio
> support, and can be easily fixed with page_pgoff() convertion.
> 
> 
> But since we're here, I'm also wondering can we do it better with a
> folio or multi-page bvec way?
> 
> The current folio bio iteration helper can only start from the beginning
> of a bio (bio_for_each_folio_all() and bio_first_folio()), thus it's not
> a good fit for point 3.
> 
> On the other hand, I'm having some internal code to convert a bio_vec
> into a folio and offset inside the folio already.
> Thus I'm wondering can we provide something like bio_for_each_folio()?
> Or is it too niche that only certain fs can benefit from?

I don't understand your requirements. but doing something different that
fills in a folio_iter along the lines of bio_for_each_folio_all()
would make sense.

Re: Proper way to copy de-compressed data into a bio, in folio style?

[Christoph Hellwig]

On Thu, Apr 03, 2025 at 06:44:22PM +0100, Matthew Wilcox wrote:
> > On the other hand, I'm having some internal code to convert a bio_vec
> > into a folio and offset inside the folio already.
> > Thus I'm wondering can we provide something like bio_for_each_folio()?
> > Or is it too niche that only certain fs can benefit from?
> 
> I don't understand your requirements. but doing something different that
> fills in a folio_iter along the lines of bio_for_each_folio_all()
> would make sense.
>

Looking at btrfs_decompress_buf2page it uses bio_iter_iovec, which is
the building block use by bio_for_each_segment to build segments, that
is bvecs that are constrained to PAGE_SIZE boundaries.  In this for
a good reason as it then wants to copy into them with a single kmap
mapping.

This means that bv_offset is always less than PAGE_SIZE for these
generated bio_vecs.

In the short run I'd suggest to just keep using the existing open-coded
bio_for_each_segment-like loop so that each iteration covers a PAGE_SIZE
granularity chunk.  Just stop messing th the bvec fields directly and
use bvec_kmap_local to map the destination and do a plain memcpy into
that instead of using memcpy_to_page.

In the long run it would be great to just rewrite the low-level
decompressors to work on an iov_iter as output and remove the need
for the extra data copy entirely, but if that can't happen we'll
just need for figure out how we can do useful kmaps larger than
PAGE_SIZE or stop allowing highmem pages for btrfs and then switch to
a folio based iterator.