Re: [RFC PATCH 1/1] mm: Skip folio with private data during isolation

[Zhaoyang Huang <huangzhaoyang@gmail.com>]

On Mon, Aug 26, 2024 at 6:36 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 26.08.24 10:50, zhaoyang.huang wrote:
> > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> >
> > Since clean target folio with private data will be given up finally in
> > __remove_mapping as it has extra refcnt, it is better to skip it during
> > isolation to save the slot for more qualified folio. Current one could
> > be the candidate for next round of scanning after the private data gone.
> >
> > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > ---
> >   mm/vmscan.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index cfa839284b92..755bf3a387f3 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -1685,6 +1685,8 @@ static unsigned long isolate_lru_folios(unsigned long nr_to_scan,
> >                */
> >               scan += nr_pages;
> >
> > +             if (folio_test_private(folio) && !folio_test_dirty(folio))
> > +                     goto move;
> >               if (!folio_test_lru(folio))
> >                       goto move;
> >               if (!sc->may_unmap && folio_mapped(folio))
>
> An earlier filemap_release_folio() would have failed if the private data
> (buffers) cannot get freed, and we went into the activate_locked path.
>
>
> if (folio_needs_release(folio)) {
>         if (!filemap_release_folio(folio, sc->gfp_mask)
>                 goto activate_locked;
> ...
>
> if (folio_test_anon(folio) && !folio_test_swapbacked(folio)) {
>         ...
> } else if (!mapping || !__remove_mapping(mapping, folio, true,
> }
>
> At least on the shrink_folio_list() path, I'm not sure the code you are
> adding could even trigger. We should not reach __remove_mapping() with
> folio_test_private().
Thanks for heads up. You are right, the bh is judged if existing
before __remove_mapping. ASAIU, the metadata associated with the bh
has risk to be freed such as journal data etc or it introduces extra
IO. Actually, this patch is inspired by a practical problem we just
run across which the bh remains on LRU for a long time since it is
attached to a journal_head that can not be freed by jbd2.
>
> --
> Cheers,
>
> David / dhildenb
>