Re: [PATCH v1 02/16] iomap: rename cur_folio_in_bio to folio_unlockedOM

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Sep 04, 2025 at 03:06:52PM -0700, Joanne Koong wrote:
> On Wed, Sep 3, 2025 at 11:03 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Wed, Sep 03, 2025 at 01:26:37PM -0700, Darrick J. Wong wrote:
> > > On Fri, Aug 29, 2025 at 04:56:13PM -0700, Joanne Koong wrote:
> > > > The purpose of struct iomap_readpage_ctx's cur_folio_in_bio is to track
> > > > if the folio needs to be unlocked or not. Rename this to folio_unlocked
> > > > to make the purpose more clear and so that when iomap read/readahead
> > > > logic is made generic, the name also makes sense for filesystems that
> > > > don't use bios.
> > >
> > > Hrmmm.  The problem is, "cur_folio_in_bio" captures the meaning that the
> > > (locked) folio is attached to the bio, so the bio_io_end function has to
> > > unlock the folio.  The readahead context is basically borrowing the
> > > folio and cannot unlock the folio itelf.
> > >
> > > The name folio_unlocked doesn't capture the change in ownership, it just
> > > fixates on the lock state which (imo) is a side effect of the folio lock
> > > ownership.
> >
> > Agreed.  Not sure what a good name is in a world where the folio can be
> > in something else than the bio.  Maybe just replace bio with ctx or
> > similar? cur_folio_in_ctx?  cur_folio_locked_by_ctx?
> 
> I find the ctx naming to be more confusing, the "ctx" imo is too
> easily confused with the iomap_readfolio_ctx struct.
> 
> What about "cur_folio_owned" or "cur_folio_handled"? Or keeping it as
> "cur_folio_unlocked" and adding a comment to explain the change in
> ownership?

folio_owned_by_ctx?
or maybe just folio_owned?

Leaving a comment would solve most of my confusion, I think.  Something
like this?

	/*
	 * Is the folio owned by this readpage context, or by some
	 * external IO helper?  Either way, the owner of the folio is
	 * responsible for unlocking it when the read completes.
	 */
	bool folio_owned;

--D

> >
> > > > +   bool                    folio_unlocked;
> > >
> > > Maybe this ought to be called cur_folio_borrowed?
> >
> > I don't think 'borrow' makes much sense here.  It's not like we're
> > borrowing it, we transfer the lock context to the bio (or whatever else
> > Joanne is going to use for fuse, I haven't read down to that yet).
>