Re: [PATCH 1/5] fs/mpage: use blocks_per_folio instead of blocks_per_page

[Matthew Wilcox <willy@infradead.org>]

On Fri, May 10, 2024 at 12:29:02PM +0200, hare@kernel.org wrote:
> +++ b/fs/mpage.c
> @@ -114,7 +114,7 @@ static void map_buffer_to_folio(struct folio *folio, struct buffer_head *bh,
>  		 * don't make any buffers if there is only one buffer on
>  		 * the folio and the folio just needs to be set up to date
>  		 */
> -		if (inode->i_blkbits == PAGE_SHIFT &&
> +		if (inode->i_blkbits == folio_shift(folio) &&

yes

> @@ -160,7 +160,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
>  	struct folio *folio = args->folio;
>  	struct inode *inode = folio->mapping->host;
>  	const unsigned blkbits = inode->i_blkbits;
> -	const unsigned blocks_per_page = PAGE_SIZE >> blkbits;
> +	const unsigned blocks_per_folio = folio_size(folio) >> blkbits;
>  	const unsigned blocksize = 1 << blkbits;
>  	struct buffer_head *map_bh = &args->map_bh;
>  	sector_t block_in_file;
> @@ -168,7 +168,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
>  	sector_t last_block_in_file;
>  	sector_t first_block;
>  	unsigned page_block;
> -	unsigned first_hole = blocks_per_page;
> +	unsigned first_hole = blocks_per_folio;

yes

> @@ -189,7 +189,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
>  		goto confused;
>  
>  	block_in_file = (sector_t)folio->index << (PAGE_SHIFT - blkbits);
> -	last_block = block_in_file + args->nr_pages * blocks_per_page;
> +	last_block = block_in_file + args->nr_pages * blocks_per_folio;

no.  args->nr_pages really is the number of pages, so last_block is
block_in_file + nr_pages * blocks_per_page.  except that blocks_per_page
might now be 0.

so i think this needs to be rewritten as:

	last_block = block_in_file + (args->nr_pages * PAGE_SIZE) >> blkbits;

or have i confused myself?

> @@ -275,7 +275,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
>  		bdev = map_bh->b_bdev;
>  	}
>  
> -	if (first_hole != blocks_per_page) {
> +	if (first_hole != blocks_per_folio) {
>  		folio_zero_segment(folio, first_hole << blkbits, PAGE_SIZE);

... doesn't that need to be folio_size(folio)?

there may be other problems, but let's settle these questions first.