On Wed, Jan 15, 2025 at 02:46:44PM -0700, Yu Zhao wrote:
> On Wed, Jan 15, 2025 at 2:35 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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);
Okay, makes sense to me.
--
Kiryl Shutsemau / Kirill A. Shutemov
