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.
> Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
> fs/iomap/buffered-io.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index f8bdb2428819..4b173aad04ed 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -352,7 +352,7 @@ static void iomap_read_end_io(struct bio *bio)
>
> struct iomap_readpage_ctx {
> struct folio *cur_folio;
> - bool cur_folio_in_bio;
> + bool folio_unlocked;
Maybe this ought to be called cur_folio_borrowed?
/*
* Folio readahead can transfer ownership of a folio lock to
* an external reader (e.g. bios) with the expectation that
* the new owner will unlock the folio when the readahead is
* complete. Under these circumstances, the readahead context
* is merely borrowing the folio and must not unlock it.
*/
bool cur_folio_borrowed;
> struct bio *bio;
> struct readahead_control *rac;
> };
> @@ -367,7 +367,7 @@ static void iomap_read_folio_range_async(const struct iomap_iter *iter,
> loff_t length = iomap_length(iter);
> sector_t sector;
>
> - ctx->cur_folio_in_bio = true;
> + ctx->folio_unlocked = true;
ctx->cur_folio_borrowed = true;
> if (ifs) {
> spin_lock_irq(&ifs->state_lock);
> ifs->read_bytes_pending += plen;
> @@ -480,9 +480,9 @@ int iomap_read_folio(struct folio *folio, const struct iomap_ops *ops)
>
> if (ctx.bio) {
> submit_bio(ctx.bio);
> - WARN_ON_ONCE(!ctx.cur_folio_in_bio);
> + WARN_ON_ONCE(!ctx.folio_unlocked);
> } else {
> - WARN_ON_ONCE(ctx.cur_folio_in_bio);
> + WARN_ON_ONCE(ctx.folio_unlocked);
> folio_unlock(folio);
> }
>
> @@ -503,13 +503,13 @@ static int iomap_readahead_iter(struct iomap_iter *iter,
> while (iomap_length(iter)) {
> if (ctx->cur_folio &&
> offset_in_folio(ctx->cur_folio, iter->pos) == 0) {
> - if (!ctx->cur_folio_in_bio)
> + if (!ctx->folio_unlocked)
> folio_unlock(ctx->cur_folio);
> ctx->cur_folio = NULL;
> }
> if (!ctx->cur_folio) {
> ctx->cur_folio = readahead_folio(ctx->rac);
> - ctx->cur_folio_in_bio = false;
> + ctx->folio_unlocked = false;
ctx->cur_folio_borrowed = false;
> }
> ret = iomap_readpage_iter(iter, ctx);
> if (ret)
> @@ -552,10 +552,8 @@ void iomap_readahead(struct readahead_control *rac, const struct iomap_ops *ops)
>
> if (ctx.bio)
> submit_bio(ctx.bio);
> - if (ctx.cur_folio) {
> - if (!ctx.cur_folio_in_bio)
> - folio_unlock(ctx.cur_folio);
> - }
> + if (ctx.cur_folio && !ctx.folio_unlocked)
> + folio_unlock(ctx.cur_folio);
if (ctx.cur_folio && !ctx.cur_folio_borrowed)
folio_unlock(ctx.cur_folio);
> }
> EXPORT_SYMBOL_GPL(iomap_readahead);
>
> --
> 2.47.3
>
>
