On Fri, Jun 10, 2022 at 12:22 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> - Don't release a folio while it's still locked
Ugh.
I *hate* this patch. It's just incredibly broken.
Yes, I've pulled this, but I have looked at that readahead_folio()
function before, and I have despised it before, but this patch really
drove home how incredibly broken that function is.
Honestly, readahead_folio() returns a folio *AFTER* it has dropped the
ref to that folio.
That's broken to start with. It's not only really wrong to say "here's
a pointer, and by the way, you don't actually hold a ref to it any
more".
It's doubly broken because it's *very* different from the similarly
named readahead_page() that doesn't have that fundamental interface
bug.
But it's now *extra* broken now that you realize that the dropping of
the ref was very very wrong, so you re-get the ref. WTF?
As far as I can tell, ALL THE OTHER users of this broken function fall
into two categories:
(a) they are broken (see the exact same broken pattern in
fs/erofs/fscache.c, for example)
(b) they only use the thing as a boolean
Anyway, I've pulled this, but I really seriously object to that
completely mis-designed function.
Please send me a follow-up patch that fixes it by just making the
*callers* drop the refcount, instead of doing it incorrectly inside
readahead_folio().
Alternatively, make "readahead_folio()" just return a boolean - so
that the (b) case situation can continue the use this function - and
create a new function that just exposes __readahead_folio() without
this broken "drop refcount" behavior).
Or explain why that "drop and retake ref" isn't
(a) completely broken and racy
(b) inefficient
(c) returning a non-ref'd folio pointer is horribly broken interface
to begin with
because right now it's just disgusting in so many ways and this bug is
just the last drop for me.
Linus
