| Message ID | 20240417160842.76665-7-ryncsn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/swap: optimize swap cache search space | expand |
Hi Kairui, kernel test robot noticed the following build errors: [auto build test ERROR on ceph-client/testing] [also build test ERROR on ceph-client/for-linus trondmy-nfs/linux-next konis-nilfs2/upstream jaegeuk-f2fs/dev-test jaegeuk-f2fs/dev cifs/for-next linus/master v6.9-rc4 next-20240418] [cannot apply to akpm-mm/mm-everything] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Kairui-Song/NFS-remove-nfs_page_lengthg-and-usage-of-page_index/20240418-001343 base: https://github.com/ceph/ceph-client.git testing patch link: https://lore.kernel.org/r/20240417160842.76665-7-ryncsn%40gmail.com patch subject: [PATCH 6/8] mm/swap: get the swap file offset directly config: alpha-defconfig (https://download.01.org/0day-ci/archive/20240419/202404190204.cy1pBxFg-lkp@intel.com/config) compiler: alpha-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240419/202404190204.cy1pBxFg-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202404190204.cy1pBxFg-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from mm/shmem.c:43: mm/swap.h: In function 'swap_file_pos': >> mm/swap.h:12:25: error: implicit declaration of function 'swp_offset'; did you mean 'pmd_offset'? [-Werror=implicit-function-declaration] 12 | return ((loff_t)swp_offset(entry)) << PAGE_SHIFT; | ^~~~~~~~~~ | pmd_offset In file included from mm/shmem.c:68: include/linux/swapops.h: At top level: >> include/linux/swapops.h:107:23: error: conflicting types for 'swp_offset'; have 'long unsigned int(swp_entry_t)' 107 | static inline pgoff_t swp_offset(swp_entry_t entry) | ^~~~~~~~~~ mm/swap.h:12:25: note: previous implicit declaration of 'swp_offset' with type 'int()' 12 | return ((loff_t)swp_offset(entry)) << PAGE_SHIFT; | ^~~~~~~~~~ cc1: some warnings being treated as errors -- In file included from mm/show_mem.c:19: mm/swap.h: In function 'swap_file_pos': >> mm/swap.h:12:25: error: implicit declaration of function 'swp_offset'; did you mean 'pmd_offset'? [-Werror=implicit-function-declaration] 12 | return ((loff_t)swp_offset(entry)) << PAGE_SHIFT; | ^~~~~~~~~~ | pmd_offset cc1: some warnings being treated as errors vim +12 mm/swap.h 9 10 static inline loff_t swap_file_pos(swp_entry_t entry) 11 { > 12 return ((loff_t)swp_offset(entry)) << PAGE_SHIFT; 13 } 14
Kairui Song <ryncsn@gmail.com> writes: > From: Kairui Song <kasong@tencent.com> > > 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@tencent.com> > --- > 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
On Tue, Apr 23, 2024 at 9:43 AM Huang, Ying <ying.huang@intel.com> wrote: > > Kairui Song <ryncsn@gmail.com> writes: > > > From: Kairui Song <kasong@tencent.com> > > > > 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@tencent.com> > > --- > > 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. Good suggestion! The definition looks good to me, swap_dev_pos also looks better, "swap_file" looks confusing indeed. > > 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
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;