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

[Hannes Reinecke <hare@suse.de>]

On 5/11/24 00:19, Matthew Wilcox wrote:
> 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?
> 
Aw. No. MIssed that line.

Will be fixing it up.


>> @@ -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.
> 

Thanks for the review.
I'll be sure to pester you next week :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich