Messages in this thread | | | From | Linus Torvalds <> | Date | Wed, 23 Nov 2022 10:26:36 -0800 | Subject | Re: [PATCH v3] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache |
| |
On Wed, Nov 23, 2022 at 5:02 AM David Howells <dhowells@redhat.com> wrote: > > Is the attached patch too heavy to be applied this late in the merge cycle? > Or would you prefer it to wait for the merge window?
This patch is much too much for this point in the release.
But I also think it's strange in another way, with that odd placement of
mapping_clear_release_always(inode->i_mapping);
at inode eviction time. That just feels very random.
Similarly, that change to shrink_folio_list() looks strange, with the nasty folio_needs_release() helper. It seems entirely pointless, with the use then being
if (folio_needs_release(folio)) { if (!filemap_release_folio(folio, sc->gfp_mask)) goto activate_locked;
when everybody else is just using filemap_release_folio() and checking its return value. I like how you changed other cases of
if (folio_has_private(folio) && !filemap_release_folio(folio, 0)) return 0;
to just use "!filemap_release_folio()" directly, and that felt like a cleanup, but the shrink_folio_list() changes look like a step backwards.
And the change to mm/filemap.c is completely unacceptable in all forms, and this added test
+ if ((!mapping || !mapping_release_always(mapping)) && + !folio_test_private(folio) && + !folio_test_private_2(folio)) + return true;
will not be accepted even during the merge window. That code makes no sense what-so-ever, and is in no way acceptable.
That code makes no sense what-so-ever. Why isn't it using "folio_has_private()"? Why is it using it's own illegible version of that that doesn't match any other case? Why is this done as an open-coded - and *badly* so - version of !folio_needs_release() that you for some reason made private to mm/vmscan.c?
So no, this patch is too ugly to apply as-is *ever*, much less during the late rc series.
Linus
| |