do_mpage_readpage(): remove first_logical_block parameter

PATCH In review Franck Bui-Huu 2 participants 7 messages Nov 25, 2008 – Nov 30, 2008 VFS

lore

Share & tools

lore

Franck Bui-Huu

author

2008-11-25 20:29 UTC #0 lore raw

PATCH 1/1 do_mpage_readpage(): remove first_logical_block parameter

From: Franck Bui-Huu <[email protected]>

This patch makes this function more readable IMHO by getting rid of its parameter 'first_logical_block'.

This parameter was previously used to remember the original block number in the file (ie the page index) that 'map_bh' was pointing at first. Indeed this was needed since 'map_bh->page' was modified even if 'map_bh' maps the whole current page (IOW when get_block() is not used). This had the bad effect to make the state of 'map_bh' inconsistent too.

This patch changes 'map_bh->page' only when get_block() is called hence removes the need of the 'first_logical_block' argument.

The value stored in 'logical_block_parameter' is now recalculated each time do_mpage_readpage() but it shouldn't matter since the operation is trivial.

Signed-off-by Franck Bui-Huu <[email protected]>

---

Andrew,

I made this when trying to understand this function. IMHO it makes the code more readable.

I still don't know why 'map_bh->page' was setup every time do_mpage_readpage() is called, but I'm just discovering this code, so...

Franck

 fs/mpage.c |   57 ++++++++++++++++++++++++++++-----------------------------
 1 files changed, 28 insertions(+), 29 deletions(-)

fs/mpage.c +28 ~~-29~~

index cf05ca7..da4d66f 100644
@@ -168,7 +168,7 @@ map_buffer_to_page(struct page *page, struct buffer_head *bh, int page_block)
 static struct bio *
 do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 		sector_t *last_block_in_bio, struct buffer_head *map_bh,
-		unsigned long *first_logical_block, get_block_t get_block)
+		get_block_t get_block)
 {
 	struct inode *inode = page->mapping->host;
 	const unsigned blkbits = inode->i_blkbits;

@@ -184,7 +184,7 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 	int length;
 	int fully_mapped = 1;
 	unsigned nblocks;
-	unsigned relative_block;
+	unsigned i;
 
 	if (page_has_buffers(page))
 		goto confused;

@@ -199,40 +199,42 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 	/*
 	 * Map blocks using the result from the previous get_blocks call first.
 	 */
-	nblocks = map_bh->b_size >> blkbits;
-	if (buffer_mapped(map_bh) && block_in_file > *first_logical_block &&
-			block_in_file < (*first_logical_block + nblocks)) {
-		unsigned map_offset = block_in_file - *first_logical_block;
-		unsigned last = nblocks - map_offset;
-
-		for (relative_block = 0; ; relative_block++) {
-			if (relative_block == last) {
-				clear_buffer_mapped(map_bh);
-				break;
+	if (buffer_mapped(map_bh)) {
+		sector_t map_bk = map_bh->b_page->index << \
+					(PAGE_CACHE_SHIFT - blkbits);
+
+		nblocks = map_bh->b_size >> blkbits;
+		if (block_in_file > map_bk && block_in_file < map_bk + nblocks) {
+			unsigned map_offset = block_in_file - map_bk;
+			sector_t map_blocknr = map_bh->b_blocknr;
+
+			for (i = map_offset; ; i++) {
+				if (i == nblocks) {
+					clear_buffer_mapped(map_bh);
+					break;
+				}
+				if (page_block == blocks_per_page)
+					break;
+				blocks[page_block] = map_blocknr + i;
+				page_block++;
+				block_in_file++;
 			}
-			if (page_block == blocks_per_page)
-				break;
-			blocks[page_block] = map_bh->b_blocknr + map_offset +
-						relative_block;
-			page_block++;
-			block_in_file++;
+			bdev = map_bh->b_bdev;
 		}
-		bdev = map_bh->b_bdev;
 	}
 
 	/*
 	 * Then do more get_blocks calls until we are done with this page.
 	 */
-	map_bh->b_page = page;
 	while (page_block < blocks_per_page) {
 		map_bh->b_state = 0;
 		map_bh->b_size = 0;
 
 		if (block_in_file < last_block) {
+			map_bh->b_page = page;
 			map_bh->b_size = (last_block-block_in_file) << blkbits;
 			if (get_block(inode, block_in_file, map_bh, 0))
 				goto confused;
-			*first_logical_block = block_in_file;
 		}
 
 		if (!buffer_mapped(map_bh)) {

@@ -254,7 +256,7 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 			map_buffer_to_page(page, map_bh, page_block);
 			goto confused;
 		}
-	
+
 		if (first_hole != blocks_per_page)
 			goto confused;		/* hole -> non-hole */

@@ -262,13 +264,13 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 		if (page_block && blocks[page_block-1] != map_bh->b_blocknr-1)
 			goto confused;
 		nblocks = map_bh->b_size >> blkbits;
-		for (relative_block = 0; ; relative_block++) {
-			if (relative_block == nblocks) {
+		for (i = 0; ; i++) {
+			if (i == nblocks) {
 				clear_buffer_mapped(map_bh);
 				break;
 			} else if (page_block == blocks_per_page)
 				break;
-			blocks[page_block] = map_bh->b_blocknr+relative_block;
+			blocks[page_block] = map_bh->b_blocknr + i;
 			page_block++;
 			block_in_file++;
 		}

@@ -375,7 +377,6 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 	unsigned page_idx;
 	sector_t last_block_in_bio = 0;
 	struct buffer_head map_bh;
-	unsigned long first_logical_block = 0;
 
 	clear_buffer_mapped(&map_bh);
 	for (page_idx = 0; page_idx < nr_pages; page_idx++) {

@@ -388,7 +389,6 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 			bio = do_mpage_readpage(bio, page,
 					nr_pages - page_idx,
 					&last_block_in_bio, &map_bh,
-					&first_logical_block,
 					get_block);
 		}
 		page_cache_release(page);

@@ -408,11 +408,10 @@ int mpage_readpage(struct page *page, get_block_t get_block)
 	struct bio *bio = NULL;
 	sector_t last_block_in_bio = 0;
 	struct buffer_head map_bh;
-	unsigned long first_logical_block = 0;
 
 	clear_buffer_mapped(&map_bh);
 	bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio,
-			&map_bh, &first_logical_block, get_block);
+			&map_bh, get_block);
 	if (bio)
 		mpage_bio_submit(READ, bio);
 	return 0;

Signature · 1 line

1.6.0.2.GIT

Andrew Morton

2008-11-26 00:48 UTC #1 lore raw

Quoted 12 lines · On Tue, 25 Nov 2008 21:29:11 +0100 Franck Bui-Huu <[email protected]> wrote:

From: Franck Bui-Huu <[email protected]>
This patch makes this function more readable IMHO by getting rid of its parameter 'first_logical_block'.
This parameter was previously used to remember the original block number in the file (ie the page index) that 'map_bh' was pointing at first. Indeed this was needed since 'map_bh->page' was modified even if 'map_bh' maps the whole current page (IOW when get_block() is not used). This had the bad effect to make the state of 'map_bh' inconsistent too.

Quoted 7 lines

This patch changes 'map_bh->page' only when get_block() is called hence removes the need of the 'first_logical_block' argument.
The value stored in 'logical_block_parameter' is now recalculated each time do_mpage_readpage() but it shouldn't matter since the operation is trivial.

umm, ok, it seems a bit simpler.

Quoted 14 lines

fs/mpage.c +1 -1

index cf05ca7..da4d66f 100644
@@ -168,7 +168,7 @@ map_buffer_to_page(struct page *page, struct buffer_head *bh, int page_block)
 static struct bio *
 do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 		sector_t *last_block_in_bio, struct buffer_head *map_bh,
-		unsigned long *first_logical_block, get_block_t get_block)
+		get_block_t get_block)
 {
 	struct inode *inode = page->mapping->host;
 	const unsigned blkbits = inode->i_blkbits;

fs/mpage.c do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, · line 187

Quoted 5 lines

fs/mpage.c +1 -1

@@ -184,7 +184,7 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 	int length;
 	int fully_mapped = 1;
 	unsigned nblocks;
-	unsigned relative_block;
+	unsigned i;

This function is really complicated, and adding a variable called `i' doesn't help. I suggest that you think up a better name. A good name would include the text "blocks" (or whatever) which communicates the units which this variable contains.

These pagecache functions tend to have a mixture of sector numbers, page numbers, block-within-page numbers, file offsets, etc, etc. They get quite confusing and very careful choice of identifiers will help.

Quoted 9 lines

fs/mpage.c +0 -3

 	if (page_has_buffers(page))
 		goto confused;

@@ -199,40 +199,42 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 	/*
 	 * Map blocks using the result from the previous get_blocks call first.
 	 */
-	nblocks = map_bh->b_size >> blkbits;
-	if (buffer_mapped(map_bh) && block_in_file > *first_logical_block &&
-			block_in_file < (*first_logical_block + nblocks)) {

fs/mpage.c do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, · line 204

Quoted 10 lines

fs/mpage.c +3 -7

-		unsigned map_offset = block_in_file - *first_logical_block;
-		unsigned last = nblocks - map_offset;
-
-		for (relative_block = 0; ; relative_block++) {
-			if (relative_block == last) {
-				clear_buffer_mapped(map_bh);
-				break;
+	if (buffer_mapped(map_bh)) {
+		sector_t map_bk = map_bh->b_page->index << \
+					(PAGE_CACHE_SHIFT - blkbits);

The \ is unneeded and just adds noise.

This change adds a very bad bug. map_bh->b_page->index is 32-bit and the left shift can cause us to lose the uppermost bits.

A suitable fix would be to cast ->index to u64. A better fix, which is more efficient when sizeof(sector_t)=4 is to cast ->index to a sector_t.

There is no precendent for abbreviating "block" to "bk", so that abbreviation doesn't help readers much. "map_block" would be a more typical identifier.

fs/mpage.c do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, · line 208

Quoted 4 lines

fs/mpage.c +4 -0

+
+		nblocks = map_bh->b_size >> blkbits;
+		if (block_in_file > map_bk && block_in_file < map_bk + nblocks) {
+			unsigned map_offset = block_in_file - map_bk;

So map_offset has units "blocks". It would be better if its name were to reflect that, rather than the dimensionless "offset", which could refer to page indexes, byte offsets, whatever.

Quoted 96 lines

fs/mpage.c +19 ~~-17~~

+			sector_t map_blocknr = map_bh->b_blocknr;
+
+			for (i = map_offset; ; i++) {
+				if (i == nblocks) {
+					clear_buffer_mapped(map_bh);
+					break;
+				}
+				if (page_block == blocks_per_page)
+					break;
+				blocks[page_block] = map_blocknr + i;
+				page_block++;
+				block_in_file++;
 			}
-			if (page_block == blocks_per_page)
-				break;
-			blocks[page_block] = map_bh->b_blocknr + map_offset +
-						relative_block;
-			page_block++;
-			block_in_file++;
+			bdev = map_bh->b_bdev;
 		}
-		bdev = map_bh->b_bdev;
 	}
 
 	/*
 	 * Then do more get_blocks calls until we are done with this page.
 	 */
-	map_bh->b_page = page;
 	while (page_block < blocks_per_page) {
 		map_bh->b_state = 0;
 		map_bh->b_size = 0;
 
 		if (block_in_file < last_block) {
+			map_bh->b_page = page;
 			map_bh->b_size = (last_block-block_in_file) << blkbits;
 			if (get_block(inode, block_in_file, map_bh, 0))
 				goto confused;
-			*first_logical_block = block_in_file;
 		}
 
 		if (!buffer_mapped(map_bh)) {

@@ -254,7 +256,7 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 			map_buffer_to_page(page, map_bh, page_block);
 			goto confused;
 		}
-	
+
 		if (first_hole != blocks_per_page)
 			goto confused;		/* hole -> non-hole */

@@ -262,13 +264,13 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 		if (page_block && blocks[page_block-1] != map_bh->b_blocknr-1)
 			goto confused;
 		nblocks = map_bh->b_size >> blkbits;
-		for (relative_block = 0; ; relative_block++) {
-			if (relative_block == nblocks) {
+		for (i = 0; ; i++) {
+			if (i == nblocks) {
 				clear_buffer_mapped(map_bh);
 				break;
 			} else if (page_block == blocks_per_page)
 				break;
-			blocks[page_block] = map_bh->b_blocknr+relative_block;
+			blocks[page_block] = map_bh->b_blocknr + i;
 			page_block++;
 			block_in_file++;
 		}

@@ -375,7 +377,6 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 	unsigned page_idx;
 	sector_t last_block_in_bio = 0;
 	struct buffer_head map_bh;
-	unsigned long first_logical_block = 0;
 
 	clear_buffer_mapped(&map_bh);
 	for (page_idx = 0; page_idx < nr_pages; page_idx++) {

@@ -388,7 +389,6 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 			bio = do_mpage_readpage(bio, page,
 					nr_pages - page_idx,
 					&last_block_in_bio, &map_bh,
-					&first_logical_block,
 					get_block);
 		}
 		page_cache_release(page);

@@ -408,11 +408,10 @@ int mpage_readpage(struct page *page, get_block_t get_block)
 	struct bio *bio = NULL;
 	sector_t last_block_in_bio = 0;
 	struct buffer_head map_bh;
-	unsigned long first_logical_block = 0;
 
 	clear_buffer_mapped(&map_bh);
 	bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio,
-			&map_bh, &first_logical_block, get_block);
+			&map_bh, get_block);
 	if (bio)
 		mpage_bio_submit(READ, bio);
 	return 0;

Franck Bui-Huu

author

2008-11-26 08:03 UTC #2 lore raw

Quoted 16 lines · Andrew Morton

Quoted 16 lines

fs/mpage.c +1 -1

index cf05ca7..da4d66f 100644
@@ -168,7 +168,7 @@ map_buffer_to_page(struct page *page, struct buffer_head *bh, int page_block)
 static struct bio *
 do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 		sector_t *last_block_in_bio, struct buffer_head *map_bh,
-		unsigned long *first_logical_block, get_block_t get_block)
+		get_block_t get_block)
 {
 	struct inode *inode = page->mapping->host;
 	const unsigned blkbits = inode->i_blkbits;

@@ -184,7 +184,7 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 	int length;
 	int fully_mapped = 1;

Quoted 8 lines

fs/mpage.c do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, · line 187
Quoted 3 lines
fs/mpage.c +1 -1
 	unsigned nblocks;
-	unsigned relative_block;
+	unsigned i;
This function is really complicated, and adding a variable called `i' doesn't help. I suggest that you think up a better name. A good name would include the text "blocks" (or whatever) which communicates the units which this variable contains.

Yes, I was also wondering about this change too.

But I found 'relative_block' name quite global for a local variable and I found myself asking several times where this local is actually used to eventually see it's only used by the two for loops.

Perhaps moving the declaration close to the loops would help ?

if (block_in_file > map_bk && block_in_file < map_bk + nblocks) {
	unsigned map_offset = block_in_file - map_bk;
	sector_t map_blocknr = map_bh->b_blocknr;

+ unsigned i;

	for (i = map_offset; ; i++) {
		...
		blocks[page_block] = map_blocknr + i;
		...
	}

Or perhaps both, move the declaration and rename...

Quoted 5 lines

These pagecache functions tend to have a mixture of sector numbers, page numbers, block-within-page numbers, file offsets, etc, etc. They get quite confusing and very careful choice of identifiers will help.

I agree.

Quoted 22 lines

Quoted 9 lines

fs/mpage.c +0 -3

 	if (page_has_buffers(page))
 		goto confused;

@@ -199,40 +199,42 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 	/*
 	 * Map blocks using the result from the previous get_blocks call first.
 	 */
-	nblocks = map_bh->b_size >> blkbits;
-	if (buffer_mapped(map_bh) && block_in_file > *first_logical_block &&
-			block_in_file < (*first_logical_block + nblocks)) {

fs/mpage.c do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, · line 204

Quoted 10 lines

fs/mpage.c +3 -7

-		unsigned map_offset = block_in_file - *first_logical_block;
-		unsigned last = nblocks - map_offset;
-
-		for (relative_block = 0; ; relative_block++) {
-			if (relative_block == last) {
-				clear_buffer_mapped(map_bh);
-				break;
+	if (buffer_mapped(map_bh)) {
+		sector_t map_bk = map_bh->b_page->index << \
+					(PAGE_CACHE_SHIFT - blkbits);

The \ is unneeded and just adds noise.

Quoted 3 lines

This change adds a very bad bug. map_bh->b_page->index is 32-bit and the left shift can cause us to lose the uppermost bits.

Ouch, good catch !

I did test this patch several days on my laptop without hitting this bug unfortunately... probably because it's a 64 bits machine.

Quoted 3 lines

A suitable fix would be to cast ->index to u64. A better fix, which is more efficient when sizeof(sector_t)=4 is to cast ->index to a sector_t.

I'll do that.

Quoted 5 lines

There is no precendent for abbreviating "block" to "bk", so that abbreviation doesn't help readers much. "map_block" would be a more typical identifier.

OK.

Quoted 8 lines

fs/mpage.c do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, · line 208
Quoted 4 lines
fs/mpage.c +4 -0
+
+		nblocks = map_bh->b_size >> blkbits;
+		if (block_in_file > map_bk && block_in_file < map_bk + nblocks) {
+			unsigned map_offset = block_in_file - map_bk;
So map_offset has units "blocks". It would be better if its name were to reflect that, rather than the dimensionless "offset", which could refer to page indexes, byte offsets, whatever.

Right, since this variable is very local to the if block, I thought that would be ok... I'll change this too.

Thanks for your comments, I'll cook up a new patch.

Franck

Franck Bui-Huu

author

2008-11-26 20:05 UTC #3 lore raw

PATCH 1/1 do_mpage_readpage(): remove first_logical_block parameter

In reply to Andrew Morton

From: Franck Bui-Huu <[email protected]>

This patch makes this function more readable IMHO by getting rid of its parameter 'first_logical_block'.

This patch changes 'map_bh->page' only when get_block() is called hence removes the need of the 'first_logical_block' argument.

The value stored in 'logical_block_parameter' is now recalculated each time do_mpage_readpage() but it shouldn't matter since the operation is trivial.

While at it, this patch does a few extra cleanups such as removing a trailing space, moving 2 declarations of local variables closer to the place they're used.

Signed-off-by Franck Bui-Huu <[email protected]>

---
Andrew,

I tried to address all points you raised.

The most important was the bug introduced by the first version that you catched.

The second one was to find some good names for some locals. I also moved their statements closer to the place they're used.

Franck

 fs/mpage.c |   64 ++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 33 insertions(+), 31 deletions(-)

fs/mpage.c +33 ~~-31~~

index cf05ca7..a041a0e 100644
@@ -168,7 +168,7 @@ map_buffer_to_page(struct page *page, struct buffer_head *bh, int page_block)
 static struct bio *
 do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 		sector_t *last_block_in_bio, struct buffer_head *map_bh,
-		unsigned long *first_logical_block, get_block_t get_block)
+		get_block_t get_block)
 {
 	struct inode *inode = page->mapping->host;
 	const unsigned blkbits = inode->i_blkbits;

@@ -183,8 +183,6 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 	struct block_device *bdev = NULL;
 	int length;
 	int fully_mapped = 1;
-	unsigned nblocks;
-	unsigned relative_block;
 
 	if (page_has_buffers(page))
 		goto confused;

@@ -199,40 +197,45 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 	/*
 	 * Map blocks using the result from the previous get_blocks call first.
 	 */
-	nblocks = map_bh->b_size >> blkbits;
-	if (buffer_mapped(map_bh) && block_in_file > *first_logical_block &&
-			block_in_file < (*first_logical_block + nblocks)) {
-		unsigned map_offset = block_in_file - *first_logical_block;
-		unsigned last = nblocks - map_offset;
-
-		for (relative_block = 0; ; relative_block++) {
-			if (relative_block == last) {
-				clear_buffer_mapped(map_bh);
-				break;
+	if (buffer_mapped(map_bh)) {
+		unsigned nblocks = map_bh->b_size >> blkbits;
+		sector_t map_block = (sector_t)map_bh->b_page->index <<
+						(PAGE_CACHE_SHIFT - blkbits);
+
+		if (block_in_file > map_block &&
+		    block_in_file < map_block + nblocks) {
+			unsigned blocknr_off = block_in_file - map_block;
+			sector_t map_blocknr = map_bh->b_blocknr;
+
+			for (;; blocknr_off++) {
+				if (blocknr_off == nblocks) {
+					clear_buffer_mapped(map_bh);
+					break;
+				}
+				if (page_block == blocks_per_page)
+					break;
+				blocks[page_block] = map_blocknr + blocknr_off;
+				page_block++;
+				block_in_file++;
 			}
-			if (page_block == blocks_per_page)
-				break;
-			blocks[page_block] = map_bh->b_blocknr + map_offset +
-						relative_block;
-			page_block++;
-			block_in_file++;
+			bdev = map_bh->b_bdev;
 		}
-		bdev = map_bh->b_bdev;
 	}
 
 	/*
 	 * Then do more get_blocks calls until we are done with this page.
 	 */
-	map_bh->b_page = page;
 	while (page_block < blocks_per_page) {
+		unsigned nblocks, blocknr_off = 0;
+
 		map_bh->b_state = 0;
 		map_bh->b_size = 0;
 
 		if (block_in_file < last_block) {
+			map_bh->b_page = page;
 			map_bh->b_size = (last_block-block_in_file) << blkbits;
 			if (get_block(inode, block_in_file, map_bh, 0))
 				goto confused;
-			*first_logical_block = block_in_file;
 		}
 
 		if (!buffer_mapped(map_bh)) {

@@ -254,21 +257,23 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 			map_buffer_to_page(page, map_bh, page_block);
 			goto confused;
 		}
-	
+
 		if (first_hole != blocks_per_page)
 			goto confused;		/* hole -> non-hole */
 
 		/* Contiguous blocks? */
 		if (page_block && blocks[page_block-1] != map_bh->b_blocknr-1)
 			goto confused;
+
 		nblocks = map_bh->b_size >> blkbits;
-		for (relative_block = 0; ; relative_block++) {
-			if (relative_block == nblocks) {
+		for (;; blocknr_off++) {
+			if (blocknr_off == nblocks) {
 				clear_buffer_mapped(map_bh);
 				break;
-			} else if (page_block == blocks_per_page)
+			}
+			if (page_block == blocks_per_page)
 				break;
-			blocks[page_block] = map_bh->b_blocknr+relative_block;
+			blocks[page_block] = map_bh->b_blocknr + blocknr_off;
 			page_block++;
 			block_in_file++;
 		}

@@ -375,7 +380,6 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 	unsigned page_idx;
 	sector_t last_block_in_bio = 0;
 	struct buffer_head map_bh;
-	unsigned long first_logical_block = 0;
 
 	clear_buffer_mapped(&map_bh);
 	for (page_idx = 0; page_idx < nr_pages; page_idx++) {

@@ -388,7 +392,6 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 			bio = do_mpage_readpage(bio, page,
 					nr_pages - page_idx,
 					&last_block_in_bio, &map_bh,
-					&first_logical_block,
 					get_block);
 		}
 		page_cache_release(page);

@@ -408,11 +411,10 @@ int mpage_readpage(struct page *page, get_block_t get_block)
 	struct bio *bio = NULL;
 	sector_t last_block_in_bio = 0;
 	struct buffer_head map_bh;
-	unsigned long first_logical_block = 0;
 
 	clear_buffer_mapped(&map_bh);
 	bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio,
-			&map_bh, &first_logical_block, get_block);
+			&map_bh, get_block);
 	if (bio)
 		mpage_bio_submit(READ, bio);
 	return 0;

Signature · 1 line

1.6.0.2.GIT

Andrew Morton

2008-11-29 09:23 UTC #4 lore raw

Quoted 9 lines · On Wed, 26 Nov 2008 21:04:52 +0100 Franck Bui-Huu <[email protected]> wrote:

This patch makes this function more readable IMHO by getting rid of its parameter 'first_logical_block'.
This parameter was previously used to remember the original block number in the file (ie the page index) that 'map_bh' was pointing at first. Indeed this was needed since 'map_bh->page' was modified even if 'map_bh' maps the whole current page (IOW when get_block() is not used). This had the bad effect to make the state of 'map_bh' inconsistent too.

Unfortunately we have a conflict with the already-queued do_mpage_readpage-dont-submit-lots-of-small-bios-on-boundary.patch:

fs/mpage.c: In function 'do_mpage_readpage':
fs/mpage.c:315: error: 'relative_block' undeclared (first use in this function) fs/mpage.c:315: error: (Each undeclared identifier is reported only once fs/mpage.c:315: error: for each function it appears in.) fs/mpage.c:315: error: 'first_logical_block' undeclared (first use in this function) fs/mpage.c:316: error: 'nblocks' undeclared (first use in this function)

http://userweb.kernel.org/~akpm/mmotm/broken-out/do_mpage_readpage-dont-submit-lots-of-small-bios-on-boundary.patch

Franck Bui-Huu

author

2008-11-30 10:48 UTC #5 lore raw

Quoted 4 lines · Andrew Morton

Unfortunately we have a conflict with the already-queued do_mpage_readpage-dont-submit-lots-of-small-bios-on-boundary.patch:

How about amending this on top of take #2 ?

Franck

---8<---

fs/mpage.c +14 -4

index 74e7f83..ef26eb1 100644
@@ -157,6 +157,19 @@ map_buffer_to_page(struct page *page, struct buffer_head *bh, int page_block)
 }
 
 /*
+ * Since get_block() can map a range of pages, for all these pages the
+ * BH_Boundary flag will be set.  But we only need to push what I/O we
+ * have accumulated at the last block of this range.
+*/
+static int last_mapped_page(struct buffer_head *map_bh, struct page *page)
+{
+	unsigned map_index = map_bh->b_size >> PAGE_CACHE_SHIFT;
+
+	return buffer_boundary(map_bh) &&
+		page->index - map_bh->b_page->index >= map_index;
+}
+
+/*
  * This is the worker routine which does all the work of mapping the disk
  * blocks and constructs largest possible bios, submits them for IO if the
  * blocks are not contiguous on the disk.

@@ -312,10 +325,7 @@ alloc_new:
 		goto alloc_new;
 	}
 
-	relative_block = block_in_file - *first_logical_block;
-	nblocks = map_bh->b_size >> blkbits;
-	if ((buffer_boundary(map_bh) && relative_block == nblocks) ||
-	    (first_hole != blocks_per_page))
+	if (last_mapped_page(map_bh, page) || first_hole != blocks_per_page)
 		bio = mpage_bio_submit(READ, bio);
 	else
 		*last_block_in_bio = blocks[blocks_per_page - 1];

Franck Bui-Huu

author

2008-11-30 13:53 UTC #6 lore raw

Quoted 7 lines · Franck Bui-Huu

Quoted 3 lines · Andrew Morton
Unfortunately we have a conflict with the already-queued do_mpage_readpage-dont-submit-lots-of-small-bios-on-boundary.patch:
How about amending this on top of take #2 ?

Actually the version below is better...

Franck

---8<---

fs/mpage.c +14 -4

index 74e7f83..ca33b28 100644
@@ -157,6 +157,19 @@ map_buffer_to_page(struct page *page, struct buffer_head *bh, int page_block)
 }
 
 /*
+ * Since get_block() can map a range of pages, for all these pages the
+ * BH_Boundary flag will be set.  But we only need to push what I/O we
+ * have accumulated at the last block of this range.
+*/
+static int last_mapped_page(struct buffer_head *map_bh, struct page *page)
+{
+	pgoff_t map_index = map_bh->b_page->index;
+
+	return buffer_boundary(map_bh) &&
+		page->index >= map_index + (map_bh->b_size>>PAGE_CACHE_SHIFT);
+}
+
+/*
  * This is the worker routine which does all the work of mapping the disk
  * blocks and constructs largest possible bios, submits them for IO if the
  * blocks are not contiguous on the disk.

@@ -312,10 +325,7 @@ alloc_new:
 		goto alloc_new;
 	}
 
-	relative_block = block_in_file - *first_logical_block;
-	nblocks = map_bh->b_size >> blkbits;
-	if ((buffer_boundary(map_bh) && relative_block == nblocks) ||
-	    (first_hole != blocks_per_page))
+	if (last_mapped_page(map_bh, page) || first_hole != blocks_per_page)
 		bio = mpage_bio_submit(READ, bio);
 	else
 		*last_block_in_bio = blocks[blocks_per_page - 1];