Re: [PATCH 02/18] fs/mpage: use blocks_per_folio instead of blocks_per_page

[Hannes Reinecke <hare@suse.de>]

On 9/18/23 15:15, Matthew Wilcox wrote:
> On Mon, Sep 18, 2023 at 01:04:54PM +0200, Hannes Reinecke wrote:
>> @@ -161,7 +161,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;
>> @@ -169,7 +169,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
>>   	sector_t last_block_in_file;
>>   	sector_t blocks[MAX_BUF_PER_PAGE];
>>   	unsigned page_block;
>> -	unsigned first_hole = blocks_per_page;
>> +	unsigned first_hole = blocks_per_folio;
>>   	struct block_device *bdev = NULL;
>>   	int length;
>>   	int fully_mapped = 1;
> 
> I feel like we need an assertion that blocks_per_folio <=
> MAX_BUF_PER_PAGE.  Otherwise this function runs off the end of the
> 'blocks' array.
> 
> Or (and I tried to do this once before getting bogged down), change this
> function to not need the blocks array.  We need (first_block, length),
> because this function will punt to 'confused' if theree is an on-disk
> discontiguity.
> 
> ie this line needs to change:
> 
> -		if (page_block && blocks[page_block-1] != map_bh->b_blocknr-1)
> +		if (page_block == 0)
> +			first_block = map_bh->b_blocknr;
> +		else if (first_block + page_block != map_bh->b_blocknr)
> 			goto confused;
> 
>> @@ -474,12 +474,12 @@ static int __mpage_writepage(struct folio *folio, struct writeback_control *wbc,
>>   	struct address_space *mapping = folio->mapping;
>>   	struct inode *inode = 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;
>>   	sector_t last_block;
>>   	sector_t block_in_file;
>>   	sector_t blocks[MAX_BUF_PER_PAGE];
>>   	unsigned page_block;
>> -	unsigned first_unmapped = blocks_per_page;
>> +	unsigned first_unmapped = blocks_per_folio;
>>   	struct block_device *bdev = NULL;
>>   	int boundary = 0;
>>   	sector_t boundary_block = 0;
> 
> Similarly, this function needss an assert.  Or remove blocks[].
> 
Will have a look. Just tried the obvious conversion as I was still 
trying to get the thing to work at that point. So bear with me.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman