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@xxxxxxxxxxxxx> 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). >
