Re: [PATCH 4/8] mm/memory-failure: Convert shake_page() to shake_folio()

[Jane Chu <jane.chu@oracle.com>]

On 4/8/2024 8:36 AM, Matthew Wilcox wrote:

> On Wed, Mar 06, 2024 at 05:31:19PM +0800, Miaohe Lin wrote:
>>> -void shake_page(struct page *p)
>>> +void shake_folio(struct folio *folio)
>> It might not be a good idea to convert shake_page() into shake_folio(). shake_page() can
>> be called without holding page refcnt. So the below race could happen:
>>
>>   hwpoison_inject
>>    folio = page_folio(p); --> assume p is 4k page [1]
>>    shake_folio(folio)
>>     folio_test_slab(folio)
>>      test_bit(PG_slab, folio_flags(folio, FOLIO_PF_NO_TAIL) [2]
>>       VM_BUG_ON_PGFLAGS(PageTail(page), page) in folio_flags
>>
>> Between [1] and [2], page can become PageTail of a THP. So the VM_BUG_ON_PGFLAGS will trigger.
>> Or am I miss something?
> No, you're not missing anything.  This race can happen.  However, I've
> removed the VM_BUG_ON for folio_test_slab() with "mm: free up PG_slab".
> Now it goes through:
>
> static inline bool PageSlab(const struct page *page)
> {
>          return folio_test_slab(page_folio(page));
> }
>
> static __always_inline bool folio_test_##fname(const struct folio *folio)\
> {                                                                       \
>          return folio_test_type(folio, PG_##lname);                      \
> }                                                                       \
>
> #define folio_test_type(folio, flag)                                    \
>          ((folio->page.page_type & (PAGE_TYPE_BASE | flag)) == PAGE_TYPE_BASE)
>
> which has no assertion that the folio is not a tail page.  Maybe it
> should, but until then we'll silently get the wrong result ;-)
>
In this case (4k page -> THP), the act of drain will be triggered with 
perhaps nothing to do, so looks like a harmless 'wrong result' to me.

thanks,

-jane