Re: [PATCHv2 05/11] mm/truncate: Use folio_set_dropbehind() instead of deactivate_file_folio()k

["Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>]

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@infradead.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);

Okay, makes sense to me.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov