[PATCH v2 1/8] NFS: remove nfs_page_lengthg and usage of page_index

[PATCH v2 1/8] NFS: remove nfs_page_lengthg and usage of page_index

[Kairui Song]

From: Kairui Song <kasong@tencent.com>

This function is no longer used after
commit 4fa7a717b432 ("NFS: Fix up nfs_vm_page_mkwrite() for folios"),
all users have been converted to use folio instead, just delete it to
remove usage of page_index.

Signed-off-by: Kairui Song <kasong@tencent.com>
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
Cc: Anna Schumaker <anna@kernel.org>
Cc: linux-nfs@vger.kernel.org
---
 fs/nfs/internal.h | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 06253695fe53..deac98dce6ac 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -790,25 +790,6 @@ static inline void nfs_folio_mark_unstable(struct folio *folio,
 	}
 }
 
-/*
- * Determine the number of bytes of data the page contains
- */
-static inline
-unsigned int nfs_page_length(struct page *page)
-{
-	loff_t i_size = i_size_read(page_file_mapping(page)->host);
-
-	if (i_size > 0) {
-		pgoff_t index = page_index(page);
-		pgoff_t end_index = (i_size - 1) >> PAGE_SHIFT;
-		if (index < end_index)
-			return PAGE_SIZE;
-		if (index == end_index)
-			return ((i_size - 1) & ~PAGE_MASK) + 1;
-	}
-	return 0;
-}
-
 /*
  * Determine the number of bytes of data the page contains
  */
-- 
2.44.0

Re: [PATCH v2 7/8] mm: drop page_index/page_file_offset and convert swap helpers to use folio

[Huang, Ying]

Kairui Song <ryncsn@gmail.com> writes:

> From: Kairui Song <kasong@tencent.com>
>
> There are four helpers for retrieving the page index within address
> space, or offset within mapped file:
>
> - page_index
> - page_file_offset
> - folio_index (equivalence of page_index)
> - folio_file_pos (equivalence of page_file_offset)
>
> And they are only needed for mixed usage of swap cache & page cache (eg.
> migration, huge memory split). Else users can just use folio->index or
> folio_pos.
>
> This commit drops page_index and page_file_offset as we have eliminated
> all users, and converts folio_index and folio_file_pos (they were simply
> wrappers of page_index and page_file_offset, and implemented in a not
> very clean way) to use folio internally.
>
> After this commit, there will be only two helpers for users that may
> encounter mixed usage of swap cache and page cache:
>
> - folio_index (calls __folio_swap_cache_index for swap cache folio)
> - folio_file_pos (calls __folio_swap_dev_pos for swap cache folio)
>
> The index in swap cache and offset in swap device are still basically
> the same thing, but will be different in following commits.
>
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>  include/linux/mm.h      | 13 -------------
>  include/linux/pagemap.h | 19 +++++++++----------
>  mm/swapfile.c           | 13 +++++++++----
>  3 files changed, 18 insertions(+), 27 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0436b919f1c7..797480e76c9c 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2245,19 +2245,6 @@ static inline void *folio_address(const struct folio *folio)
>  	return page_address(&folio->page);
>  }
>  
> -extern pgoff_t __page_file_index(struct page *page);
> -
> -/*
> - * Return the pagecache index of the passed page.  Regular pagecache pages
> - * use ->index whereas swapcache pages use swp_offset(->private)
> - */
> -static inline pgoff_t page_index(struct page *page)
> -{
> -	if (unlikely(PageSwapCache(page)))
> -		return __page_file_index(page);
> -	return page->index;
> -}
> -
>  /*
>   * Return true only if the page has been allocated with
>   * ALLOC_NO_WATERMARKS and the low watermark was not
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 2df35e65557d..a7d025571ee6 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -780,7 +780,7 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
>  			mapping_gfp_mask(mapping));
>  }
>  
> -#define swapcache_index(folio)	__page_file_index(&(folio)->page)
> +extern pgoff_t __folio_swap_cache_index(struct folio *folio);
>  
>  /**
>   * folio_index - File index of a folio.
> @@ -795,9 +795,9 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
>   */
>  static inline pgoff_t folio_index(struct folio *folio)
>  {
> -        if (unlikely(folio_test_swapcache(folio)))
> -                return swapcache_index(folio);
> -        return folio->index;
> +	if (unlikely(folio_test_swapcache(folio)))
> +		return __folio_swap_cache_index(folio);
> +	return folio->index;
>  }
>  
>  /**
> @@ -920,11 +920,6 @@ static inline loff_t page_offset(struct page *page)
>  	return ((loff_t)page->index) << PAGE_SHIFT;
>  }
>  
> -static inline loff_t page_file_offset(struct page *page)
> -{
> -	return ((loff_t)page_index(page)) << PAGE_SHIFT;
> -}
> -
>  /**
>   * folio_pos - Returns the byte position of this folio in its file.
>   * @folio: The folio.
> @@ -934,6 +929,8 @@ static inline loff_t folio_pos(struct folio *folio)
>  	return page_offset(&folio->page);
>  }
>  
> +extern loff_t __folio_swap_dev_pos(struct folio *folio);
> +
>  /**
>   * folio_file_pos - Returns the byte position of this folio in its file.
>   * @folio: The folio.
> @@ -943,7 +940,9 @@ static inline loff_t folio_pos(struct folio *folio)
>   */
>  static inline loff_t folio_file_pos(struct folio *folio)
>  {
> -	return page_file_offset(&folio->page);
> +	if (unlikely(folio_test_swapcache(folio)))
> +		return __folio_swap_dev_pos(folio);
> +	return ((loff_t)folio->index << PAGE_SHIFT);

This still looks confusing for me.  The function returns the byte
position of the folio in its file.  But we returns the swap device
position of the folio.

Tried to search folio_file_pos() usage.  The 2 usage in page_io.c is
swap specific, we can use swap_dev_pos() directly.

There are also other file system users (NFS and AFS) of
folio_file_pos(), I don't know why they need to work with swap
cache. Cced file system maintainers for help.

>  }
>  
>  /*
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 4919423cce76..2387f5e131d7 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3419,12 +3419,17 @@ struct address_space *swapcache_mapping(struct folio *folio)
>  }
>  EXPORT_SYMBOL_GPL(swapcache_mapping);
>  
> -pgoff_t __page_file_index(struct page *page)
> +pgoff_t __folio_swap_cache_index(struct folio *folio)
>  {
> -	swp_entry_t swap = page_swap_entry(page);
> -	return swp_offset(swap);
> +	return swp_offset(folio->swap);
>  }
> -EXPORT_SYMBOL_GPL(__page_file_index);
> +EXPORT_SYMBOL_GPL(__folio_swap_cache_index);
> +
> +loff_t __folio_swap_dev_pos(struct folio *folio)
> +{
> +	return swap_dev_pos(folio->swap);
> +}
> +EXPORT_SYMBOL_GPL(__folio_swap_dev_pos);
>  
>  /*
>   * add_swap_count_continuation - called when a swap count is duplicated

--
Best Regards,
Huang, Ying

Re: [PATCH v2 7/8] mm: drop page_index/page_file_offset and convert swap helpers to use folio

[Kairui Song]

On Wed, Apr 24, 2024 at 10:19 AM Huang, Ying <ying.huang@intel.com> wrote:
>
> Kairui Song <ryncsn@gmail.com> writes:
>
> > From: Kairui Song <kasong@tencent.com>
> >
> > There are four helpers for retrieving the page index within address
> > space, or offset within mapped file:
> >
> > - page_index
> > - page_file_offset
> > - folio_index (equivalence of page_index)
> > - folio_file_pos (equivalence of page_file_offset)
> >
> > And they are only needed for mixed usage of swap cache & page cache (eg.
> > migration, huge memory split). Else users can just use folio->index or
> > folio_pos.
> >
> > This commit drops page_index and page_file_offset as we have eliminated
> > all users, and converts folio_index and folio_file_pos (they were simply
> > wrappers of page_index and page_file_offset, and implemented in a not
> > very clean way) to use folio internally.
> >
> > After this commit, there will be only two helpers for users that may
> > encounter mixed usage of swap cache and page cache:
> >
> > - folio_index (calls __folio_swap_cache_index for swap cache folio)
> > - folio_file_pos (calls __folio_swap_dev_pos for swap cache folio)
> >
> > The index in swap cache and offset in swap device are still basically
> > the same thing, but will be different in following commits.
> >
> > Signed-off-by: Kairui Song <kasong@tencent.com>
> > ---
> >  include/linux/mm.h      | 13 -------------
> >  include/linux/pagemap.h | 19 +++++++++----------
> >  mm/swapfile.c           | 13 +++++++++----
> >  3 files changed, 18 insertions(+), 27 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 0436b919f1c7..797480e76c9c 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2245,19 +2245,6 @@ static inline void *folio_address(const struct folio *folio)
> >       return page_address(&folio->page);
> >  }
> >
> > -extern pgoff_t __page_file_index(struct page *page);
> > -
> > -/*
> > - * Return the pagecache index of the passed page.  Regular pagecache pages
> > - * use ->index whereas swapcache pages use swp_offset(->private)
> > - */
> > -static inline pgoff_t page_index(struct page *page)
> > -{
> > -     if (unlikely(PageSwapCache(page)))
> > -             return __page_file_index(page);
> > -     return page->index;
> > -}
> > -
> >  /*
> >   * Return true only if the page has been allocated with
> >   * ALLOC_NO_WATERMARKS and the low watermark was not
> > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> > index 2df35e65557d..a7d025571ee6 100644
> > --- a/include/linux/pagemap.h
> > +++ b/include/linux/pagemap.h
> > @@ -780,7 +780,7 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
> >                       mapping_gfp_mask(mapping));
> >  }
> >
> > -#define swapcache_index(folio)       __page_file_index(&(folio)->page)
> > +extern pgoff_t __folio_swap_cache_index(struct folio *folio);
> >
> >  /**
> >   * folio_index - File index of a folio.
> > @@ -795,9 +795,9 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
> >   */
> >  static inline pgoff_t folio_index(struct folio *folio)
> >  {
> > -        if (unlikely(folio_test_swapcache(folio)))
> > -                return swapcache_index(folio);
> > -        return folio->index;
> > +     if (unlikely(folio_test_swapcache(folio)))
> > +             return __folio_swap_cache_index(folio);
> > +     return folio->index;
> >  }
> >
> >  /**
> > @@ -920,11 +920,6 @@ static inline loff_t page_offset(struct page *page)
> >       return ((loff_t)page->index) << PAGE_SHIFT;
> >  }
> >
> > -static inline loff_t page_file_offset(struct page *page)
> > -{
> > -     return ((loff_t)page_index(page)) << PAGE_SHIFT;
> > -}
> > -
> >  /**
> >   * folio_pos - Returns the byte position of this folio in its file.
> >   * @folio: The folio.
> > @@ -934,6 +929,8 @@ static inline loff_t folio_pos(struct folio *folio)
> >       return page_offset(&folio->page);
> >  }
> >
> > +extern loff_t __folio_swap_dev_pos(struct folio *folio);
> > +
> >  /**
> >   * folio_file_pos - Returns the byte position of this folio in its file.
> >   * @folio: The folio.
> > @@ -943,7 +940,9 @@ static inline loff_t folio_pos(struct folio *folio)
> >   */
> >  static inline loff_t folio_file_pos(struct folio *folio)
> >  {
> > -     return page_file_offset(&folio->page);
> > +     if (unlikely(folio_test_swapcache(folio)))
> > +             return __folio_swap_dev_pos(folio);
> > +     return ((loff_t)folio->index << PAGE_SHIFT);
>
> This still looks confusing for me.  The function returns the byte
> position of the folio in its file.  But we returns the swap device
> position of the folio.

Thanks for the comment.

This doesn't look too confusing to me, __folio_swap_dev_pos ->
swap_dev_pos also returns the byte position of the folio in swap
device. If we agree swap device is kind of equivalent to the page
cache file here, it shouldn't be too hard to understand.

>
> Tried to search folio_file_pos() usage.  The 2 usage in page_io.c is
> swap specific, we can use swap_dev_pos() directly.

The 2 usage in page_io.c is already converted to use swap_dev_pos
directly in this series (patch 6/8).

>
> There are also other file system users (NFS and AFS) of
> folio_file_pos(), I don't know why they need to work with swap
> cache. Cced file system maintainers for help.
>

Thanks, I'm not very sure if we can just drop folio_file_pos and
convert all users to use folio_pos directly. Swap cache mapping
shouldn't be exposed to fs, but I'm not confident enough that this is
a safe move. It looks OK to do so just by examining NFS code, but
let's wait for feedback from FS people.

Re: [PATCH v2 7/8] mm: drop page_index/page_file_offset and convert swap helpers to use folio

[Matthew Wilcox]

On Wed, Apr 24, 2024 at 10:17:04AM +0800, Huang, Ying wrote:
> Kairui Song <ryncsn@gmail.com> writes:
> >  static inline loff_t folio_file_pos(struct folio *folio)
> >  {
> > -	return page_file_offset(&folio->page);
> > +	if (unlikely(folio_test_swapcache(folio)))
> > +		return __folio_swap_dev_pos(folio);
> > +	return ((loff_t)folio->index << PAGE_SHIFT);
> 
> This still looks confusing for me.  The function returns the byte
> position of the folio in its file.  But we returns the swap device
> position of the folio.
> 
> Tried to search folio_file_pos() usage.  The 2 usage in page_io.c is
> swap specific, we can use swap_dev_pos() directly.
> 
> There are also other file system users (NFS and AFS) of
> folio_file_pos(), I don't know why they need to work with swap
> cache. Cced file system maintainers for help.

Time for a history lesson!

In d56b4ddf7781 (2012) we introduced page_file_index() and
page_file_mapping() to support swap-over-NFS.  Writes to the swapfile went
through ->direct_IO but reads went through ->readpage.  So NFS was changed
to remove direct references to page->mapping and page->index because
those aren't right for anon pages (or shmem pages being swapped out).

In e1209d3a7a67 (2022), we stopped using ->readpage in favour of using
->swap_rw.  Now we don't need to use page_file_*(); we get the swap_file
and ki_pos directly in the swap_iocb.  But there are still relics in NFS
that nobody has dared rip out.  And there are all the copy-and-pasted
filesystems that use page_file_* because they don't know any better.

We should delete page_file_*() and folio_file_*().  They shouldn't be
needed any more.

Re: [PATCH v2 7/8] mm: drop page_index/page_file_offset and convert swap helpers to use folio

[Huang, Ying]

Hi, Matthew,

Matthew Wilcox <willy@infradead.org> writes:

> On Wed, Apr 24, 2024 at 10:17:04AM +0800, Huang, Ying wrote:
>> Kairui Song <ryncsn@gmail.com> writes:
>> >  static inline loff_t folio_file_pos(struct folio *folio)
>> >  {
>> > -	return page_file_offset(&folio->page);
>> > +	if (unlikely(folio_test_swapcache(folio)))
>> > +		return __folio_swap_dev_pos(folio);
>> > +	return ((loff_t)folio->index << PAGE_SHIFT);
>> 
>> This still looks confusing for me.  The function returns the byte
>> position of the folio in its file.  But we returns the swap device
>> position of the folio.
>> 
>> Tried to search folio_file_pos() usage.  The 2 usage in page_io.c is
>> swap specific, we can use swap_dev_pos() directly.
>> 
>> There are also other file system users (NFS and AFS) of
>> folio_file_pos(), I don't know why they need to work with swap
>> cache. Cced file system maintainers for help.
>
> Time for a history lesson!
>
> In d56b4ddf7781 (2012) we introduced page_file_index() and
> page_file_mapping() to support swap-over-NFS.  Writes to the swapfile went
> through ->direct_IO but reads went through ->readpage.  So NFS was changed
> to remove direct references to page->mapping and page->index because
> those aren't right for anon pages (or shmem pages being swapped out).
>
> In e1209d3a7a67 (2022), we stopped using ->readpage in favour of using
> ->swap_rw.  Now we don't need to use page_file_*(); we get the swap_file
> and ki_pos directly in the swap_iocb.  But there are still relics in NFS
> that nobody has dared rip out.  And there are all the copy-and-pasted
> filesystems that use page_file_* because they don't know any better.
>
> We should delete page_file_*() and folio_file_*().  They shouldn't be
> needed any more.

Thanks a lot for your detailed explanation!  Yes, this will simplify the
semantics and improve the readability of the corresponding code.

--
Best Regards,
Huang, Ying

Re: [PATCH v2 7/8] mm: drop page_index/page_file_offset and convert swap helpers to use folio

[Kairui Song]

On Wed, Apr 24, 2024 at 12:06 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Apr 24, 2024 at 10:17:04AM +0800, Huang, Ying wrote:
> > Kairui Song <ryncsn@gmail.com> writes:
> > >  static inline loff_t folio_file_pos(struct folio *folio)
> > >  {
> > > -   return page_file_offset(&folio->page);
> > > +   if (unlikely(folio_test_swapcache(folio)))
> > > +           return __folio_swap_dev_pos(folio);
> > > +   return ((loff_t)folio->index << PAGE_SHIFT);
> >
> > This still looks confusing for me.  The function returns the byte
> > position of the folio in its file.  But we returns the swap device
> > position of the folio.
> >
> > Tried to search folio_file_pos() usage.  The 2 usage in page_io.c is
> > swap specific, we can use swap_dev_pos() directly.
> >
> > There are also other file system users (NFS and AFS) of
> > folio_file_pos(), I don't know why they need to work with swap
> > cache. Cced file system maintainers for help.
>
> Time for a history lesson!
>
> In d56b4ddf7781 (2012) we introduced page_file_index() and
> page_file_mapping() to support swap-over-NFS.  Writes to the swapfile went
> through ->direct_IO but reads went through ->readpage.  So NFS was changed
> to remove direct references to page->mapping and page->index because
> those aren't right for anon pages (or shmem pages being swapped out).
>
> In e1209d3a7a67 (2022), we stopped using ->readpage in favour of using
> ->swap_rw.  Now we don't need to use page_file_*(); we get the swap_file
> and ki_pos directly in the swap_iocb.  But there are still relics in NFS
> that nobody has dared rip out.  And there are all the copy-and-pasted
> filesystems that use page_file_* because they don't know any better.
>
> We should delete page_file_*() and folio_file_*().  They shouldn't be
> needed any more.

Thanks for the explanation! I'll update the series, and just delete
paeg_file_offset and folio_file_pos with more auditing, to make the
code cleaner. Should I add a suggest-by for the removal?