Re: [PATCH v2 13/16] iomap: move read/readahead logic out of CONFIG_BLOCK guard

[Gao Xiang <hsiangkao@linux.alibaba.com>]

On 2025/9/9 23:33, Joanne Koong wrote:
> On Mon, Sep 8, 2025 at 10:14 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>>
>> On 2025/9/9 02:51, Joanne Koong wrote:
>>> There is no longer a dependency on CONFIG_BLOCK in the iomap read and
>>> readahead logic. Move this logic out of the CONFIG_BLOCK guard. This
>>> allows non-block-based filesystems to use iomap for reads/readahead.
>>>
>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>>> ---
>>>    fs/iomap/buffered-io.c | 151 +++++++++++++++++++++--------------------
>>>    1 file changed, 76 insertions(+), 75 deletions(-)
>>>
>>> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
>>> index f673e03f4ffb..c424e8c157dd 100644
>>> --- a/fs/iomap/buffered-io.c
>>> +++ b/fs/iomap/buffered-io.c
>>> @@ -358,81 +358,6 @@ void iomap_finish_folio_read(struct folio *folio, size_t off, size_t len,
>>>    }
>>> +
>>> +/**
>>> + * Read in a folio range asynchronously through bios.
>>> + *
>>> + * This should only be used for read/readahead, not for buffered writes.
>>> + * Buffered writes must read in the folio synchronously.
>>> + */
>>> +static int iomap_read_folio_range_bio_async(const struct iomap_iter *iter,
>>> +             struct iomap_read_folio_ctx *ctx, loff_t pos, size_t plen)
>>> +{
>>> +     struct folio *folio = ctx->cur_folio;
>>> +     const struct iomap *iomap = &iter->iomap;
>>> +     size_t poff = offset_in_folio(folio, pos);
>>> +     loff_t length = iomap_length(iter);
>>> +     sector_t sector;
>>> +     struct bio *bio = ctx->private;
>>> +
>>> +     iomap_start_folio_read(folio, plen);
>>> +
>>> +     sector = iomap_sector(iomap, pos);
>>> +     if (!bio || bio_end_sector(bio) != sector ||
>>> +         !bio_add_folio(bio, folio, plen, poff)) {
>>> +             gfp_t gfp = mapping_gfp_constraint(folio->mapping, GFP_KERNEL);
>>> +             gfp_t orig_gfp = gfp;
>>> +             unsigned int nr_vecs = DIV_ROUND_UP(length, PAGE_SIZE);
>>> +
>>> +             if (bio)
>>> +                     submit_bio(bio);
>>> +
>>> +             if (ctx->rac) /* same as readahead_gfp_mask */
>>> +                     gfp |= __GFP_NORETRY | __GFP_NOWARN;
>>> +             bio = bio_alloc(iomap->bdev, bio_max_segs(nr_vecs),
>>> +                                  REQ_OP_READ, gfp);
>>> +             /*
>>> +              * If the bio_alloc fails, try it again for a single page to
>>> +              * avoid having to deal with partial page reads.  This emulates
>>> +              * what do_mpage_read_folio does.
>>> +              */
>>> +             if (!bio)
>>> +                     bio = bio_alloc(iomap->bdev, 1, REQ_OP_READ, orig_gfp);
>>> +             if (ctx->rac)
>>> +                     bio->bi_opf |= REQ_RAHEAD;
>>> +             bio->bi_iter.bi_sector = sector;
>>> +             bio->bi_end_io = iomap_read_end_io;
>>> +             bio_add_folio_nofail(bio, folio, plen, poff);
>>> +             ctx->private = bio;
>>
>> Yes, I understand some way is needed to isolate bio from non-bio
>> based filesystems, and I also agree `bio` shouldn't be stashed
>> into `iter->private` since it's just an abuse usage as mentioned
>> in:
>> https://lore.kernel.org/r/20250903203031.GM1587915@frogsfrogsfrogs
>> https://lore.kernel.org/r/aLkskcgl3Z91oIVB@infradead.org
>>
>> However, the naming of `(struct iomap_read_folio_ctx)->private`
>> really makes me feel confused because the `private` name in
>> `read_folio_ctx` is much like a filesystem read context instead
>> of just be used as `bio` internally in iomap for block-based
>> filesystems.
>>
>> also the existing of `iter->private` makes the naming of
>> `ctx->private` more confusing at least in my view.
> 
> Do you think "ctx->data" would be better? Or is there something else
> you had in mind?

At least it sounds better on my side, but anyway it's just
my own overall thought.  If other folks have different idea,
I don't have strong opinion, I just need something for my own
as previous said.

Thanks,
Gao Xiang

> 
> Thanks,
> Joanne
>>
>> Thanks,
>> Gao Xiang