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

[David Hildenbrand <david@redhat.com>]

On 26.08.24 13:10, Zhaoyang Huang wrote:
> 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.

Okay, but I assume (as stated) this patch does not have an affect on 
that, or am I missing something?

I assume you have some way to test before/after, if you run into similar 
problems in practice.

-- 
Cheers,

David / dhildenb