diff mbox series

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

Message ID 20230918110510.66470-3-hare@suse.de (mailing list archive)
State New, archived
Headers show
Series block: update buffer_head for Large-block I/O | expand

Commit Message

Hannes Reinecke Sept. 18, 2023, 11:04 a.m. UTC 
Convert mpage to folios and associate the number of blocks with
a folio and not a page.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 fs/mpage.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

Comments

Matthew Wilcox Sept. 18, 2023, 1:15 p.m. UTC | #1 
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[].
Hannes Reinecke Sept. 18, 2023, 5:45 p.m. UTC | #2 
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
diff mbox series

Patch

diff --git a/fs/mpage.c b/fs/mpage.c
index 242e213ee064..c9d9fdadb500 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -114,12 +114,12 @@  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) &&
 		    buffer_uptodate(bh)) {
 			folio_mark_uptodate(folio);
 			return;
 		}
-		create_empty_buffers(&folio->page, i_blocksize(inode), 0);
+		folio_create_empty_buffers(folio, i_blocksize(inode), 0);
 		head = folio_buffers(folio);
 	}
 
@@ -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;
@@ -190,7 +190,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;
 	last_block_in_file = (i_size_read(inode) + blocksize - 1) >> blkbits;
 	if (last_block > last_block_in_file)
 		last_block = last_block_in_file;
@@ -211,7 +211,7 @@  static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 				clear_buffer_mapped(map_bh);
 				break;
 			}
-			if (page_block == blocks_per_page)
+			if (page_block == blocks_per_folio)
 				break;
 			blocks[page_block] = map_bh->b_blocknr + map_offset +
 						relative_block;
@@ -225,7 +225,7 @@  static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 	 * Then do more get_blocks calls until we are done with this folio.
 	 */
 	map_bh->b_folio = folio;
-	while (page_block < blocks_per_page) {
+	while (page_block < blocks_per_folio) {
 		map_bh->b_state = 0;
 		map_bh->b_size = 0;
 
@@ -238,7 +238,7 @@  static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 
 		if (!buffer_mapped(map_bh)) {
 			fully_mapped = 0;
-			if (first_hole == blocks_per_page)
+			if (first_hole == blocks_per_folio)
 				first_hole = page_block;
 			page_block++;
 			block_in_file++;
@@ -256,7 +256,7 @@  static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 			goto confused;
 		}
 	
-		if (first_hole != blocks_per_page)
+		if (first_hole != blocks_per_folio)
 			goto confused;		/* hole -> non-hole */
 
 		/* Contiguous blocks? */
@@ -267,7 +267,7 @@  static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 			if (relative_block == nblocks) {
 				clear_buffer_mapped(map_bh);
 				break;
-			} else if (page_block == blocks_per_page)
+			} else if (page_block == blocks_per_folio)
 				break;
 			blocks[page_block] = map_bh->b_blocknr+relative_block;
 			page_block++;
@@ -276,7 +276,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);
 		if (first_hole == 0) {
 			folio_mark_uptodate(folio);
@@ -311,10 +311,10 @@  static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 	relative_block = block_in_file - args->first_logical_block;
 	nblocks = map_bh->b_size >> blkbits;
 	if ((buffer_boundary(map_bh) && relative_block == nblocks) ||
-	    (first_hole != blocks_per_page))
+	    (first_hole != blocks_per_folio))
 		args->bio = mpage_bio_submit_read(args->bio);
 	else
-		args->last_block_in_bio = blocks[blocks_per_page - 1];
+		args->last_block_in_bio = blocks[blocks_per_folio - 1];
 out:
 	return args->bio;
 
@@ -393,7 +393,7 @@  int mpage_read_folio(struct folio *folio, get_block_t get_block)
 {
 	struct mpage_readpage_args args = {
 		.folio = folio,
-		.nr_pages = 1,
+		.nr_pages = folio_nr_pages(folio),
 		.get_block = get_block,
 	};
 
@@ -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;
@@ -504,12 +504,12 @@  static int __mpage_writepage(struct folio *folio, struct writeback_control *wbc,
 				 */
 				if (buffer_dirty(bh))
 					goto confused;
-				if (first_unmapped == blocks_per_page)
+				if (first_unmapped == blocks_per_folio)
 					first_unmapped = page_block;
 				continue;
 			}
 
-			if (first_unmapped != blocks_per_page)
+			if (first_unmapped != blocks_per_folio)
 				goto confused;	/* hole -> non-hole */
 
 			if (!buffer_dirty(bh) || !buffer_uptodate(bh))
@@ -552,7 +552,7 @@  static int __mpage_writepage(struct folio *folio, struct writeback_control *wbc,
 		goto page_is_mapped;
 	last_block = (i_size - 1) >> blkbits;
 	map_bh.b_folio = folio;
-	for (page_block = 0; page_block < blocks_per_page; ) {
+	for (page_block = 0; page_block < blocks_per_folio; ) {
 
 		map_bh.b_state = 0;
 		map_bh.b_size = 1 << blkbits;
@@ -631,14 +631,14 @@  static int __mpage_writepage(struct folio *folio, struct writeback_control *wbc,
 	BUG_ON(folio_test_writeback(folio));
 	folio_start_writeback(folio);
 	folio_unlock(folio);
-	if (boundary || (first_unmapped != blocks_per_page)) {
+	if (boundary || (first_unmapped != blocks_per_folio)) {
 		bio = mpage_bio_submit_write(bio);
 		if (boundary_block) {
 			write_boundary_block(boundary_bdev,
 					boundary_block, 1 << blkbits);
 		}
 	} else {
-		mpd->last_block_in_bio = blocks[blocks_per_page - 1];
+		mpd->last_block_in_bio = blocks[blocks_per_folio - 1];
 	}
 	goto out;