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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/8/20 16:23, David Hildenbrand wrote:
> On 20.08.24 10:22, Kefeng Wang wrote:
>>
>>
>> On 2024/8/19 21:27, David Hildenbrand wrote:
>>> 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()?
>>
>> Yes, the find_subpage only used under iov_iter_is_xarray(),  and
>> only afs/erofs/netfs/orangefs/ceph/smb use ITER_XARRAY, no hugetlb
>> involved, so we could safety drop hugetlb part in find_subpage().
> 
> Highlighting that in the patch description would likely be best. The 
> hugetlb check is essentially dead code right now ... and confuses David :)

Thanks for your kindly review and helper, David :)
>