[PATCH] 10-times slower dm-raid1 O_SYNC

[PATCH] 10-times slower dm-raid1 O_SYNC

[Mikulas Patocka]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 717 bytes --]

Hi

Here is the patch to fix 10-times slower dm-raid1.

I realized that conditional unplug when someone is waiting on any IO (that 
I proposed on the phone call) is not correct --- it would leave the same 
problem with asynchronous IO (io_submit functions, etc.). So when kmirrord 
finishes all work, it unplugs the queue unconditionally.

The same bug exists with kcopyd, I fixed it to always use BIO_RW_SYNC bit. 
In kcopyd work routine it is not possible to find all devices that were 
involved in copying, so we don't know which queue to unplug. So the only 
way to fix it is to use synchronous submit.

Look at it and if you don't find any issues, I will post the patch to the 
customer for his testing.

Mikulas

[-- Attachment #2: Type: TEXT/PLAIN, Size: 4391 bytes --]

Avoid 3ms delay in dm-raid and kcopyd.

It is specified that any submitted bio without BIO_RW_SYNC flag may plug the
queue (i.e. block the requests from being dispatched to the physical device).

The queue is unplugged when the caller calls blk_unplug() function. Usually, the
sequence is that someone calls submit_bh to submit IO on a buffer. The IO plugs
the queue and waits (to be possibly joined with other adjacent bios). Then, when
the caller calls wait_on_buffer(), it unplugs the queue and submits the IOs to
the disk.

This was happenning:

When doing O_SYNC writes, function fsync_buffers_list() submits a list of
bios to dm_raid1, the bios are added to dm_raid1 write queue and kmirrord is
woken up.

fsync_buffers_list() calls wait_on_buffer() --- that unplugs the queue, but
there are no bios on the device queue (they are still in dm_raid1 queue)

wait_on_buffer() starts waiting until the IO is finished.

kmirrord is scheduled, kmirrord takes bios and submits them to the devices.

the submitted bio plugs the harddisk queue, there is no one to unplug it
(the process that called wait_on_buffer() is already sleeping)

So there is 3ms timeout, after which the queues on the harddisks are
unplugged, and requests are processed.

This 3ms timeout caused that in certain workloads (O_SYNC, 8kb writes), dm-raid1
is 10 times slower than md-raid1.


Every time we submit something asynchronously via dm_io, we must unplug the
queue to actually put the request to the device.

This patch adds unplug call to kmirrord --- while processing requests, it keeps
the queue plugged (so that adjacent bios can be merged), when it finishes
processing all bios, it unplugs the queue to submit the bios.

It also fixes kcopyd, that has the same potential problem. All kcopyd requests
are submitted with BIO_RW_SYNC.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-io.c     |   10 ++++++++--
 drivers/md/dm-kcopyd.c |    2 +-
 drivers/md/dm-raid1.c  |    2 ++
 3 files changed, 11 insertions(+), 3 deletions(-)

Index: linux-2.6.25-rc8/drivers/md/dm-raid1.c
===================================================================
--- linux-2.6.25-rc8.orig/drivers/md/dm-raid1.c	2008-04-03 18:45:09.000000000 +0200
+++ linux-2.6.25-rc8/drivers/md/dm-raid1.c	2008-04-21 21:55:34.000000000 +0200
@@ -1297,6 +1297,8 @@ static void do_mirror(struct work_struct
 	do_reads(ms, &reads);
 	do_writes(ms, &writes);
 	do_failures(ms, &failures);
+
+	dm_table_unplug_all(ms->ti->table);
 }
 
 
Index: linux-2.6.25-rc8/drivers/md/dm-io.c
===================================================================
--- linux-2.6.25-rc8.orig/drivers/md/dm-io.c	2008-04-21 22:32:21.000000000 +0200
+++ linux-2.6.25-rc8/drivers/md/dm-io.c	2008-04-21 22:43:14.000000000 +0200
@@ -353,7 +353,7 @@ static int sync_io(struct dm_io_client *
 {
 	struct io io;
 
-	if (num_regions > 1 && rw != WRITE) {
+	if (num_regions > 1 && (rw & RW_MASK) != WRITE) {
 		WARN_ON(1);
 		return -EIO;
 	}
@@ -390,7 +390,7 @@ static int async_io(struct dm_io_client 
 {
 	struct io *io;
 
-	if (num_regions > 1 && rw != WRITE) {
+	if (num_regions > 1 && (rw & RW_MASK) != WRITE) {
 		WARN_ON(1);
 		fn(1, context);
 		return -EIO;
@@ -437,6 +437,12 @@ static int dp_init(struct dm_io_request 
 
 /*
  * New collapsed (a)synchronous interface
+ *
+ * Attention:
+ * If the IO is asynchronous (i.e. it has notify.fn), you must either unplug the
+ * queue with blk_unplug() some times later or you must set BIO_RW_SYNC bit in
+ * io_req->bi_rw. If you fail to do one of these, the IO will be submitted to
+ * the disk with 3ms delay.
  */
 int dm_io(struct dm_io_request *io_req, unsigned num_regions,
 	  struct dm_io_region *where, unsigned long *sync_error_bits)
Index: linux-2.6.25-rc8/drivers/md/dm-kcopyd.c
===================================================================
--- linux-2.6.25-rc8.orig/drivers/md/dm-kcopyd.c	2008-04-03 18:45:08.000000000 +0200
+++ linux-2.6.25-rc8/drivers/md/dm-kcopyd.c	2008-04-21 22:34:41.000000000 +0200
@@ -333,7 +333,7 @@ static int run_io_job(struct kcopyd_job 
 {
 	int r;
 	struct dm_io_request io_req = {
-		.bi_rw = job->rw,
+		.bi_rw = job->rw | (1 << BIO_RW_SYNC),
 		.mem.type = DM_IO_PAGE_LIST,
 		.mem.ptr.pl = job->pages,
 		.mem.offset = job->offset,

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] 10-times slower dm-raid1 O_SYNC

[Jens Axboe]

On Mon, Apr 21 2008, Mikulas Patocka wrote:
> Hi
> 
> Here is the patch to fix 10-times slower dm-raid1.
> 
> I realized that conditional unplug when someone is waiting on any IO (that 
> I proposed on the phone call) is not correct --- it would leave the same 
> problem with asynchronous IO (io_submit functions, etc.). So when kmirrord 
> finishes all work, it unplugs the queue unconditionally.
> 
> The same bug exists with kcopyd, I fixed it to always use BIO_RW_SYNC bit. 
> In kcopyd work routine it is not possible to find all devices that were 
> involved in copying, so we don't know which queue to unplug. So the only 
> way to fix it is to use synchronous submit.
> 
> Look at it and if you don't find any issues, I will post the patch to the 
> customer for his testing.
> 
> Mikulas

> Avoid 3ms delay in dm-raid and kcopyd.
> 
> It is specified that any submitted bio without BIO_RW_SYNC flag may plug the
> queue (i.e. block the requests from being dispatched to the physical device).
> 
> The queue is unplugged when the caller calls blk_unplug() function. Usually, the
> sequence is that someone calls submit_bh to submit IO on a buffer. The IO plugs
> the queue and waits (to be possibly joined with other adjacent bios). Then, when
> the caller calls wait_on_buffer(), it unplugs the queue and submits the IOs to
> the disk.
> 
> This was happenning:
> 
> When doing O_SYNC writes, function fsync_buffers_list() submits a list of
> bios to dm_raid1, the bios are added to dm_raid1 write queue and kmirrord is
> woken up.
> 
> fsync_buffers_list() calls wait_on_buffer() --- that unplugs the queue, but
> there are no bios on the device queue (they are still in dm_raid1 queue)
> 
> wait_on_buffer() starts waiting until the IO is finished.
> 
> kmirrord is scheduled, kmirrord takes bios and submits them to the devices.
> 
> the submitted bio plugs the harddisk queue, there is no one to unplug it
> (the process that called wait_on_buffer() is already sleeping)
> 
> So there is 3ms timeout, after which the queues on the harddisks are
> unplugged, and requests are processed.
> 
> This 3ms timeout caused that in certain workloads (O_SYNC, 8kb writes), dm-raid1
> is 10 times slower than md-raid1.
> 
> 
> Every time we submit something asynchronously via dm_io, we must unplug the
> queue to actually put the request to the device.
> 
> This patch adds unplug call to kmirrord --- while processing requests, it keeps
> the queue plugged (so that adjacent bios can be merged), when it finishes
> processing all bios, it unplugs the queue to submit the bios.
> 
> It also fixes kcopyd, that has the same potential problem. All kcopyd requests
> are submitted with BIO_RW_SYNC.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm-io.c     |   10 ++++++++--
>  drivers/md/dm-kcopyd.c |    2 +-
>  drivers/md/dm-raid1.c  |    2 ++
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6.25-rc8/drivers/md/dm-raid1.c
> ===================================================================
> --- linux-2.6.25-rc8.orig/drivers/md/dm-raid1.c	2008-04-03 18:45:09.000000000 +0200
> +++ linux-2.6.25-rc8/drivers/md/dm-raid1.c	2008-04-21 21:55:34.000000000 +0200
> @@ -1297,6 +1297,8 @@ static void do_mirror(struct work_struct
>  	do_reads(ms, &reads);
>  	do_writes(ms, &writes);
>  	do_failures(ms, &failures);
> +
> +	dm_table_unplug_all(ms->ti->table);
>  }
>  
>  
> Index: linux-2.6.25-rc8/drivers/md/dm-io.c
> ===================================================================
> --- linux-2.6.25-rc8.orig/drivers/md/dm-io.c	2008-04-21 22:32:21.000000000 +0200
> +++ linux-2.6.25-rc8/drivers/md/dm-io.c	2008-04-21 22:43:14.000000000 +0200
> @@ -353,7 +353,7 @@ static int sync_io(struct dm_io_client *
>  {
>  	struct io io;
>  
> -	if (num_regions > 1 && rw != WRITE) {
> +	if (num_regions > 1 && (rw & RW_MASK) != WRITE) {
>  		WARN_ON(1);
>  		return -EIO;
>  	}
> @@ -390,7 +390,7 @@ static int async_io(struct dm_io_client 
>  {
>  	struct io *io;
>  
> -	if (num_regions > 1 && rw != WRITE) {
> +	if (num_regions > 1 && (rw & RW_MASK) != WRITE) {
>  		WARN_ON(1);
>  		fn(1, context);
>  		return -EIO;
> @@ -437,6 +437,12 @@ static int dp_init(struct dm_io_request 
>  
>  /*
>   * New collapsed (a)synchronous interface
> + *
> + * Attention:
> + * If the IO is asynchronous (i.e. it has notify.fn), you must either unplug the
> + * queue with blk_unplug() some times later or you must set BIO_RW_SYNC bit in
> + * io_req->bi_rw. If you fail to do one of these, the IO will be submitted to
> + * the disk with 3ms delay.
>   */
>  int dm_io(struct dm_io_request *io_req, unsigned num_regions,
>  	  struct dm_io_region *where, unsigned long *sync_error_bits)
> Index: linux-2.6.25-rc8/drivers/md/dm-kcopyd.c
> ===================================================================
> --- linux-2.6.25-rc8.orig/drivers/md/dm-kcopyd.c	2008-04-03 18:45:08.000000000 +0200
> +++ linux-2.6.25-rc8/drivers/md/dm-kcopyd.c	2008-04-21 22:34:41.000000000 +0200
> @@ -333,7 +333,7 @@ static int run_io_job(struct kcopyd_job 
>  {
>  	int r;
>  	struct dm_io_request io_req = {
> -		.bi_rw = job->rw,
> +		.bi_rw = job->rw | (1 << BIO_RW_SYNC),
>  		.mem.type = DM_IO_PAGE_LIST,
>  		.mem.ptr.pl = job->pages,
>  		.mem.offset = job->offset,

Looks ok to me.

Acked-by: Jens Axboe <jens.axboe@oracle.com>

-- 
Jens Axboe