Skip to contentSkip to conversation

fs/block-dev.c:fix performance regression in O_DIRECT writes to md block devices.

PATCH In review majianpeng 2 participants 3 messages Jul 16, 2012

Subsystems (2)
lore
Share & tools
lore

majianpeng

author
#0 lore raw

PATCH 1/2 fs/block-dev.c:fix performance regression in O_DIRECT writes to md block devices.

For regular file, write operaion used blk_plug function.But for block file,write operation did not use blk_plug.

---
 fs/block_dev.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
fs/block_dev.c +6 -1
index c2bbe1f..22cd436 100644
@@ -215,9 +215,14 @@ blkdev_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov,
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file->f_mapping->host;
+	struct blk_plug plug;
+	ssize_t ret;
 
-	return __blockdev_direct_IO(rw, iocb, inode, I_BDEV(inode), iov, offset,
+	blk_start_plug(&plug);
+	ret =  __blockdev_direct_IO(rw, iocb, inode, I_BDEV(inode), iov, offset,
 				    nr_segs, blkdev_get_blocks, NULL, NULL, 0);
+	blk_finish_plug(&plug);
+	return ret;
 }
 
 int __sync_blockdev(struct block_device *bdev, int wait)
Signature · 1 line
1.7.5.4
#1 lore raw
Quoted 18 lines · On Mon, 16 Jul 2012 09:30:50 +0800 majianpeng <[email protected]> wrote:

For regular file, write operaion used blk_plug function.But for block file,write operation did not use blk_plug.

---
 fs/block_dev.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
fs/block_dev.c +2 -0
index c2bbe1f..22cd436 100644
@@ -215,9 +215,14 @@ blkdev_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov,
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file->f_mapping->host;
+	struct blk_plug plug;
+	ssize_t ret;

fs/block_dev.c blkdev_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov, · line 228

Quoted 10 lines
fs/block_dev.c +4 -1
 
-	return __blockdev_direct_IO(rw, iocb, inode, I_BDEV(inode), iov, offset,
+	blk_start_plug(&plug);
+	ret =  __blockdev_direct_IO(rw, iocb, inode, I_BDEV(inode), iov, offset,
 				    nr_segs, blkdev_get_blocks, NULL, NULL, 0);
+	blk_finish_plug(&plug);
+	return ret;
 }
 
 int __sync_blockdev(struct block_device *bdev, int wait)

[cc:ing Jens Axboe]

I think we do need something like this, but I don't think this is the right place for it.

For normal filesystem writes, the blk_{start,finish}_plug calls are in generic_file_aio_write which is the "aio_write" function, or is called by it. aio_write calls generic_file_aio_write, calls blk_start_plug, call __generic_file_aio_write.

For block devices we bypass the generic_file_aio_write:

aio_write calls blkdev_aio_write calls __generic_file_aio_write - without calling blk_start_plug.
So I think the calls to blk_start_plug and blk_finish_plug should go in blkdev_aio_write. i.e. blkdev_aio_write should be made to look more like generic_file_aio_write (just without the mutex_lock/unlock).

If you redo the patch like that and test it I'll happily add my Reviewed-by.

I suspect it should be merged through Jens' tree.

Thanks,
NeilBrown

author
#2 lore raw

On 2012-07-16 13:21 NeilBrown <[email protected]> Wrote:

Quoted 49 lines · On Mon, 16 Jul 2012 09:30:50 +0800 majianpeng <[email protected]> wrote:
Quoted 19 lines

For regular file, write operaion used blk_plug function.But for block file,write operation did not use blk_plug.

---
 fs/block_dev.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
fs/block_dev.c +2 -0
index c2bbe1f..22cd436 100644
@@ -215,9 +215,14 @@ blkdev_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov,
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file->f_mapping->host;
+	struct blk_plug plug;
+	ssize_t ret;

fs/block_dev.c blkdev_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov, · line 228

Quoted 10 lines
fs/block_dev.c +4 -1
 
-	return __blockdev_direct_IO(rw, iocb, inode, I_BDEV(inode), iov, offset,
+	blk_start_plug(&plug);
+	ret =  __blockdev_direct_IO(rw, iocb, inode, I_BDEV(inode), iov, offset,
 				    nr_segs, blkdev_get_blocks, NULL, NULL, 0);
+	blk_finish_plug(&plug);
+	return ret;
 }
 
 int __sync_blockdev(struct block_device *bdev, int wait)

[cc:ing Jens Axboe]

I think we do need something like this, but I don't think this is the right place for it.

For normal filesystem writes, the blk_{start,finish}_plug calls are in generic_file_aio_write which is the "aio_write" function, or is called by it. aio_write calls generic_file_aio_write, calls blk_start_plug, call __generic_file_aio_write.

For block devices we bypass the generic_file_aio_write:

aio_write calls blkdev_aio_write calls __generic_file_aio_write - without calling blk_start_plug.
So I think the calls to blk_start_plug and blk_finish_plug should go in blkdev_aio_write. i.e. blkdev_aio_write should be made to look more like generic_file_aio_write (just without the mutex_lock/unlock).

Quoted 8 lines

If you redo the patch like that and test it I'll happily add my Reviewed-by.

I suspect it should be merged through Jens' tree.

Thanks,
NeilBrown

Thanks, i'll do it and resend the patch.