[PATCH 0/5] mm: finish three more folio conversion

[PATCH 0/5] mm: finish three more folio conversion

[Kefeng Wang]

Convert to use folios then remove find_subpage(), thp_nr_pages()
and PageTransHuge(). 

---
The thp_nr_pages() remove depends on "mm: memory_hotplug: remove head
variable in do_migrate_range()"[1]

[1] https://lore.kernel.org/linux-mm/20240817084941.2375713-2-wangkefeng.wang@huawei.com/

Kefeng Wang (5):
  mm: remove find_subpage()
  pagemap: use a folio in __readahead_batch()
  mm: remove thp_nr_pages()
  mm: khugepaged: pass a folio for set_huge_pmd()
  mm: remove PageTransHuge()

 include/linux/mm.h         |  9 ---------
 include/linux/page-flags.h | 13 -------------
 include/linux/pagemap.h    | 26 ++++++--------------------
 include/linux/pgtable.h    |  2 +-
 lib/iov_iter.c             | 24 +++++++++++++-----------
 mm/khugepaged.c            | 10 ++++------
 6 files changed, 24 insertions(+), 60 deletions(-)

-- 
2.27.0

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

[Kefeng Wang]

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

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;
 	}
-- 
2.27.0

[PATCH 2/5] pagemap: use a folio in __readahead_batch()

[Kefeng Wang]

Convert to use a folio in __readahead_batch(), remove the
check of PageTail and thp_nr_pages().

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/linux/pagemap.h | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 68f59cd7637d..a83913555497 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -1363,7 +1363,7 @@ static inline unsigned int __readahead_batch(struct readahead_control *rac,
 {
 	unsigned int i = 0;
 	XA_STATE(xas, &rac->mapping->i_pages, 0);
-	struct page *page;
+	struct folio *folio;
 
 	BUG_ON(rac->_batch_count > rac->_nr_pages);
 	rac->_nr_pages -= rac->_batch_count;
@@ -1372,13 +1372,12 @@ static inline unsigned int __readahead_batch(struct readahead_control *rac,
 
 	xas_set(&xas, rac->_index);
 	rcu_read_lock();
-	xas_for_each(&xas, page, rac->_index + rac->_nr_pages - 1) {
-		if (xas_retry(&xas, page))
+	xas_for_each(&xas, folio, rac->_index + rac->_nr_pages - 1) {
+		if (xas_retry(&xas, folio))
 			continue;
-		VM_BUG_ON_PAGE(!PageLocked(page), page);
-		VM_BUG_ON_PAGE(PageTail(page), page);
-		array[i++] = page;
-		rac->_batch_count += thp_nr_pages(page);
+		VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
+		array[i++] = &folio->page;
+		rac->_batch_count += folio_nr_pages(folio);
 		if (i == array_sz)
 			break;
 	}
-- 
2.27.0

[PATCH 3/5] mm: remove thp_nr_pages()

[Kefeng Wang]

There are no users of thp_nr_pages(), remove it.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/linux/mm.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 653287396808..d6a1b8356a31 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2065,15 +2065,6 @@ static inline unsigned long compound_nr(struct page *page)
 #endif
 }
 
-/**
- * thp_nr_pages - The number of regular pages in this huge page.
- * @page: The head page of a huge page.
- */
-static inline int thp_nr_pages(struct page *page)
-{
-	return folio_nr_pages((struct folio *)page);
-}
-
 /**
  * folio_next - Move to the next physical folio.
  * @folio: The folio we're currently operating on.
-- 
2.27.0

[PATCH 4/5] mm: khugepaged: pass a folio for set_huge_pmd()

[Kefeng Wang]

Save one compound_head() and remove last PageTransHuge() in set_huge_pmd().

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/khugepaged.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 02e1463e1a79..ae796219f8a0 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1455,7 +1455,7 @@ static void collect_mm_slot(struct khugepaged_mm_slot *mm_slot)
 #ifdef CONFIG_SHMEM
 /* hpage must be locked, and mmap_lock must be held */
 static int set_huge_pmd(struct vm_area_struct *vma, unsigned long addr,
-			pmd_t *pmdp, struct page *hpage)
+			pmd_t *pmdp, struct folio *folio)
 {
 	struct vm_fault vmf = {
 		.vma = vma,
@@ -1464,13 +1464,12 @@ static int set_huge_pmd(struct vm_area_struct *vma, unsigned long addr,
 		.pmd = pmdp,
 	};
 
-	VM_BUG_ON(!PageTransHuge(hpage));
 	mmap_assert_locked(vma->vm_mm);
 
-	if (do_set_pmd(&vmf, hpage))
+	if (do_set_pmd(&vmf, &folio->page))
 		return SCAN_FAIL;
 
-	get_page(hpage);
+	folio_get(folio);
 	return SCAN_SUCCEED;
 }
 
@@ -1670,8 +1669,7 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
 maybe_install_pmd:
 	/* step 5: install pmd entry */
 	result = install_pmd
-			? set_huge_pmd(vma, haddr, pmd, &folio->page)
-			: SCAN_SUCCEED;
+			? set_huge_pmd(vma, haddr, pmd, folio) : SCAN_SUCCEED;
 	goto drop_folio;
 abort:
 	if (nr_ptes) {
-- 
2.27.0

[PATCH 5/5] mm: remove PageTransHuge()

[Kefeng Wang]

There are no users of PageTransHuge(), remove it.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/linux/page-flags.h | 13 -------------
 include/linux/pgtable.h    |  2 +-
 2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index b753d158762f..71e972ba489e 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -871,19 +871,6 @@ FOLIO_FLAG_FALSE(partially_mapped)
 #define PG_head_mask ((1UL << PG_head))
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-/*
- * PageHuge() only returns true for hugetlbfs pages, but not for
- * normal or transparent huge pages.
- *
- * PageTransHuge() returns true for both transparent huge and
- * hugetlbfs pages, but not normal pages. PageTransHuge() can only be
- * called only in the core VM paths where hugetlbfs pages can't exist.
- */
-static inline int PageTransHuge(const struct page *page)
-{
-	VM_BUG_ON_PAGE(PageTail(page), page);
-	return PageHead(page);
-}
 
 /*
  * PageTransCompound returns true for both transparent huge pages
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index 780f3b439d98..36fc336782e8 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -398,7 +398,7 @@ extern int pmdp_clear_flush_young(struct vm_area_struct *vma,
 #else
 /*
  * Despite relevant to THP only, this API is called from generic rmap code
- * under PageTransHuge(), hence needs a dummy implementation for !THP
+ * under THP, hence needs a dummy implementation for !THP
  */
 static inline int pmdp_clear_flush_young(struct vm_area_struct *vma,
 					 unsigned long address, pmd_t *pmdp)
-- 
2.27.0

Re: [PATCH 0/5] mm: finish three more folio conversion

[Matthew Wilcox]

On Sat, Aug 17, 2024 at 05:51:17PM +0800, Kefeng Wang wrote:
> Convert to use folios then remove find_subpage(), thp_nr_pages()
> and PageTransHuge(). 

This patch series is premature.  None of these APIs are particularly
advantageous to remove (they're inline functions, so cost nothing unless
used).  All of the places which use them are scheduled for removal.
If you want to help, converting the filesystems which use the batch
interfaces to use folios instead of pages would be a good way to start.

Re: [PATCH 0/5] mm: finish three more folio conversion

[David Hildenbrand]

On 18.08.24 22:33, Matthew Wilcox wrote:
> On Sat, Aug 17, 2024 at 05:51:17PM +0800, Kefeng Wang wrote:
>> Convert to use folios then remove find_subpage(), thp_nr_pages()
>> and PageTransHuge().
> 
> This patch series is premature.  None of these APIs are particularly
> advantageous to remove (they're inline functions, so cost nothing unless
> used). 
I do enjoy seeing the removal of thp_nr_pages(), though :)

-- 
Cheers,

David / dhildenb

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

[Kefeng Wang]

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.

Re: [PATCH 0/5] mm: finish three more folio conversion

[Kefeng Wang]

On 2024/8/19 17:59, David Hildenbrand wrote:
> On 18.08.24 22:33, Matthew Wilcox wrote:
>> On Sat, Aug 17, 2024 at 05:51:17PM +0800, Kefeng Wang wrote:
>>> Convert to use folios then remove find_subpage(), thp_nr_pages()
>>> and PageTransHuge().
>>
>> This patch series is premature.  None of these APIs are particularly
>> advantageous to remove (they're inline functions, so cost nothing unless
>> used). 
> I do enjoy seeing the removal of thp_nr_pages(), though :)
> 

The thp_nr_pages() and PageTransHuge() removing are found when improve
memory hotplug[1], also find_subpage() has issue[2], this is where 
patches come from, since there are only few callers, I think we can
cleanup it earlier?

[1] 
https://lore.kernel.org/linux-mm/20240817084941.2375713-2-wangkefeng.wang@huawei.com/
[2] 
https://lore.kernel.org/linux-mm/646f8a48-820f-40ae-bf96-7d554bf4493a@huawei.com/T/#u

Re: [PATCH 0/5] mm: finish three more folio conversion

[Matthew Wilcox]

On Mon, Aug 19, 2024 at 07:13:18PM +0800, Kefeng Wang wrote:
> On 2024/8/19 17:59, David Hildenbrand wrote:
> > On 18.08.24 22:33, Matthew Wilcox wrote:
> > > On Sat, Aug 17, 2024 at 05:51:17PM +0800, Kefeng Wang wrote:
> > > > Convert to use folios then remove find_subpage(), thp_nr_pages()
> > > > and PageTransHuge().
> > > 
> > > This patch series is premature.  None of these APIs are particularly
> > > advantageous to remove (they're inline functions, so cost nothing unless
> > > used).
> > I do enjoy seeing the removal of thp_nr_pages(), though :)
> > 
> 
> The thp_nr_pages() and PageTransHuge() removing are found when improve
> memory hotplug[1], also find_subpage() has issue[2], this is where patches
> come from, since there are only few callers, I think we can
> cleanup it earlier?

No, leave them alone until we get rid of readahead_batch() and friends.

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

[David Hildenbrand]

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.

-- 
Cheers,

David / dhildenb

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

[Kefeng Wang]

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.

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

[David Hildenbrand]

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

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

[Kefeng Wang]

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

Re: [PATCH 0/5] mm: finish three more folio conversion

[Kefeng Wang]

On 2024/8/19 21:20, Matthew Wilcox wrote:
> On Mon, Aug 19, 2024 at 07:13:18PM +0800, Kefeng Wang wrote:
>> On 2024/8/19 17:59, David Hildenbrand wrote:
>>> On 18.08.24 22:33, Matthew Wilcox wrote:
>>>> On Sat, Aug 17, 2024 at 05:51:17PM +0800, Kefeng Wang wrote:
>>>>> Convert to use folios then remove find_subpage(), thp_nr_pages()
>>>>> and PageTransHuge().
>>>>
>>>> This patch series is premature.  None of these APIs are particularly
>>>> advantageous to remove (they're inline functions, so cost nothing unless
>>>> used).
>>> I do enjoy seeing the removal of thp_nr_pages(), though :)
>>>
>>
>> The thp_nr_pages() and PageTransHuge() removing are found when improve
>> memory hotplug[1], also find_subpage() has issue[2], this is where patches
>> come from, since there are only few callers, I think we can
>> cleanup it earlier?
> 
> No, leave them alone until we get rid of readahead_batch() and friends.
> 

It is beyond my scope about changing fuse/squashfs to remove
readahead_batch, will separate find_subpage/set_huge_pmd changes,
thanks for you suggestion.