Re: [GIT PULL] Folio fixes for 5.19

[Linus Torvalds]

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