linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Franck Bui-Huu <vagabon.xyz@gmail.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-fsdevel@vger.kernel.org
Subject: [PATCH take2] do_mpage_readpage(): remove first_logical_block parameter
Date: Wed, 26 Nov 2008 21:04:52 +0100	[thread overview]
Message-ID: <492DABE4.7010709@gmail.com> (raw)
In-Reply-To: <20081125164803.952e9fd9.akpm@linux-foundation.org>

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


  parent reply	other threads:[~2008-11-26 20:05 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-11-25 20:29 [PATCH] do_mpage_readpage(): remove first_logical_block parameter Franck Bui-Huu
2008-11-26  0:48 ` Andrew Morton
2008-11-26  8:03   ` Franck Bui-Huu
2008-11-26 20:04   ` Franck Bui-Huu [this message]
2008-11-29  9:23     ` [PATCH take2] " Andrew Morton
2008-11-30 10:48       ` Franck Bui-Huu
2008-11-30 13:53         ` Franck Bui-Huu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=492DABE4.7010709@gmail.com \
    --to=vagabon.xyz@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=linux-fsdevel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).