| Message ID | 20200507075100.1779-7-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clean up SECTOR related macros and sectors/pages conversions | expand |
On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote: > @@ -266,7 +266,7 @@ int swap_writepage(struct page *page, struct writeback_control *wbc) > > static sector_t swap_page_sector(struct page *page) > { > - return (sector_t)__page_file_index(page) << (PAGE_SHIFT - 9); > + return npage_to_sectors((sector_t)__page_file_index(page)); If you make npage_to_sectors() a proper function instead of a macro, you can do the casting inside the function instead of in the callers (which is prone to bugs). Also, this is a great example of why page_to_sector() was a better name than npage_to_sectors(). This function doesn't return a count of sectors, it returns a sector number within the swap device.
On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote: > +++ b/mm/page_io.c > @@ -38,7 +38,7 @@ static struct bio *get_swap_bio(gfp_t gfp_flags, > > bio->bi_iter.bi_sector = map_swap_page(page, &bdev); > bio_set_dev(bio, bdev); > - bio->bi_iter.bi_sector <<= PAGE_SHIFT - 9; > + bio->bi_iter.bi_sector *= PAGE_SECTORS; > bio->bi_end_io = end_io; This just doesn't look right. Why is map_swap_page() returning a sector_t which isn't actually a sector_t?
On 2020/5/15 12:06, Matthew Wilcox wrote: > On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote: >> @@ -266,7 +266,7 @@ int swap_writepage(struct page *page, struct writeback_control *wbc) >> >> static sector_t swap_page_sector(struct page *page) >> { >> - return (sector_t)__page_file_index(page) << (PAGE_SHIFT - 9); >> + return npage_to_sectors((sector_t)__page_file_index(page)); > > If you make npage_to_sectors() a proper function instead of a macro, > you can do the casting inside the function instead of in the callers > (which is prone to bugs). Oh, yes. __page_file_index(page) maybe called many times in marco, althouth currently it is not. So that, not all are suitable for page_to_sector(). And for this example, still need to use "<< PAGE_SECTORS_SHIFT". > > Also, this is a great example of why page_to_sector() was a better name > than npage_to_sectors(). This function doesn't return a count of sectors, > it returns a sector number within the swap device. OK, so I will change to page_to_sector()/sector_to_page(). > > > . >
On 2020/5/15 12:14, Matthew Wilcox wrote: > On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote: >> +++ b/mm/page_io.c >> @@ -38,7 +38,7 @@ static struct bio *get_swap_bio(gfp_t gfp_flags, >> >> bio->bi_iter.bi_sector = map_swap_page(page, &bdev); >> bio_set_dev(bio, bdev); >> - bio->bi_iter.bi_sector <<= PAGE_SHIFT - 9; >> + bio->bi_iter.bi_sector *= PAGE_SECTORS; >> bio->bi_end_io = end_io; > > This just doesn't look right. Why is map_swap_page() returning a sector_t > which isn't actually a sector_t? I try to understand map_swap_page(). Here maybe a bug. Otherwise, it would be better to add a temporary variable to cache the return value of map_swap_page(page, &bdev). > > > . >
diff --git a/mm/page_io.c b/mm/page_io.c index 76965be1d40e..23291a49ab91 100644 --- a/mm/page_io.c +++ b/mm/page_io.c @@ -38,7 +38,7 @@ static struct bio *get_swap_bio(gfp_t gfp_flags, bio->bi_iter.bi_sector = map_swap_page(page, &bdev); bio_set_dev(bio, bdev); - bio->bi_iter.bi_sector <<= PAGE_SHIFT - 9; + bio->bi_iter.bi_sector *= PAGE_SECTORS; bio->bi_end_io = end_io; bio_add_page(bio, page, PAGE_SIZE * hpage_nr_pages(page), 0); @@ -266,7 +266,7 @@ int swap_writepage(struct page *page, struct writeback_control *wbc) static sector_t swap_page_sector(struct page *page) { - return (sector_t)__page_file_index(page) << (PAGE_SHIFT - 9); + return npage_to_sectors((sector_t)__page_file_index(page)); } static inline void count_swpout_vm_event(struct page *page) diff --git a/mm/swapfile.c b/mm/swapfile.c index 5871a2aa86a5..c8be92f972a4 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -177,8 +177,8 @@ static int discard_swap(struct swap_info_struct *si) /* Do not discard the swap header page! */ se = first_se(si); - start_block = (se->start_block + 1) << (PAGE_SHIFT - 9); - nr_blocks = ((sector_t)se->nr_pages - 1) << (PAGE_SHIFT - 9); + start_block = npage_to_sectors(se->start_block + 1); + nr_blocks = npage_to_sectors((sector_t)se->nr_pages - 1); if (nr_blocks) { err = blkdev_issue_discard(si->bdev, start_block, nr_blocks, GFP_KERNEL, 0); @@ -188,8 +188,8 @@ static int discard_swap(struct swap_info_struct *si) } for (se = next_se(se); se; se = next_se(se)) { - start_block = se->start_block << (PAGE_SHIFT - 9); - nr_blocks = (sector_t)se->nr_pages << (PAGE_SHIFT - 9); + start_block = npage_to_sectors(se->start_block); + nr_blocks = npage_to_sectors((sector_t)se->nr_pages); err = blkdev_issue_discard(si->bdev, start_block, nr_blocks, GFP_KERNEL, 0); @@ -240,8 +240,8 @@ static void discard_swap_cluster(struct swap_info_struct *si, start_page += nr_blocks; nr_pages -= nr_blocks; - start_block <<= PAGE_SHIFT - 9; - nr_blocks <<= PAGE_SHIFT - 9; + start_block *= PAGE_SECTORS; + nr_blocks *= PAGE_SECTORS; if (blkdev_issue_discard(si->bdev, start_block, nr_blocks, GFP_NOIO, 0)) break;
1. Replace "<<= (PAGE_SHIFT - 9)" with "*= PAGE_SECTORS" 2. Replace "<< (PAGE_SHIFT - 9)" with npage_to_sectors() Suggested-by: Matthew Wilcox <willy@infradead.org> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- mm/page_io.c | 4 ++-- mm/swapfile.c | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-)