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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux