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 <[email protected]>
---
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 *can_merge_io; /* IO merging check function */
loff_t logical_offset_in_bio; /* current first logical block in bio */
sector_t final_block_in_bio; /* current final block in bio + 1 */
sector_t next_block_for_io; /* next block to be put under IO,
@@ -661,6 +662,10 @@ static int dio_send_cur_page(struct dio *dio)
*/
else if (dio->boundary)
dio_bio_submit(dio);
+ else if (dio->can_merge_io &&
+ dio->can_merge_io(dio->cur_page_len, dio->inode,
+ dio->bio))
+ dio_bio_submit(dio);
}
if (dio->bio == NULL) {
@@ -983,7 +988,7 @@ static ssize_t
direct_io_worker(int rw, struct kiocb *iocb, struct inode *inode,
const struct iovec *iov, loff_t offset, unsigned long nr_segs,
unsigned blkbits, get_block_t get_block, dio_iodone_t end_io,
- dio_submit_t submit_io, struct dio *dio)
+ dio_submit_t submit_io, can_merge_io_t can_merge_io, struct dio *dio)
{
unsigned long user_addr;
unsigned long flags;
@@ -1001,6 +1006,7 @@ direct_io_worker(int rw, struct kiocb *iocb, struct inode *inode,
dio->get_block = get_block;
dio->end_io = end_io;
dio->submit_io = submit_io;
+ dio->can_merge_io = can_merge_io;
dio->final_block_in_bio = -1;
dio->next_block_for_io = -1;
@@ -1159,7 +1165,7 @@ ssize_t
__blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
struct block_device *bdev, const struct iovec *iov, loff_t offset,
unsigned long nr_segs, get_block_t get_block, dio_iodone_t end_io,
- dio_submit_t submit_io, int flags)
+ dio_submit_t submit_io, can_merge_io_t can_merge_io, int flags)
{
int seg;
size_t size;
@@ -1247,7 +1253,7 @@ __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
retval = direct_io_worker(rw, iocb, inode, iov, offset,
nr_segs, blkbits, get_block, end_io,
- submit_io, dio);
+ submit_io, can_merge_io, dio);
out:
return retval;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index bdbe699..b67964b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3553,7 +3553,7 @@ retry:
ret = __blockdev_direct_IO(rw, iocb, inode,
inode->i_sb->s_bdev, iov,
offset, nr_segs,
- ext4_get_block, NULL, NULL, 0);
+ ext4_get_block, NULL, NULL, NULL, 0);
else {
ret = blockdev_direct_IO(rw, iocb, inode,
inode->i_sb->s_bdev, iov,
diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 4f36f88..35fc14b 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -1039,7 +1039,7 @@ static ssize_t gfs2_direct_IO(int rw, struct kiocb *iocb,
rv = __blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
offset, nr_segs, gfs2_get_block_direct,
- NULL, NULL, 0);
+ NULL, NULL, NULL, 0);
out:
gfs2_glock_dq_m(1, &gh);
gfs2_holder_uninit(&gh);
diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index f1e962c..5d006ea 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -631,7 +631,7 @@ static ssize_t ocfs2_direct_IO(int rw,
ret = __blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev,
iov, offset, nr_segs,
ocfs2_direct_IO_get_blocks,
- ocfs2_dio_end_io, NULL, 0);
+ ocfs2_dio_end_io, NULL, NULL, 0);
mlog_exit(ret);
return ret;
diff --git a/fs/xfs/linux-2.6/xfs_aops.c b/fs/xfs/linux-2.6/xfs_aops.c
index 7d287af..1f3eaa5 100644
--- a/fs/xfs/linux-2.6/xfs_aops.c
+++ b/fs/xfs/linux-2.6/xfs_aops.c
@@ -1484,14 +1484,15 @@ xfs_vm_direct_IO(
ret = __blockdev_direct_IO(rw, iocb, inode, bdev, iov,
offset, nr_segs,
xfs_get_blocks_direct,
- xfs_end_io_direct_write, NULL, 0);
+ xfs_end_io_direct_write, NULL,
+ NULL, 0);
if (ret != -EIOCBQUEUED && iocb->private)
xfs_destroy_ioend(iocb->private);
} else {
ret = __blockdev_direct_IO(rw, iocb, inode, bdev, iov,
offset, nr_segs,
xfs_get_blocks_direct,
- NULL, NULL, 0);
+ NULL, NULL, NULL, 0);
}
return ret;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 334d68a..b4c2976 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2311,6 +2311,8 @@ static inline int xip_truncate_page(struct address_space *mapping, loff_t from)
typedef void (dio_submit_t)(int rw, struct bio *bio, struct inode *inode,
loff_t file_offset);
+typedef int (can_merge_io_t)(size_t size, struct inode *inode, struct bio *bio);
+
enum {
/* need locking between buffered and direct access */
DIO_LOCKING = 0x01,
@@ -2324,7 +2326,7 @@ void dio_end_io(struct bio *bio, int error);
ssize_t __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
struct block_device *bdev, const struct iovec *iov, loff_t offset,
unsigned long nr_segs, get_block_t get_block, dio_iodone_t end_io,
- dio_submit_t submit_io, int flags);
+ dio_submit_t submit_io, can_merge_io_t can_merge_io, int flags);
static inline ssize_t blockdev_direct_IO(int rw, struct kiocb *iocb,
struct inode *inode, struct block_device *bdev, const struct iovec *iov,
@@ -2332,7 +2334,7 @@ static inline ssize_t blockdev_direct_IO(int rw, struct kiocb *iocb,
dio_iodone_t end_io)
{
return __blockdev_direct_IO(rw, iocb, inode, bdev, iov, offset,
- nr_segs, get_block, end_io, NULL,
+ nr_segs, get_block, end_io, NULL, NULL,
DIO_LOCKING | DIO_SKIP_HOLES);
}
#endif
--
1.7.0.1
On Wed, Nov 17, 2010 at 12:18:35PM +0800, Miao Xie wrote:
> 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.
>
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
On Wed, Nov 17, 2010 at 02:06:59AM -0500, Josef Bacik wrote:
> On Wed, Nov 17, 2010 at 12:18:35PM +0800, Miao Xie wrote:
> > 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.
> >
>
> 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);
>
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
Hi, Josef
On wed, 17 Nov 2010 04:37:21 -0500, Josef Bacik wrote:
>> 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);
>>
>
> 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);
> }
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
On Wed, Nov 17, 2010 at 06:11:03PM +0800, Miao Xie wrote:
> Hi, Josef
>
> On wed, 17 Nov 2010 04:37:21 -0500, Josef Bacik wrote:
>>> 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);
>>>
>>
>> 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);
>> }
>
> 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.
>
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
Excerpts from Josef Bacik's message of 2010-11-17 07:50:11 -0500:
> On Wed, Nov 17, 2010 at 06:11:03PM +0800, Miao Xie wrote:
> > Hi, Josef
> >
> > On wed, 17 Nov 2010 04:37:21 -0500, Josef Bacik wrote:
> >>> 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);
> >>>
> >>
> >> 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);
> >> }
> >
> > 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.
> >
>
> 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,
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
On wed, 17 Nov 2010 11:55:28 -0500, Chris Mason wrote:
> Excerpts from Josef Bacik's message of 2010-11-17 07:50:11 -0500:
>> On Wed, Nov 17, 2010 at 06:11:03PM +0800, Miao Xie wrote:
>>> Hi, Josef
>>>
>>> On wed, 17 Nov 2010 04:37:21 -0500, Josef Bacik wrote:
>>>>> 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);
>>>>>
>>>>
>>>> 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);
>>>> }
>>>
>>> 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.
>>>
>>
>> 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,
>
> 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.
Do you mean you are fixing this bug now?
Thanks
Miao
Excerpts from Miao Xie's message of 2010-11-17 20:18:18 -0500:
> >> 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,
> >
> > 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.
>
> Do you mean you are fixing this bug now?
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
On wed, 17 Nov 2010 20:24:39 -0500, Chris Mason wrote:
> Excerpts from Miao Xie's message of 2010-11-17 20:18:18 -0500:
>>>> 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,
>>>
>>> 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.
>>
>> Do you mean you are fixing this bug now?
>
> 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.
Ok, I'll do it.
Thanks
Miao