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

[Huang, Ying]

Kairui Song <ryncsn@xxxxxxxxx> writes:

> From: Kairui Song <kasong@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
>  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