linux-kernel - Re: [PATCHv2 05/11] mm/truncate: Use folio_set_dropbehind() instead of deactivate_file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAOUHufZ42ZV1SU+rGLM-2j2Hp43Q9eLY_yFYg8jsifOfcPHUgQ@mail.gmail.com>
Date: Wed, 15 Jan 2025 14:46:44 -0700
From: Yu Zhao <yuzhao@...gle.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>, 
	Andrew Morton <akpm@...ux-foundation.org>, Jens Axboe <axboe@...nel.dk>, 
	"Jason A. Donenfeld" <Jason@...c4.com>, Andi Shyti <andi.shyti@...ux.intel.com>, 
	Chengming Zhou <chengming.zhou@...ux.dev>, Christian Brauner <brauner@...nel.org>, 
	Christophe Leroy <christophe.leroy@...roup.eu>, Dan Carpenter <dan.carpenter@...aro.org>, 
	David Airlie <airlied@...il.com>, David Hildenbrand <david@...hat.com>, Hao Ge <gehao@...inos.cn>, 
	Jani Nikula <jani.nikula@...ux.intel.com>, Johannes Weiner <hannes@...xchg.org>, 
	Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>, Josef Bacik <josef@...icpanda.com>, 
	Masami Hiramatsu <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, 
	Miklos Szeredi <miklos@...redi.hu>, Nhat Pham <nphamcs@...il.com>, 
	Oscar Salvador <osalvador@...e.de>, Ran Xiaokai <ran.xiaokai@....com.cn>, 
	Rodrigo Vivi <rodrigo.vivi@...el.com>, Simona Vetter <simona@...ll.ch>, 
	Steven Rostedt <rostedt@...dmis.org>, Tvrtko Ursulin <tursulin@...ulin.net>, 
	Vlastimil Babka <vbabka@...e.cz>, Yosry Ahmed <yosryahmed@...gle.com>, intel-gfx@...ts.freedesktop.org, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org, 
	linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCHv2 05/11] mm/truncate: Use folio_set_dropbehind() instead
 of deactivate_file_folio()

On Wed, Jan 15, 2025 at 2:35 PM Matthew Wilcox <willy@...radead.org> wrote:
>
> On Wed, Jan 15, 2025 at 11:31:29AM +0200, Kirill A. Shutemov wrote:
> > -static void lru_deactivate_file(struct lruvec *lruvec, struct folio *folio)
> > -{
> > -     bool active = folio_test_active(folio) || lru_gen_enabled();
> > -     long nr_pages = folio_nr_pages(folio);
> > -
> > -     if (folio_test_unevictable(folio))
> > -             return;
> > -
> > -     /* Some processes are using the folio */
> > -     if (folio_mapped(folio))
> > -             return;
> > -
> > -     lruvec_del_folio(lruvec, folio);
> > -     folio_clear_active(folio);
> > -     folio_clear_referenced(folio);
> > -
> > -     if (folio_test_writeback(folio) || folio_test_dirty(folio)) {
> > -             /*
> > -              * Setting the reclaim flag could race with
> > -              * folio_end_writeback() and confuse readahead.  But the
> > -              * race window is _really_ small and  it's not a critical
> > -              * problem.
> > -              */
> > -             lruvec_add_folio(lruvec, folio);
> > -             folio_set_reclaim(folio);
> > -     } else {
> > -             /*
> > -              * The folio's writeback ended while it was in the batch.
> > -              * We move that folio to the tail of the inactive list.
> > -              */
> > -             lruvec_add_folio_tail(lruvec, folio);
> > -             __count_vm_events(PGROTATED, nr_pages);
> > -     }
> > -
> > -     if (active) {
> > -             __count_vm_events(PGDEACTIVATE, nr_pages);
> > -             __count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE,
> > -                                  nr_pages);
> > -     }
> > -}
>
> > +++ b/mm/truncate.c
> > @@ -486,7 +486,7 @@ unsigned long mapping_try_invalidate(struct address_space *mapping,
> >                        * of interest and try to speed up its reclaim.
> >                        */
> >                       if (!ret) {
> > -                             deactivate_file_folio(folio);
> > +                             folio_set_dropbehind(folio);
>
> brr.
>
> This is a fairly substantial change in semantics, and maybe it's fine.
>
> At a high level, we're trying to remove pages from an inode that aren't
> in use.  But we might find that some of them are in use (eg they're
> mapped or under writeback).  If they are mapped, we don't currently
> try to accelerate their reclaim, but now we're going to mark them
> as dropbehind.  I think that's wrong.
>
> If they're dirty or under writeback, then yes, mark them as dropbehind, but
> I think we need to be a little more surgical here.  Maybe preserve the
> unevictable check too.

Right -- deactivate_file_folio() does make sure the folio is not
unevictable or mapped. So probably something like below would the
change in semantics be close enough?

  if (!folio_test_unevictable(folio) && !folio_mapped(folio))
    folio_set_dropbehind(folio);