Re: [PATCH v5 2/5] mm/vmscan: extract folio_free() from shrink_folio_list()

[Barry Song]

On Mon, Jul 20, 2026 at 1:08 PM Zhang Peng <zippermonkey@xxxxxxxxxx> wrote:
>
> shrink_folio_list() contains a self-contained folio-freeing section:
> buffer release, lazyfree, __remove_mapping, and folio_batch drain.
> Extract it into folio_free() to reduce the size of shrink_folio_list()
> and make the freeing step independently readable.
>
> No functional change.
>
> Signed-off-by: Zhang Peng <bruzzhang@xxxxxxxxxxx>
> ---
>  mm/vmscan.c | 164 +++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 89 insertions(+), 75 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 5ba880dce21e..a0807dd01c5a 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1076,6 +1076,93 @@ static void folio_activate_locked(struct folio *folio,
>         }
>  }
>
> +static bool folio_try_reclaim_free(struct folio *folio,
> +               struct folio_batch *free_folios,
> +               struct scan_control *sc, struct reclaim_stat *stat,
> +               unsigned int *nr_reclaimed)
> +{
> +       const unsigned int nr_pages = folio_nr_pages(folio);
> +       struct address_space *mapping = folio_mapping(folio);
> +
> +       /*
> +        * If the folio has buffers, try to free the buffer mappings
> +        * associated with this folio. If we succeed we try to free
> +        * the folio as well.
> +        *
> +        * We do this even if the folio is dirty.
> +        * filemap_release_folio() does not perform I/O, but it is
> +        * possible for a folio to have the dirty flag set, but it
> +        * is actually clean (all its buffers are clean).  This
> +        * happens if the buffers were written out directly, with
> +        * submit_bh(). ext3 will do this, as well as the blockdev
> +        * mapping.  filemap_release_folio() will discover that
> +        * cleanness and will drop the buffers and mark the folio
> +        * clean - it can be freed.
> +        *
> +        * Rarely, folios can have buffers and no ->mapping.  These
> +        * are the folios which were not successfully invalidated in
> +        * truncate_cleanup_folio().  We try to drop those buffers
> +        * here and if that worked, and the folio is no longer
> +        * mapped into process address space (refcount == 1) it can
> +        * be freed.  Otherwise, leave the folio on the LRU so it is
> +        * swappable.
> +        */
> +       if (folio_needs_release(folio)) {
> +               if (!filemap_release_folio(folio, sc->gfp_mask)) {
> +                       folio_activate_locked(folio, stat);

Could we avoid hiding the activate semantics inside
folio_try_reclaim_free()? It makes the logic harder to read and
can be confusing.

Could we pull this out so that the three possible outcomes are
explicit?

1. activate
2. keep
3. free

[...]

> -               } else if (!mapping || !__remove_mapping(mapping, folio, true,
> -                                                        sc->target_mem_cgroup))
> +               if (!folio_try_reclaim_free(folio, &free_folios, sc, stat,
> +                                          &nr_reclaimed))
>                         goto keep_locked;

I mean, this is confusing because an activated folio ends up in the
"keep" path. Can we make the activation semantics explicit at the
outer level?

Best Regards
Barry