Re: [PATCH v4 4/4] selftests/mm: add tests for HWPOISON hugetlbfs read

[Sidhartha Kumar <sidhartha.kumar@oracle.com>]

On 1/11/24 10:30 AM, Jiaqi Yan wrote:
> On Thu, Jan 11, 2024 at 10:11 AM Sidhartha Kumar
> <sidhartha.kumar@oracle.com> wrote:
>>
>> On 1/11/24 10:03 AM, Matthew Wilcox wrote:
>>> On Thu, Jan 11, 2024 at 09:51:47AM -0800, Sidhartha Kumar wrote:
>>>> On 1/11/24 9:34 AM, Jiaqi Yan wrote:
>>>>>> -                       if (!folio_test_has_hwpoisoned(folio))
>>>>>> +                       if (!folio_test_hwpoison(folio))
>>>>>
>>>>> Sidhartha, just curious why this change is needed? Does
>>>>> PageHasHWPoisoned change after commit
>>>>> "a08c7193e4f18dc8508f2d07d0de2c5b94cb39a3"?
>>>>
>>>> No its not an issue PageHasHWPoisoned(), the original code is testing for
>>>> the wrong flag and I realized that has_hwpoison and hwpoison are two
>>>> different flags. The memory-failure code calls folio_test_set_hwpoison() to
>>>> set the hwpoison flag and does not set the has_hwpoison flag. When
>>>> debugging, I realized this if statement was never true despite the code
>>>> hitting folio_test_set_hwpoison(). Now we are testing the correct flag.
>>>>
>>>>   From page-flags.h
>>>>
>>>> #ifdef CONFIG_MEMORY_FAILURE
>>>>       PG_hwpoison,            /* hardware poisoned page. Don't touch */
>>>> #endif
>>>>
>>>> folio_test_hwpoison() checks this flag ^^^
>>>>
>>>> /* At least one page in this folio has the hwpoison flag set */
>>>> PG_has_hwpoisoned = PG_error,
>>>>
>>>> while folio_test_has_hwpoisoned() checks this flag ^^^
>>>
>>> So what you're saying is that hugetlb behaves differently from THP
>>> with how memory-failure sets the flags?
>>
>> I think so, in memory_failure() THP goes through this path:
>>
>>          hpage = compound_head(p);
>>          if (PageTransHuge(hpage)) {
>>                  /*
>>                   * The flag must be set after the refcount is bumped
>>                   * otherwise it may race with THP split.
>>                   * And the flag can't be set in get_hwpoison_page() since
>>                   * it is called by soft offline too and it is just called
>>                   * for !MF_COUNT_INCREASED.  So here seems to be the best
>>                   * place.
>>                   *
>>                   * Don't need care about the above error handling paths for
>>                   * get_hwpoison_page() since they handle either free page
>>                   * or unhandlable page.  The refcount is bumped iff the
>>                   * page is a valid handlable page.
>>                   */
>>                  SetPageHasHWPoisoned(hpage);
>>
>> which sets has_hwpoisoned flag while hugetlb goes through
>> folio_set_hugetlb_hwpoison() which calls folio_test_set_hwpoison().
> 
> Yes, hugetlb sets HWPoison flag as the whole hugepage is poisoned once
> a raw page is poisoned. It can't split to make other supages still
> available as THP. This "Improve hugetlbfs read on HWPOISON hugepages"
> patchset only improves fs case as splitting is not needed.
> 
> I found commit a08c7193e4f18 ("mm/filemap: remove hugetlb special
> casing in filemap.c") has the following changes in inode.c:
> 
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -334,7 +334,7 @@ static ssize_t hugetlbfs_read_iter(struct kiocb
> *iocb, struct iov_iter *to)
>          ssize_t retval = 0;
> 
>          while (iov_iter_count(to)) {
> -               struct page *page;
> +               struct folio *folio;
>                  size_t nr, copied, want;
> 
>                  /* nr is the maximum number of bytes to copy from this page */
> @@ -352,18 +352,18 @@ static ssize_t hugetlbfs_read_iter(struct kiocb
> *iocb, struct iov_iter *to)
>                  }
>                  nr = nr - offset;
> 
> 
>                  /* nr is the maximum number of bytes to copy from this page */
> @@ -352,18 +352,18 @@ static ssize_t hugetlbfs_read_iter(struct kiocb
> *iocb, struct iov_iter *to)
>                  }
>                  nr = nr - offset;
> 
> -               /* Find the page */
> -               page = find_lock_page(mapping, index);
> -               if (unlikely(page == NULL)) {
> +               /* Find the folio */
> +               folio = filemap_lock_hugetlb_folio(h, mapping, index);
> +               if (IS_ERR(folio)) {
>                          /*
>                           * We have a HOLE, zero out the user-buffer for the
>                           * length of the hole or request.
>                           */
>                          copied = iov_iter_zero(nr, to);
>                  } else {
> -                       unlock_page(page);
> +                       folio_unlock(folio);
> 
> -                       if (!PageHWPoison(page))
> +                       if (!folio_test_has_hwpoisoned(folio))
>                                  want = nr;
> 
> So I guess this "PageHWPoison => folio_test_has_hwpoisoned" change is
> another regression aside from the refcount thing?

ya this is another error. The refcount change fixes the madvise() call in the 
tests but the poison read tests still failed. The change to 
folio_test_hwpoison() fixes the poison read tests after the madvise() call 
succeeds.