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

[Andrew Morton <akpm@linux-foundation.org>]

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;