[PATCH] do_mpage_readpage(): remove first_logical_block parameter

[PATCH] do_mpage_readpage(): remove first_logical_block parameter

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

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 <fbuihuu@gmail.com>
---

   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(-)

diff --git a/fs/mpage.c b/fs/mpage.c
index cf05ca7..da4d66f 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -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;
-- 
1.6.0.2.GIT

Re: [PATCH] do_mpage_readpage(): remove first_logical_block parameter

[Andrew Morton]

On Tue, 25 Nov 2008 21:29:11 +0100
Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:

> From: Franck Bui-Huu <fbuihuu@gmail.com>
> 
> 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.
> 

umm, ok, it seems a bit simpler.

> diff --git a/fs/mpage.c b/fs/mpage.c
> index cf05ca7..da4d66f 100644
> --- a/fs/mpage.c
> +++ b/fs/mpage.c
> @@ -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;

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.

>  	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);

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.


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


> +			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;

Re: [PATCH] do_mpage_readpage(): remove first_logical_block parameter

[Franck Bui-Huu]

Andrew Morton wrote:
>> diff --git a/fs/mpage.c b/fs/mpage.c
>> index cf05ca7..da4d66f 100644
>> --- a/fs/mpage.c
>> +++ b/fs/mpage.c
>> @@ -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;
> 
> 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...

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

>>  	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);
> 
> 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.
> 

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.

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

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

>> +
>> +		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
 

[PATCH take2] do_mpage_readpage(): remove first_logical_block parameter

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

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.

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 <fbuihuu@gmail.com>
---
   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(-)

diff --git a/fs/mpage.c b/fs/mpage.c
index cf05ca7..a041a0e 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -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;
-- 
1.6.0.2.GIT

Re: [PATCH take2] do_mpage_readpage(): remove first_logical_block parameter

[Andrew Morton]

On Wed, 26 Nov 2008 21:04:52 +0100 Franck Bui-Huu <vagabon.xyz@gmail.com> 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

Re: [PATCH take2] do_mpage_readpage(): remove first_logical_block parameter

[Franck Bui-Huu]

Andrew Morton wrote:
> 
> 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<---

diff --git a/fs/mpage.c b/fs/mpage.c
index 74e7f83..ef26eb1 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -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];

Re: [PATCH take2] do_mpage_readpage(): remove first_logical_block parameter

[Franck Bui-Huu]

Franck Bui-Huu wrote:
> Andrew Morton wrote:
>> 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<---

diff --git a/fs/mpage.c b/fs/mpage.c
index 74e7f83..ca33b28 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -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];