On Tue, 25 Nov 2008 21:29:11 +0100
Franck Bui-Huu <vagabon.xyz@xxxxxxxxx> wrote:
> From: Franck Bui-Huu <fbuihuu@xxxxxxxxx>
>
> 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
