On Fri, 15 Sept 2023 at 11:37, Matthew Wilcox (Oracle)
<willy@xxxxxxxxxxxxx> wrote:
>
> Perform one atomic operation (acquiring the spinlock) instead of
> two (spinlock & atomic_sub) per read completion.
I think this may be a worthwhile thing to do, but...
> -static void iomap_finish_folio_read(struct folio *folio, size_t offset,
> +static void iomap_finish_folio_read(struct folio *folio, size_t off,
this function really turns into a mess.
The diff is hard to read, and I'm not talking about the 'offset' ->
'off' part, but about how now about half of the function has various
'if (ifs)' tests spread out.
And I think it actually hides what is going on.
If you decide to combine all the "if (ifs)" parts on one side, and
then simplify the end result, you actually end up with a much
easier-to-read function.
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 think the above looks a *lot* more legible than having three
different versions of "if (ifs)" spread out in the function, and it
also makes the parts that are actually protected by ifs->state_lock a
lot more obvious.
But again: I looked at your patch, found it very hard to follow, and
then decided to quickly do a "what happens if I apply the patch and
then try to simplify the result".
I might have made some simplification error. But please give that a look, ok?
Linus
