Messages in this thread | | | From | Linus Torvalds <> | Date | Fri, 10 Jun 2022 12:56:48 -0700 | Subject | Re: [GIT PULL] Folio fixes for 5.19 |
| |
On Fri, Jun 10, 2022 at 12:22 PM Matthew Wilcox <willy@infradead.org> 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
| |