Re: [PATCH 1/5] mm: remove find_subpage()

[David Hildenbrand <david@redhat.com>]

On 19.08.24 13:02, Kefeng Wang wrote:
> 
> 
> On 2024/8/17 17:51, Kefeng Wang wrote:
>> After commit a08c7193e4f1 ("mm/filemap: remove hugetlb special casing
>> in filemap.c"), the find_subpage() should remove hugetlb case as the
>> folio_file_page(), furthermore, we could convert to use folio_file_page()
>> to remove find_subpage().
> 
> There are some comments from David to the non-public send(forget to cc
> list),
> the problem of find_subpage() is not described , so adding some here,
> 

Thanks!

> 
> see commit a08c7193e4f1,
> 
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -789,9 +789,6 @@ static inline pgoff_t folio_next_index(struct folio
> *folio)
>     */
>    static inline struct page *folio_file_page(struct folio *folio,
> pgoff_t index)
>    {
> -       /* HugeTLBfs indexes the page cache in units of hpage_size */
> -       if (folio_test_hugetlb(folio))
> -               return &folio->page;
>           return folio_page(folio, index & (folio_nr_pages(folio) - 1));
>    }
> 
> It changes the granularity of ->index to the base page size rather than
> the huge page size, so for hugetlb, the special handling(return head
> page) is removed from folio_file_page(), so we need remove special
> hugetlb handling find_subpage() too, maybe this is a bugfix as a
> separate patch.

That's the part I understand. Any caller of folio_file_page() is 
expected to be able to deal with a hugetlb tail page after this patch.

Now, your assumption is the callers of find_subpage() are find with a 
tail page as well. That's the part I am not sure about, but if you think 
all callers are fine, then please spell that out in the patch description.

Something like

"Note that find_subpage() would never return the tail page of a hugetlb 
folio, but folio_file_page() will return tail pages. This, however, is 
fine because XYZ".

But I am wondering if these functions here even get called for hugetlb 
ever ... :)

They only trigger for iov_iter_is_xarray()?

> 
> And after removing hugetlb handling in find_subpage(), there is
> another issue about "head + (index & (thp_nr_pages(head) - 1))", for
> hugetlb without sparsemem vmemmap, struct page is not guaranteed to be
> contiguous beyond a section, so we need to use
> 
>    nth_page(head, (index & (thp_nr_pages(head) - 1))

Agreed. So as soon as we would unlock that code for THP, we would run 
into that issue.

> 
> and in order to reduce code maintain between folio_file_page() and
> find_subpage()， just use folio_file_page() in find_subpage() to
> fix above two issue.
> 
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index d9c7edb6422b..e2553e4ac3ef 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -866,11 +866,9 @@ static inline bool folio_contains(struct folio
> *folio, pgoff_t index)
>     */
>    static inline struct page *find_subpage(struct page *head, pgoff_t index)
>    {
> -       /* HugeTLBfs wants the head page regardless */
> -       if (PageHuge(head))
> -               return head;
> +       struct folio *folio = (struct folio *)head;
> 
> -       return head + (index & (thp_nr_pages(head) - 1));
> +       return folio_file_page(folio);

^ I assume you meant "folio_file_page(folio, index);"

>    }
> 
> And this will correctly handle head/tail page, correct me if I am wrong.

Right.

Is iov_iter_xarray() one of the things Willy mentioned is scheduled for 
removal? Then I agree that looking into removing that part completely 
does also sounds reasonable.

-- 
Cheers,

David / dhildenb