On Thu, Sep 11, 2025 at 7:31 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>
> > +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;
}
Thanks,
Joanne
