On Wed, Nov 23, 2022 at 5:02 AM David Howells <dhowells@xxxxxxxxxx> 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
