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

[David Hildenbrand <david@redhat.com>]

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 :)

-- 
Cheers,

David / dhildenb