map multiple blocks at a time in mpage_readpage(s)

This patch changes mpage_readpage/mpage_readpages to use a get_blocks call that gets the disk mapping information for multiple blocks at the same time, similar to the way direct I/O code works. For extent based filesystems like jfs, xfs or reiser4 this allows to reduce the overhead of the allocator calls, which is especially nice when that codepath is rather heavyweight (as it is for example in xfs). For filesystems that don't have an allocator that make use of it nothing changes.

I've tested this heavily on XFS with block size = page size and 512 byte blocks, and it passes the XFS QA regression test suite fine. I've done some basic fsx testing on all the other affected filesystems.

This patch changes mpage_readpage/mpage_readpages to use a get_blocks call that gets the disk mapping information for multiple blocks at the same time, similar to the way direct I/O code works. For extent based filesystems like jfs, xfs or reiser4 this allows to reduce the overhead of the allocator calls, which is especially nice when that codepath is rather heavyweight (as it is for example in xfs). For filesystems that don't have an allocator that make use of it nothing changes.

I've tested this heavily on XFS with block size = page size and 512 byte blocks, and it passes the XFS QA regression test suite fine. I've done some basic fsx testing on all the other affected filesystems.

Index: linux-2.6/fs/ext3/inode.c
===================================================================

fs/ext3/inode.c +15 -2

@@ -800,6 +800,18 @@
 	return ret;
 }
 
+static int
+ext3_get_blocks(struct inode *inode, sector_t iblock, unsigned long max_blocks,
+			struct buffer_head *bh_result, int create)
+{
+	int ret;
+
+	ret = ext3_get_block(inode, iblock, bh_result, create);
+	if (ret == 0)
+		bh_result->b_size = (1 << inode->i_blkbits);
+	return ret;
+}
+
 #define DIO_CREDITS (EXT3_RESERVE_TRANS_BLOCKS + 32)
 
 static int

@@ -1411,14 +1423,15 @@
 
 static int ext3_readpage(struct file *file, struct page *page)
 {
-	return mpage_readpage(page, ext3_get_block);
+	return mpage_readpage(page, ext3_get_blocks, ext3_get_block);
 }
 
 static int
 ext3_readpages(struct file *file, struct address_space *mapping,
 		struct list_head *pages, unsigned nr_pages)
 {
-	return mpage_readpages(mapping, pages, nr_pages, ext3_get_block);
+	return mpage_readpages(mapping, pages, nr_pages,
+			ext3_get_blocks, ext3_get_block);
 }
 
 static int ext3_invalidatepage(struct page *page, unsigned long offset)

Index: linux-2.6/fs/mpage.c
===================================================================

fs/mpage.c +76 -19

@@ -165,7 +165,9 @@
 
 static struct bio *
 do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
-			sector_t *last_block_in_bio, get_block_t get_block)
+			sector_t *last_block_in_bio, struct buffer_head *map_bh,
+			unsigned long *first_logical_block, int *map_valid,
+			get_blocks_t get_blocks, get_block_t get_block)
 {
 	struct inode *inode = page->mapping->host;
 	const unsigned blkbits = inode->i_blkbits;

@@ -177,29 +179,63 @@
 	unsigned page_block;
 	unsigned first_hole = blocks_per_page;
 	struct block_device *bdev = NULL;
-	struct buffer_head bh;
 	int length;
 	int fully_mapped = 1;
+	unsigned nblocks, i;
 
 	if (page_has_buffers(page))
 		goto confused;
 
 	block_in_file = page->index << (PAGE_CACHE_SHIFT - blkbits);
 	last_block = (i_size_read(inode) + blocksize - 1) >> blkbits;
+	page_block = 0;
+
+	/*
+	 * Map blocks using the result from the last get_blocks call first.
+	 */
+	nblocks = map_bh->b_size >> inode->i_blkbits;
+	if (*map_valid &&
+	    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 (i = 0; ; i++) {
+			if (i == last) {
+				*map_valid = 0;
+				break;
+			} else if (page_block == blocks_per_page)
+				break;
+			blocks[page_block] = map_bh->b_blocknr + map_offset + i;
+			page_block++;
+			block_in_file++;
+		}
+		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;
 
-	bh.b_page = page;
-	for (page_block = 0; page_block < blocks_per_page;
-				page_block++, block_in_file++) {
-		bh.b_state = 0;
 		if (block_in_file < last_block) {
-			if (get_block(inode, block_in_file, &bh, 0))
+			if (get_blocks(inode, block_in_file,
+				       last_block - block_in_file, map_bh, 0))
 				goto confused;
+			*first_logical_block = block_in_file;
+			*map_valid  = 1;
 		}
 
-		if (!buffer_mapped(&bh)) {
+		if (!buffer_mapped(map_bh)) {
 			fully_mapped = 0;
 			if (first_hole == blocks_per_page)
 				first_hole = page_block;
+			page_block++;
+			block_in_file++;
+			*map_valid = 0;
 			continue;
 		}
 

@@ -209,8 +245,8 @@
 		 * we just collected from get_block into the page's buffers
 		 * so readpage doesn't have to repeat the get_block call
 		 */
-		if (buffer_uptodate(&bh)) {
-			map_buffer_to_page(page, &bh, page_block);
+		if (buffer_uptodate(map_bh)) {
+			map_buffer_to_page(page, map_bh, page_block);
 			goto confused;
 		}
 

@@ -218,10 +254,20 @@
 			goto confused;		/* hole -> non-hole */
 
 		/* Contiguous blocks? */
-		if (page_block && blocks[page_block-1] != bh.b_blocknr-1)
+		if (page_block && blocks[page_block-1] != map_bh->b_blocknr-1)
 			goto confused;
-		blocks[page_block] = bh.b_blocknr;
-		bdev = bh.b_bdev;
+		nblocks = map_bh->b_size >> inode->i_blkbits;
+		for (i = 0; ; i++) {
+			if (i == nblocks) {
+				*map_valid = 0;
+				break;
+			} else if (page_block == blocks_per_page)
+				break;
+			blocks[page_block] = map_bh->b_blocknr + i;
+			page_block++;
+			block_in_file++;
+		}
+		bdev = map_bh->b_bdev;
 	}
 
 	if (first_hole != blocks_per_page) {

@@ -260,7 +306,7 @@
 		goto alloc_new;
 	}
 
-	if (buffer_boundary(&bh) || (first_hole != blocks_per_page))
+	if (buffer_boundary(map_bh) || (first_hole != blocks_per_page))
 		bio = mpage_bio_submit(READ, bio);
 	else
 		*last_block_in_bio = blocks[blocks_per_page - 1];

@@ -325,12 +371,16 @@
  */
 int
 mpage_readpages(struct address_space *mapping, struct list_head *pages,
-				unsigned nr_pages, get_block_t get_block)
+				unsigned nr_pages, get_blocks_t get_blocks,
+				get_block_t get_block)
 {
 	struct bio *bio = NULL;
 	unsigned page_idx;
 	sector_t last_block_in_bio = 0;
 	struct pagevec lru_pvec;
+	struct buffer_head map_bh;
+	unsigned long first_logical_block = 0;
+	int map_valid = 0;
 
 	pagevec_init(&lru_pvec, 0);
 	for (page_idx = 0; page_idx < nr_pages; page_idx++) {

@@ -342,7 +392,9 @@
 					page->index, GFP_KERNEL)) {
 			bio = do_mpage_readpage(bio, page,
 					nr_pages - page_idx,
-					&last_block_in_bio, get_block);
+					&last_block_in_bio, &map_bh,
+					&first_logical_block, &map_valid,
+					get_blocks, get_block);
 			if (!pagevec_add(&lru_pvec, page))
 				__pagevec_lru_add(&lru_pvec);
 		} else {

@@ -360,13 +412,18 @@
 /*
  * This isn't called much at all
  */
-int mpage_readpage(struct page *page, get_block_t get_block)
+int mpage_readpage(struct page *page, get_blocks_t get_blocks,
+		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;
+	int map_valid = 0;
 
-	bio = do_mpage_readpage(bio, page, 1,
-			&last_block_in_bio, get_block);
+	bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio,
+			&map_bh, &first_logical_block, &map_valid,
+			get_blocks, get_block);
 	if (bio)
 		mpage_bio_submit(READ, bio);
 	return 0;

Index: linux-2.6/fs/xfs/linux-2.6/xfs_aops.c
===================================================================

fs/xfs/linux-2.6/xfs_aops.c +15 -2

@@ -1034,6 +1034,18 @@
 }
 
 STATIC int
+linvfs_get_blocks(
+	struct inode		*inode,
+	sector_t		iblock,
+	unsigned long		max_blocks,
+	struct buffer_head	*bh_result,
+	int			create)
+{
+	return __linvfs_get_block(inode, iblock, max_blocks, bh_result,
+					create, 0, BMAPI_WRITE);
+}
+
+STATIC int
 linvfs_get_blocks_direct(
 	struct inode		*inode,
 	sector_t		iblock,

@@ -1139,7 +1151,7 @@
 	struct file		*unused,
 	struct page		*page)
 {
-	return mpage_readpage(page, linvfs_get_block);
+	return mpage_readpage(page, linvfs_get_blocks, linvfs_get_block);
 }
 
 STATIC int

@@ -1149,7 +1161,8 @@
 	struct list_head	*pages,
 	unsigned		nr_pages)
 {
-	return mpage_readpages(mapping, pages, nr_pages, linvfs_get_block);
+	return mpage_readpages(mapping, pages, nr_pages, linvfs_get_blocks,
+			linvfs_get_block);
 }
 
 STATIC void

Index: linux-2.6/include/linux/mpage.h
===================================================================

include/linux/mpage.h +0 -0

@@ -14,8 +14,9 @@
 typedef int (writepage_t)(struct page *page, struct writeback_control *wbc);

Do you really want mpage_readpage()/mpage_readpages() to take get_blocks() and get_block() to take as arguments ? Or it just a initial version and you have grand plans to clean it up ? Me & Suparna did a version which needed only get_blocks() few months ago. I can try to dig it up, if you like.

In fact, we want to do the same thing for writepages() also - so that the filesystem can do the block allocation in a single shot. (as part of delayed allocation).

Quoted 5 lines · Badari Pulavarty

Do you really want mpage_readpage()/mpage_readpages() to take get_blocks() and get_block() to take as arguments ? Or it just a initial version and you have grand plans to clean it up ? Me & Suparna did a version which needed only get_blocks() few months ago. I can try to dig it up, if you like.

I don't see how to get rid of it easily. It's need for the confused case that calls block_read_full_page, which needs a get_block() callback. Adding a failback_writepage callback would be a tad nicer but not really solve the general issue.

fs/mpage.c line 397

Quoted 7 lines

fs/mpage.c +3 -1

 					page->index, GFP_KERNEL)) {
 			bio = do_mpage_readpage(bio, page,
 					nr_pages - page_idx,
-					&last_block_in_bio, get_block);
+					&last_block_in_bio, &map_bh,
+					&first_logical_block, &map_valid,
+					get_blocks, get_block);

Oh, function taking 9 arguments, please... this is not windows.

By the way, I cannot see where get_block callback is used by mpage.c code, why is it needed?

Quoted 8 lines · Nikita Danilov

Quoted 7 lines

fs/mpage.c +3 -1

 					page->index, GFP_KERNEL)) {
 			bio = do_mpage_readpage(bio, page,
 					nr_pages - page_idx,
-					&last_block_in_bio, get_block);
+					&last_block_in_bio, &map_bh,
+					&first_logical_block, &map_valid,
+					get_blocks, get_block);

Quoted 1 line

Oh, function taking 9 arguments, please... this is not windows.

I'm happy about suggestion to reduce the count. One of the real problems is that the buffer_head structure is extremly badly suited for the get_blocks callback. Having information like in the xfs_iomap_t structure (which is XFS bmap information container) would simplify lots of these things.

Quoted 4 lines

By the way, I cannot see where get_block callback is used by mpage.c code, why is it needed?

It's only used by block_read_full_page which is called in the 'confused' case.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

This patch changes mpage_readpage/mpage_readpages to use a get_blocks call that gets the disk mapping information for multiple blocks at the same time, similar to the way direct I/O code works. For extent based filesystems like jfs, xfs or reiser4 this allows to reduce the overhead of the allocator calls, which is especially nice when that codepath is rather heavyweight (as it is for example in xfs). For filesystems that don't have an allocator that make use of it nothing changes.

I've tested this heavily on XFS with block size = page size and 512 byte blocks, and it passes the XFS QA regression test suite fine. I've done some basic fsx testing on all the other affected filesystems.

Quoted 8 lines · Christoph Hellwig

This patch changes mpage_readpage/mpage_readpages to use a get_blocks call that gets the disk mapping information for multiple blocks at the same time, similar to the way direct I/O code works. For extent based filesystems like jfs, xfs or reiser4 this allows to reduce the overhead of the allocator calls, which is especially nice when that codepath is rather heavyweight (as it is for example in xfs). For filesystems that don't have an allocator that make use of it nothing changes.

Quoted 3 lines

I've tested this heavily on XFS with block size = page size and 512 byte blocks, and it passes the XFS QA regression test suite fine. I've done some basic fsx testing on all the other affected filesystems.

I think the patch should go into -mm after no one had serious objections.

Signed-off-by Christoph Hellwig <[email protected]>

Quoted 8 lines · Christoph Hellwig

This patch changes mpage_readpage/mpage_readpages to use a get_blocks call that gets the disk mapping information for multiple blocks at the same time, similar to the way direct I/O code works. For extent based filesystems like jfs, xfs or reiser4 this allows to reduce the overhead of the allocator calls, which is especially nice when that codepath is rather heavyweight (as it is for example in xfs). For filesystems that don't have an allocator that make use of it nothing changes.

Quoted 3 lines

I've tested this heavily on XFS with block size = page size and 512 byte blocks, and it passes the XFS QA regression test suite fine. I've done some basic fsx testing on all the other affected filesystems.

I think the patch should go into -mm after no one had serious objections.

Quoted 14 lines · Christoph Hellwig

Quoted 11 lines · Christoph Hellwig

This patch changes mpage_readpage/mpage_readpages to use a get_blocks call that gets the disk mapping information for multiple blocks at the same time, similar to the way direct I/O code works. For extent based filesystems like jfs, xfs or reiser4 this allows to reduce the overhead of the allocator calls, which is especially nice when that codepath is rather heavyweight (as it is for example in xfs). For filesystems that don't have an allocator that make use of it nothing changes.

I've tested this heavily on XFS with block size = page size and 512 byte blocks, and it passes the XFS QA regression test suite fine. I've done some basic fsx testing on all the other affected filesystems.

Quoted 1 line

I think the patch should go into -mm after no one had serious objections.

It adds a bunch of complexity and overhead which will speed up a filesystem which few people use while slowing down the filesystems which most people use.

If/when we have an ext3 implementation of get_blocks() and some benchmarks which show it's worthwhile then OK.

Could we please see the xfs benchmark results?

first_logical_block should have type sector_t.

Did you consider aggregating those nine args to do_mpage_readpage() into a struct, passing the address of that?

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 1 line · Christoph Hellwig

I think the patch should go into -mm after no one had serious objections.

It adds a bunch of complexity and overhead which will speed up a filesystem which few people use while slowing down the filesystems which most people use.

If/when we have an ext3 implementation of get_blocks() and some benchmarks which show it's worthwhile then OK.

Could we please see the xfs benchmark results?

first_logical_block should have type sector_t.

Did you consider aggregating those nine args to do_mpage_readpage() into a struct, passing the address of that?

It will speed up 3 filesystems which a significant number of people do use. I doubt the overhead to ext3 is even measurable, and I'd venture to say that the majority using ext3 aren't using it because of its speed.