| Message ID | 20240817095122.2460977-2-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: finish three more folio conversion | expand |
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, 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. 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)) 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); } And this will correctly handle head/tail page, correct me if I am wrong. Thanks.
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.
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.
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 :)
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 :) >
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index d9c7edb6422b..68f59cd7637d 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -860,19 +860,6 @@ static inline bool folio_contains(struct folio *folio, pgoff_t index) return index - folio_index(folio) < folio_nr_pages(folio); } -/* - * Given the page we found in the page cache, return the page corresponding - * to this index in the file - */ -static inline struct page *find_subpage(struct page *head, pgoff_t index) -{ - /* HugeTLBfs wants the head page regardless */ - if (PageHuge(head)) - return head; - - return head + (index & (thp_nr_pages(head) - 1)); -} - unsigned filemap_get_folios(struct address_space *mapping, pgoff_t *start, pgoff_t end, struct folio_batch *fbatch); unsigned filemap_get_folios_contig(struct address_space *mapping, diff --git a/lib/iov_iter.c b/lib/iov_iter.c index 4a6a9f419bd7..b0bb1e5ff331 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -891,21 +891,21 @@ static ssize_t iter_xarray_populate_pages(struct page **pages, struct xarray *xa pgoff_t index, unsigned int nr_pages) { XA_STATE(xas, xa, index); - struct page *page; + struct folio *folio; unsigned int ret = 0; rcu_read_lock(); - for (page = xas_load(&xas); page; page = xas_next(&xas)) { - if (xas_retry(&xas, page)) + for (folio = xas_load(&xas); folio; folio = xas_next(&xas)) { + if (xas_retry(&xas, folio)) continue; /* Has the page moved or been split? */ - if (unlikely(page != xas_reload(&xas))) { + if (unlikely(folio != xas_reload(&xas))) { xas_reset(&xas); continue; } - pages[ret] = find_subpage(page, xas.xa_index); + pages[ret] = folio_file_page(folio, xas.xa_index); get_page(pages[ret]); if (++ret == nr_pages) break; @@ -1408,7 +1408,8 @@ static ssize_t iov_iter_extract_xarray_pages(struct iov_iter *i, iov_iter_extraction_t extraction_flags, size_t *offset0) { - struct page *page, **p; + struct page **p; + struct folio *folio; unsigned int nr = 0, offset; loff_t pos = i->xarray_start + i->iov_offset; pgoff_t index = pos >> PAGE_SHIFT; @@ -1420,20 +1421,21 @@ static ssize_t iov_iter_extract_xarray_pages(struct iov_iter *i, maxpages = want_pages_array(pages, maxsize, offset, maxpages); if (!maxpages) return -ENOMEM; + p = *pages; rcu_read_lock(); - for (page = xas_load(&xas); page; page = xas_next(&xas)) { - if (xas_retry(&xas, page)) + for (folio = xas_load(&xas); folio; folio = xas_next(&xas)) { + if (xas_retry(&xas, folio)) continue; - /* Has the page moved or been split? */ - if (unlikely(page != xas_reload(&xas))) { + /* Has the folio moved or been split? */ + if (unlikely(folio != xas_reload(&xas))) { xas_reset(&xas); continue; } - p[nr++] = find_subpage(page, xas.xa_index); + p[nr++] = folio_file_page(folio, xas.xa_index); if (nr == maxpages) break; }
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(). Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- include/linux/pagemap.h | 13 ------------- lib/iov_iter.c | 24 +++++++++++++----------- 2 files changed, 13 insertions(+), 24 deletions(-)