[PATCH] Remove BIO_RW_SYNCIO flag from kcopyd

[PATCH] Remove BIO_RW_SYNCIO flag from kcopyd

[Mikulas Patocka]

Remove BIO_RW_SYNCIO flag from kcopyd

This improves write throughput twice when writing to the origin with snapshot
on the same device.

This helps for CFQ I/O scheduler that has separate queues for sync and async
I/O, async is optimized for throughput, sync for latency.
Reallocations are triggered by writes that are usually async, so mark it
as async as well.

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

---
 drivers/md/dm-kcopyd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.36-rc7-fast/drivers/md/dm-kcopyd.c
===================================================================
--- linux-2.6.36-rc7-fast.orig/drivers/md/dm-kcopyd.c	2010-10-07 16:09:13.000000000 +0200
+++ linux-2.6.36-rc7-fast/drivers/md/dm-kcopyd.c	2010-10-15 03:18:52.000000000 +0200
@@ -345,7 +345,7 @@ static int run_io_job(struct kcopyd_job 
 {
 	int r;
 	struct dm_io_request io_req = {
-		.bi_rw = job->rw | REQ_SYNC | REQ_UNPLUG,
+		.bi_rw = job->rw | REQ_UNPLUG,
 		.mem.type = DM_IO_PAGE_LIST,
 		.mem.ptr.pl = job->pages,
 		.mem.offset = job->offset,

Re: Remove BIO_RW_SYNCIO flag from kcopyd

[Mike Snitzer]

Hi Mikulas,

On Sun, Oct 24 2010 at  9:02pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Remove BIO_RW_SYNCIO flag from kcopyd
> 
> This improves write throughput twice when writing to the origin with snapshot
> on the same device.

Can you please be more specific on the performance improvement you
observed?  You're saying something like: baseline=10MB/s patched=20MB/s?

Did you use a larger chunk_size for the snapshot device (e.g. 512K)?

I can't reproduce such significant performance improvements but my testbed
may be overly constrained (I'm passing an entire disk from host into KVM
guest using virtio).  I can look to test on bare metal early next week
(unless you have more insight that would obviate the need to do so).

> This helps for CFQ I/O scheduler that has separate queues for sync and async
> I/O, async is optimized for throughput, sync for latency.
> Reallocations are triggered by writes that are usually async, so mark it
> as async as well.

This change is at odds with a previous patch you made to reduce latency
of SYNC IO, see linux-2.6 commit:
7ff14a36159d947872870  "dm: unplug queues in threads"

So we're now trading latency for throughput?  Given the concerns about
snapshot-origin throughput (on lvm-general) I think that is reasonable.
But I just want to make sure we're consciously favoring throughput over
latency (I'll likely update the patch header to explicitly say as much).

> Index: linux-2.6.36-rc7-fast/drivers/md/dm-kcopyd.c
> ===================================================================
> --- linux-2.6.36-rc7-fast.orig/drivers/md/dm-kcopyd.c	2010-10-07 16:09:13.000000000 +0200
> +++ linux-2.6.36-rc7-fast/drivers/md/dm-kcopyd.c	2010-10-15 03:18:52.000000000 +0200
> @@ -345,7 +345,7 @@ static int run_io_job(struct kcopyd_job 
>  {
>  	int r;
>  	struct dm_io_request io_req = {
> -		.bi_rw = job->rw | REQ_SYNC | REQ_UNPLUG,
> +		.bi_rw = job->rw | REQ_UNPLUG,
>  		.mem.type = DM_IO_PAGE_LIST,
>  		.mem.ptr.pl = job->pages,
>  		.mem.offset = job->offset,
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: Remove BIO_RW_SYNCIO flag from kcopyd

[Mikulas Patocka]

Hi

On Fri, 19 Nov 2010, Mike Snitzer wrote:

> Hi Mikulas,
> 
> On Sun, Oct 24 2010 at  9:02pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > Remove BIO_RW_SYNCIO flag from kcopyd
> > 
> > This improves write throughput twice when writing to the origin with snapshot
> > on the same device.
> 
> Can you please be more specific on the performance improvement you
> observed?  You're saying something like: baseline=10MB/s patched=20MB/s?

Yes. I rechecked it now, and it is approximatelly so:
(for chunk size 4k, 32k, 512k)
unpatched: 8.5, 8.6, 9.3 MB/s
this patch: 15.2, 18.5, 17.5 MB/s

> Did you use a larger chunk_size for the snapshot device (e.g. 512K)?

It has effect for all chunk sizes.

> I can't reproduce such significant performance improvements but my testbed
> may be overly constrained (I'm passing an entire disk from host into KVM
> guest using virtio).  I can look to test on bare metal early next week
> (unless you have more insight that would obviate the need to do so).

My scenario is: LSI MPT controller, U320 15k SCSI disk, 64GiB file mapped 
with dm-loop, volume group on dm-loop, with both snapshot and origin in 
it.

> > This helps for CFQ I/O scheduler that has separate queues for sync and async
> > I/O, async is optimized for throughput, sync for latency.
> > Reallocations are triggered by writes that are usually async, so mark it
> > as async as well.
> 
> This change is at odds with a previous patch you made to reduce latency
> of SYNC IO, see linux-2.6 commit:
> 7ff14a36159d947872870  "dm: unplug queues in threads"
> 
> So we're now trading latency for throughput?

Yes. I think throughput matters more than latency for writes.

> Given the concerns about
> snapshot-origin throughput (on lvm-general) I think that is reasonable.
> But I just want to make sure we're consciously favoring throughput over
> latency (I'll likely update the patch header to explicitly say as much).

Yes. Write it there.

If someone need latency for writes, we could propagate the SYNC flag from 
the original write bio here. But if no one needs it, it would be needless 
complication.

Mikulas

> > Index: linux-2.6.36-rc7-fast/drivers/md/dm-kcopyd.c
> > ===================================================================
> > --- linux-2.6.36-rc7-fast.orig/drivers/md/dm-kcopyd.c	2010-10-07 16:09:13.000000000 +0200
> > +++ linux-2.6.36-rc7-fast/drivers/md/dm-kcopyd.c	2010-10-15 03:18:52.000000000 +0200
> > @@ -345,7 +345,7 @@ static int run_io_job(struct kcopyd_job 
> >  {
> >  	int r;
> >  	struct dm_io_request io_req = {
> > -		.bi_rw = job->rw | REQ_SYNC | REQ_UNPLUG,
> > +		.bi_rw = job->rw | REQ_UNPLUG,
> >  		.mem.type = DM_IO_PAGE_LIST,
> >  		.mem.ptr.pl = job->pages,
> >  		.mem.offset = job->offset,
> > 
> > --
> > dm-devel mailing list
> > dm-devel@redhat.com
> > https://www.redhat.com/mailman/listinfo/dm-devel
> 

[PATCH v2] dm kcopyd: remove SYNC from flags used in dm_io_request

[Mike Snitzer]

From: Mikulas Patocka <mpatocka@redhat.com>

Removing the REQ_SYNC flag improves write throughput twice when writing
to the origin with a snapshot on the same device (using the CFQ I/O
scheduler).

Sequential write throughput (chunksize of 4k, 32k, 512k)
unpatched:  8.5,  8.6,  9.3 MB/s
patched:   15.2, 18.5, 17.5 MB/s

Snapshot exception reallocations are triggered by writes that are
usually async, so mark the associated dm_io_request as async as well.
This helps when using the CFQ I/O scheduler because it has separate
queues for sync and async I/O, async is optimized for throughput, sync
for latency.  With this change we're consciously favoring throughput
over latency.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-kcopyd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

v2: updated patch header

Index: linux-2.6.36-rc7-fast/drivers/md/dm-kcopyd.c
===================================================================
--- linux-2.6.36-rc7-fast.orig/drivers/md/dm-kcopyd.c	2010-10-07 16:09:13.000000000 +0200
+++ linux-2.6.36-rc7-fast/drivers/md/dm-kcopyd.c	2010-10-15 03:18:52.000000000 +0200
@@ -345,7 +345,7 @@ static int run_io_job(struct kcopyd_job 
 {
 	int r;
 	struct dm_io_request io_req = {
-		.bi_rw = job->rw | REQ_SYNC | REQ_UNPLUG,
+		.bi_rw = job->rw | REQ_UNPLUG,
 		.mem.type = DM_IO_PAGE_LIST,
 		.mem.ptr.pl = job->pages,
 		.mem.offset = job->offset,