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().
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.
Since no hugetlb involved, there is no issue even we drop hugetlb
handling.
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);"
Right.
}
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.
I think Willy want to remove __readahead_batch() totally, not related
about iter xarray.
