Re: [PATCH 1/3] direct-io: add a hook for the fs to provide its own bio merging check function

Previous thread: [PATCH 13/13] writeback: make nr_to_write a per-file limit by Wu Fengguang on Tuesday, November 16, 2010 - 8:58 pm. (1 message)

Next thread: [PATCH 2/3] btrfs: restructure btrfs_merge_bio_hook() by Miao Xie on Tuesday, November 16, 2010 - 9:23 pm. (1 message)
From: Miao Xie
Date: Tuesday, November 16, 2010 - 9:18 pm

BTRFS can not submit bios that span its chunks or stripes, so it needs a
function to check it when we want to add a page into the bios. So we add a
can_merge_io hook to do it.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/block_dev.c              |    3 ++-
 fs/btrfs/inode.c            |    2 +-
 fs/direct-io.c              |   12 +++++++++---
 fs/ext4/inode.c             |    2 +-
 fs/gfs2/aops.c              |    2 +-
 fs/ocfs2/aops.c             |    2 +-
 fs/xfs/linux-2.6/xfs_aops.c |    5 +++--
 include/linux/fs.h          |    6 ++++--
 8 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 06e8ff1..e3728f6 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -188,7 +188,8 @@ blkdev_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov,
 	struct inode *inode = file->f_mapping->host;
 
 	return __blockdev_direct_IO(rw, iocb, inode, I_BDEV(inode), iov, offset,
-				    nr_segs, blkdev_get_blocks, NULL, NULL, 0);
+				    nr_segs, blkdev_get_blocks, NULL, NULL,
+				    NULL, 0);
 }
 
 int __sync_blockdev(struct block_device *bdev, int wait)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 558cac2..3906e48 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5868,7 +5868,7 @@ static ssize_t btrfs_direct_IO(int rw, struct kiocb *iocb,
 	ret = __blockdev_direct_IO(rw, iocb, inode,
 		   BTRFS_I(inode)->root->fs_info->fs_devices->latest_bdev,
 		   iov, offset, nr_segs, btrfs_get_blocks_direct, NULL,
-		   btrfs_submit_direct, 0);
+		   btrfs_submit_direct, NULL, 0);
 
 	if (ret < 0 && ret != -EIOCBQUEUED) {
 		clear_extent_bit(&BTRFS_I(inode)->io_tree, offset,
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 85882f6..f0b14a4 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -83,6 +83,7 @@ struct dio {
 	get_block_t *get_block;		/* block mapping function */
 	dio_iodone_t *end_io;		/* IO completion function */
 	dio_submit_t *submit_io;	/* IO submition function */
+	can_merge_io_t ...
From: Josef Bacik
Date: Wednesday, November 17, 2010 - 12:06 am

Heh so I was going to fix this after the hole punching stuff.  The fact is btrfs
maps everything that is ok to do in one IO via get_blocks().  So all we need to
do is add another DIO_ flag to tell us to treat each get_blocks() call as
discrete.  I wanted to use buffer_boundary for this, but I think it's too
drastic of a change for people who already use buffer_boundary();

What happens today is that say we map 4k, we do submit_page_section, but if this
is our first bit of IO we just set dio->cur_page and such and then loop again.
Say there is 4k-hole-4k, we do the next mapping and set buffer_boundary again,
and come into submit_page_section and because cur_page is set, we do
dio_send_cur_page.  Because there is no dio->bio we setup a new bio, but when we
do that we clear dio->boundary, and leave the bio all setup.  So the next time
we loop around the tail 4k gets added to our previously setup bio and boom we
hit this problem with btrfs.

If we can add a DIO_GET_BLOCKS_DISCRETE or some other such non-sense then we can
easily kill all the logical offset code I had and just make some simple changes
to make the DIO stuff work for us.  All we do is in get_more_blocks we do

if ((dio->flags & DIO_GET_BLOCKS_DISCRETE) && dio->bio)
	dio_submit_bio(dio);

before we do anything else and that way btrfs is satisfied since we won't merge
non logically contiguous requests.

So thats a long-winded way of saying NACK, lets not add even more complicated
special crap for dealing with btrfs when we can just do something like the
above.  Thanks,

Josef
--

From: Josef Bacik
Date: Wednesday, November 17, 2010 - 2:37 am

Right after I went to bed I realized this should be

if (dio->flags & DIO_GET_BLOCKS_DISCRETE) {
	if (dio->cur_page) {
		dio_send_cur_page(dio);
		page_cache_release(dio->cur_page);
		dio->cur_page = NULL;
	}

	if (dio->bio)
		dio_submit_bio(dio);
 }

Thanks,

Josef
--

From: Miao Xie
Date: Wednesday, November 17, 2010 - 3:11 am

Hi, Josef


As far as I know, get_block() can not make sure the IO doesn't span the chunks or
stripes. Maybe we can do this check in get_blocks(). In this way, we needn't change
vfs.

I have written the patch and is testing it now. Up to now, it works well.

Thanks
Miao
--

From: Josef Bacik
Date: Wednesday, November 17, 2010 - 5:50 am

Right thats the idea, if we can't span chunks/stripes we should be doing that
limiting in our get_blocks call and that way we don't have to screw with the
generic direct io stuff too much.  Thanks,

Josef
--

From: Chris Mason
Date: Wednesday, November 17, 2010 - 9:55 am

In this case we're adding complexity to the O_DIRECT mapping code, when
we really should be adding it to the btrfs submit bio hook.  It can
easily break up the bio into smaller units, which will leave us with a
smaller number of get_blocks calls overall.

I'm working that out now.

-chris


--

From: Miao Xie
Date: Wednesday, November 17, 2010 - 6:18 pm

Do you mean you are fixing this bug now?

Thanks
Miao
--

From: Chris Mason
Date: Wednesday, November 17, 2010 - 6:24 pm

I started on it this afternoon, but lost network due to high winds here.
So, I didn't make any real progress.

If you'd like to fix this in the btrfs direct-io bio submit call you're
welcome to continue working on it.

The idea is to just clone and split up the bio, which will keep us from
filling up fs/direct-io.c w/btrfs rules and allow us to take fewer
trips into the get_blocks call.

-chris
--

From: Miao Xie
Date: Wednesday, November 17, 2010 - 6:33 pm

Ok, I'll do it.

Thanks
Miao
--

Previous thread: [PATCH 13/13] writeback: make nr_to_write a per-file limit by Wu Fengguang on Tuesday, November 16, 2010 - 8:58 pm. (1 message)

Next thread: [PATCH 2/3] btrfs: restructure btrfs_merge_bio_hook() by Miao Xie on Tuesday, November 16, 2010 - 9:23 pm. (1 message)