On Fri, Sep 12, 2025 at 10:30 AM Joanne Koong [off-list ref] wrote:

On Thu, Sep 11, 2025 at 7:31 AM Christoph Hellwig [off-list ref] wrote:

quoted

quoted

+static void __iomap_finish_folio_read(struct folio *folio, size_t off,
+             size_t len, int error, bool update_bitmap)
 {
      struct iomap_folio_state *ifs = folio->private;
      bool uptodate = !error;

@@ -340,7 +340,7 @@ void iomap_finish_folio_read(struct folio *folio, size_t off, size_t len,
              unsigned long flags;

              spin_lock_irqsave(&ifs->state_lock, flags);
-             if (!error)
+             if (!error && update_bitmap)
                      uptodate = ifs_set_range_uptodate(folio, ifs, off, len);

This code sharing keeps confusing me a bit.  I think it's technically
perfectly fine, but not helpful for readability.  We'd solve that by
open coding the !update_bitmap case in iomap_read_folio_iter.  Which
would also allow to use spin_lock_irq instead of spin_lock_irqsave there
as a nice little micro-optimization.  If we'd then also get rid of the
error return from ->read_folio_range and always do asynchronous error
returns it would be even simpler.

Or maybe I just need to live with the magic bitmap update, but the
fact that "len" sometimes is an actual length, and sometimes just a
counter for read_bytes_pending keeps confusing me

I think you're right, this is probably clearer without trying to share
the function.

I think maybe we can make this even simpler. Right now we mark the
bitmap uptodate every time a range is read in but I think instead we
can just do one bitmap uptodate operation for the entire folio when
the read has completely finished.  If we do this, then we can make
"ifs->read_bytes_pending" back to an atomic_t since we don't save one
atomic operation from doing it through a spinlock anymore (eg what
commit f45b494e2a "iomap: protect read_bytes_pending with the
state_lock" optimized). And then this bias thing can just become:

if (ifs) {
    if (atomic_dec_and_test(&ifs->read_bytes_pending))
        folio_end_read(folio, !ret);
    *cur_folio_owned = true;
}

This idea doesn't work unfortunately because reading in a range might fail.

I'll change this to open coding the !update_bitmap case with
spin_lock_irq, like Christoph suggested.

Thanks,
Joanne