Re: [PATCH 6/8] mm/swap: get the swap file offset directly

[Huang, Ying]

Kairui Song <ryncsn@xxxxxxxxx> writes:

> From: Kairui Song <kasong@xxxxxxxxxxx>
>
> folio_file_pos and page_file_offset are for mixed usage of swap cache
> and page cache, it can't be page cache here, so introduce a new helper
> to get the swap offset in swap file directly.
>
> Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> ---
>  mm/page_io.c | 6 +++---
>  mm/swap.h    | 5 +++++
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/mm/page_io.c b/mm/page_io.c
> index ae2b49055e43..93de5aadb438 100644
> --- a/mm/page_io.c
> +++ b/mm/page_io.c
> @@ -279,7 +279,7 @@ static void sio_write_complete(struct kiocb *iocb, long ret)
>  		 * be temporary.
>  		 */
>  		pr_err_ratelimited("Write error %ld on dio swapfile (%llu)\n",
> -				   ret, page_file_offset(page));
> +				   ret, swap_file_pos(page_swap_entry(page)));
>  		for (p = 0; p < sio->pages; p++) {
>  			page = sio->bvec[p].bv_page;
>  			set_page_dirty(page);
> @@ -298,7 +298,7 @@ static void swap_writepage_fs(struct folio *folio, struct writeback_control *wbc
>  	struct swap_iocb *sio = NULL;
>  	struct swap_info_struct *sis = swp_swap_info(folio->swap);
>  	struct file *swap_file = sis->swap_file;
> -	loff_t pos = folio_file_pos(folio);
> +	loff_t pos = swap_file_pos(folio->swap);
>  
>  	count_swpout_vm_event(folio);
>  	folio_start_writeback(folio);
> @@ -429,7 +429,7 @@ static void swap_read_folio_fs(struct folio *folio, struct swap_iocb **plug)
>  {
>  	struct swap_info_struct *sis = swp_swap_info(folio->swap);
>  	struct swap_iocb *sio = NULL;
> -	loff_t pos = folio_file_pos(folio);
> +	loff_t pos = swap_file_pos(folio->swap);
>  
>  	if (plug)
>  		sio = *plug;
> diff --git a/mm/swap.h b/mm/swap.h
> index fc2f6ade7f80..2de83729aaa8 100644
> --- a/mm/swap.h
> +++ b/mm/swap.h
> @@ -7,6 +7,11 @@ struct mempolicy;
>  #ifdef CONFIG_SWAP
>  #include <linux/blk_types.h> /* for bio_end_io_t */
>  
> +static inline loff_t swap_file_pos(swp_entry_t entry)
> +{
> +	return ((loff_t)swp_offset(entry)) << PAGE_SHIFT;
> +}
> +
>  /* linux/mm/page_io.c */
>  int sio_pool_init(void);
>  struct swap_iocb;

I feel that the file concept for swap is kind of confusing.  From the
file cache point of view, one "struct address space" conresponds to one
file.  If so, we have a simple file system on a swap device (block
device backed or file backed), where the size of each file is 64M.  The
swap entry encode the file system (swap_type), the file name
(swap_offset >> SWAP_ADDRESS_SPACE_SHIFT), and the offset in file (lower
bits of swap_offset).

If the above definition is good, it's better to rename swap_file_pos()
to swap_dev_pos(), because it returns the swap device position of the
swap entry.

And, when we reaches consensus on the swap file related concept, we may
document it somewhere and review all naming in swap code to cleanup.

--
Best Regards,
Huang, Ying