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

[Franck Bui-Huu <vagabon.xyz@gmail.com>]

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