Re: [PATCH v3 07/15] iomap: track read/readahead folio ownership internally

Joanne Koong
2025-09-19T18:15:06Z

On Thu, Sep 18, 2025 at 2:49 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Sep 16, 2025 at 04:44:17PM -0700, Joanne Koong wrote:
> > The purpose of "struct iomap_read_folio_ctx->cur_folio_in_bio" is to
> > track folio ownership to know who is responsible for unlocking it.
> > Rename "cur_folio_in_bio" to "cur_folio_owned" to better reflect this
> > purpose and so that this can be generically used later on by filesystems
> > that are not block-based.
>
> Hrmm, well if this is becoming a private variable, then I'd shorten it
> to "folio_owned" since it's not really in the ctx/cur anymore, but I
> might be bikeshedding now. :)

Oo I'll do this for ->read_folio (and for the arg name in
iomap_read_folio_iter()) since there's only 1 folio which makes it
obvious which folio it is, but for ->readahead I'll keep it as
"cur_folio_owned" to make it more obvious that the folio owned state
refers to the folio in ctx->cur_folio.

>
> > Since "struct iomap_read_folio_ctx" will be made a public interface
> > later on when read/readahead takes in caller-provided callbacks, track
> > the folio ownership state internally instead of exposing it in "struct
> > iomap_read_folio_ctx" to make the interface simpler for end users.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> > fs/iomap/buffered-io.c | 34 +++++++++++++++++++++++-----------
> > 1 file changed, 23 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index 6c5a631848b7..587bbdbd24bc 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -352,7 +352,6 @@ static void iomap_read_end_io(struct bio *bio)
> >
> > struct iomap_read_folio_ctx {
> > struct folio *cur_folio;
> > - bool cur_folio_in_bio;
> > void *read_ctx;
> > struct readahead_control *rac;
> > };
> > @@ -376,7 +375,6 @@ static void iomap_bio_read_folio_range(const struct iomap_iter *iter,
> > sector_t sector;
> > struct bio *bio = ctx->read_ctx;
> >
> > - ctx->cur_folio_in_bio = true;
> > if (ifs) {
> > spin_lock_irq(&ifs->state_lock);
> > ifs->read_bytes_pending += plen;
> > @@ -413,7 +411,7 @@ static void iomap_bio_read_folio_range(const struct iomap_iter *iter,
> > }
> >
> > static int iomap_read_folio_iter(struct iomap_iter *iter,
> > - struct iomap_read_folio_ctx *ctx)
> > + struct iomap_read_folio_ctx *ctx, bool *cur_folio_owned)
> > {
> > const struct iomap *iomap = &iter->iomap;
> > loff_t pos = iter->pos;
> > @@ -450,6 +448,7 @@ static int iomap_read_folio_iter(struct iomap_iter *iter,
> > folio_zero_range(folio, poff, plen);
> > iomap_set_range_uptodate(folio, poff, plen);
> > } else {
> > + *cur_folio_owned = true;
> > iomap_bio_read_folio_range(iter, ctx, pos, plen);
> > }
> >
> > @@ -472,16 +471,22 @@ int iomap_read_folio(struct folio *folio, const struct iomap_ops *ops)
> > struct iomap_read_folio_ctx ctx = {
> > .cur_folio = folio,
> > };
> > + /*
> > + * If an external IO helper takes ownership of the folio, it is
> > + * responsible for unlocking it when the read completes.
>
> Not sure what "external" means here -- I think for your project it means
> a custom folio read method supplied by a filesystem, but for the exist
> code (xfs submitting unaltered bios), that part is still mostly internal
> to iomap.
>
> If we were *only* refactoring code I would suggest s/external/async/
> because that's what the bio code does, but a filesystem supplying its
> own folio read function could very well fill the folio synchronously and
> it'd still have to unlock the folio.
>
> Maybe just get rid of the word external? The rest of the code changes
> look fine to me.

Sounds good, I'll get rid of "external".

Thanks for reviewing this patchset!
>
> --D
>