Re: [PATCH 02/17] iomap: Protect read_bytes_pending with the state_lock

Matthew Wilcox
2023-09-16T15:51:45Z

On Fri, Sep 15, 2023 at 05:11:55PM -0700, Linus Torvalds wrote:
> I think it ends up looking like this:
>
> static void iomap_finish_folio_read(struct folio *folio, size_t off,
> size_t len, int error)
> {
> struct iomap_folio_state *ifs = folio->private;
> bool uptodate = true;
> bool finished = true;
>
> if (ifs) {
> unsigned long flags;
>
> spin_lock_irqsave(&ifs->state_lock, flags);
>
> if (!error)
> uptodate = ifs_set_range_uptodate(folio, ifs,
> off, len);
>
> ifs->read_bytes_pending -= len;
> finished = !ifs->read_bytes_pending;
> spin_unlock_irqrestore(&ifs->state_lock, flags);
> }
>
> if (unlikely(error))
> folio_set_error(folio);
> else if (uptodate)
> folio_mark_uptodate(folio);
> if (finished)
> folio_unlock(folio);
> }
>
> but that was just a quick hack-work by me (the above does, for
> example, depend on folio_mark_uptodate() not needing the
> ifs->state_lock, so the shared parts then got moved out).

I really like this. One tweak compared to your version:

bool uptodate = !error;
...
if (error)
folio_set_error(folio);
if (uptodate)
folio_mark_uptodate(folio);
if (finished)
folio_unlock(folio);

... and then the later patch becomes

if (finished)
folio_end_read(folio, uptodate);