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

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

[Yu Zhao]

On Wed, Jan 15, 2025 at 2:32 AM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> The recently introduced PG_dropbehind allows for freeing folios
> immediately after writeback. Unlike PG_reclaim, it does not need vmscan
> to be involved to get the folio freed.
>
> The new flag allows to replace whole deactivate_file_folio() machinery
> with simple folio_set_dropbehind().
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Acked-by: Yu Zhao <yuzhao@google.com>

Re: [PATCHv2 09/11] mm: Remove PG_reclaim

[Yu Zhao]

On Wed, Jan 15, 2025 at 2:32 AM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> Nobody sets the flag anymore.
>
> Remove the PG_reclaim, making PG_readhead exclusive user of the page
> flag bit.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Acked-by: Yu Zhao <yuzhao@google.com>

Re: [PATCHv2 11/11] mm: Rename PG_dropbehind to PG_reclaim

[Yu Zhao]

On Wed, Jan 15, 2025 at 2:32 AM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> Now as PG_reclaim is gone, its name can be reclaimed for better
> use :)
>
> Rename PG_dropbehind to PG_reclaim and rename all helpers around it.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Acked-by: Yu Zhao <yuzhao@google.com>

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

[Matthew Wilcox]

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.

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

[Yu Zhao]

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);

Re: [PATCHv2 11/11] mm: Rename PG_dropbehind to PG_reclaim

[Christoph Hellwig]

On Wed, Jan 15, 2025 at 11:31:35AM +0200, Kirill A. Shutemov wrote:
> Now as PG_reclaim is gone, its name can be reclaimed for better
> use :)
> 
> Rename PG_dropbehind to PG_reclaim and rename all helpers around it.

Why?  reclaim is completely generic and reclaim can mean many
different things.  dropbehind is much more specific.

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

[Kirill A. Shutemov]

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

Re: [PATCHv2 11/11] mm: Rename PG_dropbehind to PG_reclaim

[Kirill A. Shutemov]

On Wed, Jan 15, 2025 at 10:18:16PM -0800, Christoph Hellwig wrote:
> On Wed, Jan 15, 2025 at 11:31:35AM +0200, Kirill A. Shutemov wrote:
> > Now as PG_reclaim is gone, its name can be reclaimed for better
> > use :)
> > 
> > Rename PG_dropbehind to PG_reclaim and rename all helpers around it.
> 
> Why?  reclaim is completely generic and reclaim can mean many
> different things.  dropbehind is much more specific.

Dropbehind is somewhat obscure name. You need fair bit of context to
understand what it does.

But I don't care that much. We can keep it as PG_dropbehind.

Anybody else has opinion on this?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov