Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-08-02 Thread Asias He

On 07/30/2012 09:44 PM, Christoph Hellwig wrote:

On Mon, Jul 30, 2012 at 09:31:06AM +0200, Paolo Bonzini wrote:

You only need to add REQ_FLUSH support.  The virtio-blk protocol does
not support REQ_FUA, because there's no easy way to do it in userspace.


A bio-based driver needs to handle both REQ_FLUSH and REQ_FUA as it does
not get the sequencing of REQ_FUA into REQ_FLUSH that request based drivers
can request.  To what the REQ_FUA request gets translated is a different story.


I just sent out V5 to support both REQ_FLUSH AND REQ_FUA.
Thanks, Christoph!

--
Asias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-08-02 Thread Asias He

On 07/30/2012 09:44 PM, Christoph Hellwig wrote:

On Mon, Jul 30, 2012 at 09:31:06AM +0200, Paolo Bonzini wrote:

You only need to add REQ_FLUSH support.  The virtio-blk protocol does
not support REQ_FUA, because there's no easy way to do it in userspace.


A bio-based driver needs to handle both REQ_FLUSH and REQ_FUA as it does
not get the sequencing of REQ_FUA into REQ_FLUSH that request based drivers
can request.  To what the REQ_FUA request gets translated is a different story.


I just sent out V5 to support both REQ_FLUSH AND REQ_FUA.
Thanks, Christoph!

--
Asias
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-31 Thread Asias He

On 07/30/2012 09:43 PM, Christoph Hellwig wrote:

On Mon, Jul 30, 2012 at 12:43:12PM +0800, Asias He wrote:

I think we can add REQ_FLUSH & REQ_FUA support to bio path and that
deserves another patch.


Adding it is a requirement for merging the code.



OK. Will add that.

--
Asias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-31 Thread Asias He

On 07/30/2012 09:43 PM, Christoph Hellwig wrote:

On Mon, Jul 30, 2012 at 12:43:12PM +0800, Asias He wrote:

I think we can add REQ_FLUSH  REQ_FUA support to bio path and that
deserves another patch.


Adding it is a requirement for merging the code.



OK. Will add that.

--
Asias
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-30 Thread Christoph Hellwig
On Mon, Jul 30, 2012 at 12:43:12PM +0800, Asias He wrote:
> I think we can add REQ_FLUSH & REQ_FUA support to bio path and that 
> deserves another patch.

Adding it is a requirement for merging the code.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-30 Thread Christoph Hellwig
On Mon, Jul 30, 2012 at 09:31:06AM +0200, Paolo Bonzini wrote:
> You only need to add REQ_FLUSH support.  The virtio-blk protocol does
> not support REQ_FUA, because there's no easy way to do it in userspace.

A bio-based driver needs to handle both REQ_FLUSH and REQ_FUA as it does
not get the sequencing of REQ_FUA into REQ_FLUSH that request based drivers
can request.  To what the REQ_FUA request gets translated is a different story.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-30 Thread Paolo Bonzini
Il 30/07/2012 06:43, Asias He ha scritto:
>>
> 
> Yes. Something like this:
> 
>   qemu -drive file=foo.img,cache=writeback/unsafe
> 
> is not safe against power losses also?

cache=writeback and cache=none are safe, cache=unsafe isn't.

> I think we can add REQ_FLUSH & REQ_FUA support to bio path and that
> deserves another patch.

You only need to add REQ_FLUSH support.  The virtio-blk protocol does
not support REQ_FUA, because there's no easy way to do it in userspace.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-30 Thread Paolo Bonzini
Il 30/07/2012 06:43, Asias He ha scritto:

 
 Yes. Something like this:
 
   qemu -drive file=foo.img,cache=writeback/unsafe
 
 is not safe against power losses also?

cache=writeback and cache=none are safe, cache=unsafe isn't.

 I think we can add REQ_FLUSH  REQ_FUA support to bio path and that
 deserves another patch.

You only need to add REQ_FLUSH support.  The virtio-blk protocol does
not support REQ_FUA, because there's no easy way to do it in userspace.

Paolo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-30 Thread Christoph Hellwig
On Mon, Jul 30, 2012 at 09:31:06AM +0200, Paolo Bonzini wrote:
 You only need to add REQ_FLUSH support.  The virtio-blk protocol does
 not support REQ_FUA, because there's no easy way to do it in userspace.

A bio-based driver needs to handle both REQ_FLUSH and REQ_FUA as it does
not get the sequencing of REQ_FUA into REQ_FLUSH that request based drivers
can request.  To what the REQ_FUA request gets translated is a different story.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-30 Thread Christoph Hellwig
On Mon, Jul 30, 2012 at 12:43:12PM +0800, Asias He wrote:
 I think we can add REQ_FLUSH  REQ_FUA support to bio path and that 
 deserves another patch.

Adding it is a requirement for merging the code.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-29 Thread Asias He

On 07/28/2012 02:42 PM, Paolo Bonzini wrote:

I'm not sure what the correct behavior for bio & cacheflush is, if
any.


REQ_FLUSH is not supported in the bio path.


Ouch, that's correct:

@@ -414,7 +529,7 @@ static void virtblk_update_cache_mode(struct virtio_device 
*vdev)
u8 writeback = virtblk_get_cache_mode(vdev);
struct virtio_blk *vblk = vdev->priv;

-   if (writeback)
+   if (writeback && !use_bio)
blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
else
blk_queue_flush(vblk->disk->queue, 0);

then it is not safe against power losses.


Yes. Something like this:

  qemu -drive file=foo.img,cache=writeback/unsafe

is not safe against power losses also?

I think we can add REQ_FLUSH & REQ_FUA support to bio path and that 
deserves another patch.


--
Asias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-29 Thread Asias He

On 07/29/2012 08:59 PM, Michael S. Tsirkin wrote:

On Sat, Jul 28, 2012 at 10:38:41AM +0800, Asias He wrote:

On 07/27/2012 08:33 AM, Rusty Russell wrote:

On Fri, 13 Jul 2012 16:38:51 +0800, Asias He  wrote:

Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
use_bio=1' to enable ->make_request_fn() based I/O path.


This patch conflicts with Paolo's Bonzini's 'virtio-blk: allow toggling
host cache between writeback and writethrough' which is also queued (see
linux-next).


Rebased against Paolo's patch in V4.


I'm not sure what the correct behavior for bio & cacheflush is, if any.


REQ_FLUSH is not supported in the bio path.


But as to the patch itself: it's a hack.

1) Leaving the guest's admin to turn on the switch is a terrible choice.
2) The block layer should stop merging and sorting when a device is
fast, not the driver.
3) I pointed out that slow disks have low IOPS, so why is this
conditional?  Sure, more guest exits, but it's still a small number
for a slow device.
4) The only case where we want merging is on a slow device when the host
isn't doing it.

Now, despite this, I'm prepared to commit it.  But in my mind it's a
hack: we should aim for use_bio to be based on a feature bit fed from
the host, and use the module parameter only if we want to override it.


OK. A feature bit from host sound like a choice but a switch is also
needed on host side.


qemu automatically gives you the ability to control
any feature bit.


Automatically?


And for other OS, e.g. Windows, the bio thing
does not apply at all.


Let's try to define when it's a good idea. Is it a hint to guest that
backend handles small accesses efficiently so ok to disable batching?


Yes. It's also a hint for latency reduction.


Anyway, I have to admit that adding a module parameter here is not
the best choice. Let's think more.

--
Asias



--
Asias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-29 Thread Michael S. Tsirkin
On Sat, Jul 28, 2012 at 10:38:41AM +0800, Asias He wrote:
> On 07/27/2012 08:33 AM, Rusty Russell wrote:
> >On Fri, 13 Jul 2012 16:38:51 +0800, Asias He  wrote:
> >>Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
> >>use_bio=1' to enable ->make_request_fn() based I/O path.
> >
> >This patch conflicts with Paolo's Bonzini's 'virtio-blk: allow toggling
> >host cache between writeback and writethrough' which is also queued (see
> >linux-next).
> 
> Rebased against Paolo's patch in V4.
> 
> >I'm not sure what the correct behavior for bio & cacheflush is, if any.
> 
> REQ_FLUSH is not supported in the bio path.
> 
> >But as to the patch itself: it's a hack.
> >
> >1) Leaving the guest's admin to turn on the switch is a terrible choice.
> >2) The block layer should stop merging and sorting when a device is
> >fast, not the driver.
> >3) I pointed out that slow disks have low IOPS, so why is this
> >conditional?  Sure, more guest exits, but it's still a small number
> >for a slow device.
> >4) The only case where we want merging is on a slow device when the host
> >isn't doing it.
> >
> >Now, despite this, I'm prepared to commit it.  But in my mind it's a
> >hack: we should aim for use_bio to be based on a feature bit fed from
> >the host, and use the module parameter only if we want to override it.
> 
> OK. A feature bit from host sound like a choice but a switch is also
> needed on host side.

qemu automatically gives you the ability to control
any feature bit.

> And for other OS, e.g. Windows, the bio thing
> does not apply at all.

Let's try to define when it's a good idea. Is it a hint to guest that
backend handles small accesses efficiently so ok to disable batching?

> Anyway, I have to admit that adding a module parameter here is not
> the best choice. Let's think more.
> 
> -- 
> Asias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-29 Thread Michael S. Tsirkin
On Sat, Jul 28, 2012 at 10:38:41AM +0800, Asias He wrote:
 On 07/27/2012 08:33 AM, Rusty Russell wrote:
 On Fri, 13 Jul 2012 16:38:51 +0800, Asias He as...@redhat.com wrote:
 Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
 use_bio=1' to enable -make_request_fn() based I/O path.
 
 This patch conflicts with Paolo's Bonzini's 'virtio-blk: allow toggling
 host cache between writeback and writethrough' which is also queued (see
 linux-next).
 
 Rebased against Paolo's patch in V4.
 
 I'm not sure what the correct behavior for bio  cacheflush is, if any.
 
 REQ_FLUSH is not supported in the bio path.
 
 But as to the patch itself: it's a hack.
 
 1) Leaving the guest's admin to turn on the switch is a terrible choice.
 2) The block layer should stop merging and sorting when a device is
 fast, not the driver.
 3) I pointed out that slow disks have low IOPS, so why is this
 conditional?  Sure, more guest exits, but it's still a small number
 for a slow device.
 4) The only case where we want merging is on a slow device when the host
 isn't doing it.
 
 Now, despite this, I'm prepared to commit it.  But in my mind it's a
 hack: we should aim for use_bio to be based on a feature bit fed from
 the host, and use the module parameter only if we want to override it.
 
 OK. A feature bit from host sound like a choice but a switch is also
 needed on host side.

qemu automatically gives you the ability to control
any feature bit.

 And for other OS, e.g. Windows, the bio thing
 does not apply at all.

Let's try to define when it's a good idea. Is it a hint to guest that
backend handles small accesses efficiently so ok to disable batching?

 Anyway, I have to admit that adding a module parameter here is not
 the best choice. Let's think more.
 
 -- 
 Asias
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-29 Thread Asias He

On 07/29/2012 08:59 PM, Michael S. Tsirkin wrote:

On Sat, Jul 28, 2012 at 10:38:41AM +0800, Asias He wrote:

On 07/27/2012 08:33 AM, Rusty Russell wrote:

On Fri, 13 Jul 2012 16:38:51 +0800, Asias He as...@redhat.com wrote:

Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
use_bio=1' to enable -make_request_fn() based I/O path.


This patch conflicts with Paolo's Bonzini's 'virtio-blk: allow toggling
host cache between writeback and writethrough' which is also queued (see
linux-next).


Rebased against Paolo's patch in V4.


I'm not sure what the correct behavior for bio  cacheflush is, if any.


REQ_FLUSH is not supported in the bio path.


But as to the patch itself: it's a hack.

1) Leaving the guest's admin to turn on the switch is a terrible choice.
2) The block layer should stop merging and sorting when a device is
fast, not the driver.
3) I pointed out that slow disks have low IOPS, so why is this
conditional?  Sure, more guest exits, but it's still a small number
for a slow device.
4) The only case where we want merging is on a slow device when the host
isn't doing it.

Now, despite this, I'm prepared to commit it.  But in my mind it's a
hack: we should aim for use_bio to be based on a feature bit fed from
the host, and use the module parameter only if we want to override it.


OK. A feature bit from host sound like a choice but a switch is also
needed on host side.


qemu automatically gives you the ability to control
any feature bit.


Automatically?


And for other OS, e.g. Windows, the bio thing
does not apply at all.


Let's try to define when it's a good idea. Is it a hint to guest that
backend handles small accesses efficiently so ok to disable batching?


Yes. It's also a hint for latency reduction.


Anyway, I have to admit that adding a module parameter here is not
the best choice. Let's think more.

--
Asias



--
Asias
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-29 Thread Asias He

On 07/28/2012 02:42 PM, Paolo Bonzini wrote:

I'm not sure what the correct behavior for bio  cacheflush is, if
any.


REQ_FLUSH is not supported in the bio path.


Ouch, that's correct:

@@ -414,7 +529,7 @@ static void virtblk_update_cache_mode(struct virtio_device 
*vdev)
u8 writeback = virtblk_get_cache_mode(vdev);
struct virtio_blk *vblk = vdev-priv;

-   if (writeback)
+   if (writeback  !use_bio)
blk_queue_flush(vblk-disk-queue, REQ_FLUSH);
else
blk_queue_flush(vblk-disk-queue, 0);

then it is not safe against power losses.


Yes. Something like this:

  qemu -drive file=foo.img,cache=writeback/unsafe

is not safe against power losses also?

I think we can add REQ_FLUSH  REQ_FUA support to bio path and that 
deserves another patch.


--
Asias
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-28 Thread Paolo Bonzini
> > I'm not sure what the correct behavior for bio & cacheflush is, if
> > any.
> 
> REQ_FLUSH is not supported in the bio path.

Ouch, that's correct:

@@ -414,7 +529,7 @@ static void virtblk_update_cache_mode(struct virtio_device 
*vdev)
u8 writeback = virtblk_get_cache_mode(vdev);
struct virtio_blk *vblk = vdev->priv;
 
-   if (writeback)
+   if (writeback && !use_bio)
blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
else
blk_queue_flush(vblk->disk->queue, 0);

then it is not safe against power losses.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-28 Thread Paolo Bonzini
  I'm not sure what the correct behavior for bio  cacheflush is, if
  any.
 
 REQ_FLUSH is not supported in the bio path.

Ouch, that's correct:

@@ -414,7 +529,7 @@ static void virtblk_update_cache_mode(struct virtio_device 
*vdev)
u8 writeback = virtblk_get_cache_mode(vdev);
struct virtio_blk *vblk = vdev-priv;
 
-   if (writeback)
+   if (writeback  !use_bio)
blk_queue_flush(vblk-disk-queue, REQ_FLUSH);
else
blk_queue_flush(vblk-disk-queue, 0);

then it is not safe against power losses.

Paolo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-27 Thread Asias He

On 07/27/2012 08:33 AM, Rusty Russell wrote:

On Fri, 13 Jul 2012 16:38:51 +0800, Asias He  wrote:

Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
use_bio=1' to enable ->make_request_fn() based I/O path.


This patch conflicts with Paolo's Bonzini's 'virtio-blk: allow toggling
host cache between writeback and writethrough' which is also queued (see
linux-next).


Rebased against Paolo's patch in V4.


I'm not sure what the correct behavior for bio & cacheflush is, if any.


REQ_FLUSH is not supported in the bio path.


But as to the patch itself: it's a hack.

1) Leaving the guest's admin to turn on the switch is a terrible choice.
2) The block layer should stop merging and sorting when a device is
fast, not the driver.
3) I pointed out that slow disks have low IOPS, so why is this
conditional?  Sure, more guest exits, but it's still a small number
for a slow device.
4) The only case where we want merging is on a slow device when the host
isn't doing it.

Now, despite this, I'm prepared to commit it.  But in my mind it's a
hack: we should aim for use_bio to be based on a feature bit fed from
the host, and use the module parameter only if we want to override it.


OK. A feature bit from host sound like a choice but a switch is also 
needed on host side. And for other OS, e.g. Windows, the bio thing does 
not apply at all.


Anyway, I have to admit that adding a module parameter here is not the 
best choice. Let's think more.


--
Asias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-27 Thread Rusty Russell
On Fri, 13 Jul 2012 16:38:51 +0800, Asias He  wrote:
> This patch introduces bio-based IO path for virtio-blk.

Acked-by: Rusty Russell 

I just hope we can do better than a module option in future.

Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-27 Thread Rusty Russell
On Fri, 13 Jul 2012 16:38:51 +0800, Asias He as...@redhat.com wrote:
 This patch introduces bio-based IO path for virtio-blk.

Acked-by: Rusty Russell ru...@rustcorp.com.au

I just hope we can do better than a module option in future.

Thanks,
Rusty.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-27 Thread Asias He

On 07/27/2012 08:33 AM, Rusty Russell wrote:

On Fri, 13 Jul 2012 16:38:51 +0800, Asias He as...@redhat.com wrote:

Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
use_bio=1' to enable -make_request_fn() based I/O path.


This patch conflicts with Paolo's Bonzini's 'virtio-blk: allow toggling
host cache between writeback and writethrough' which is also queued (see
linux-next).


Rebased against Paolo's patch in V4.


I'm not sure what the correct behavior for bio  cacheflush is, if any.


REQ_FLUSH is not supported in the bio path.


But as to the patch itself: it's a hack.

1) Leaving the guest's admin to turn on the switch is a terrible choice.
2) The block layer should stop merging and sorting when a device is
fast, not the driver.
3) I pointed out that slow disks have low IOPS, so why is this
conditional?  Sure, more guest exits, but it's still a small number
for a slow device.
4) The only case where we want merging is on a slow device when the host
isn't doing it.

Now, despite this, I'm prepared to commit it.  But in my mind it's a
hack: we should aim for use_bio to be based on a feature bit fed from
the host, and use the module parameter only if we want to override it.


OK. A feature bit from host sound like a choice but a switch is also 
needed on host side. And for other OS, e.g. Windows, the bio thing does 
not apply at all.


Anyway, I have to admit that adding a module parameter here is not the 
best choice. Let's think more.


--
Asias
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-26 Thread Rusty Russell
On Fri, 13 Jul 2012 16:38:51 +0800, Asias He  wrote:
> Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
> use_bio=1' to enable ->make_request_fn() based I/O path.

This patch conflicts with Paolo's Bonzini's 'virtio-blk: allow toggling
host cache between writeback and writethrough' which is also queued (see
linux-next).

I'm not sure what the correct behavior for bio & cacheflush is, if any.

But as to the patch itself: it's a hack.

1) Leaving the guest's admin to turn on the switch is a terrible choice.
2) The block layer should stop merging and sorting when a device is
   fast, not the driver.
3) I pointed out that slow disks have low IOPS, so why is this
   conditional?  Sure, more guest exits, but it's still a small number
   for a slow device.
4) The only case where we want merging is on a slow device when the host
   isn't doing it.

Now, despite this, I'm prepared to commit it.  But in my mind it's a
hack: we should aim for use_bio to be based on a feature bit fed from
the host, and use the module parameter only if we want to override it.

Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-26 Thread Rusty Russell
On Fri, 13 Jul 2012 16:38:51 +0800, Asias He as...@redhat.com wrote:
 Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
 use_bio=1' to enable -make_request_fn() based I/O path.

This patch conflicts with Paolo's Bonzini's 'virtio-blk: allow toggling
host cache between writeback and writethrough' which is also queued (see
linux-next).

I'm not sure what the correct behavior for bio  cacheflush is, if any.

But as to the patch itself: it's a hack.

1) Leaving the guest's admin to turn on the switch is a terrible choice.
2) The block layer should stop merging and sorting when a device is
   fast, not the driver.
3) I pointed out that slow disks have low IOPS, so why is this
   conditional?  Sure, more guest exits, but it's still a small number
   for a slow device.
4) The only case where we want merging is on a slow device when the host
   isn't doing it.

Now, despite this, I'm prepared to commit it.  But in my mind it's a
hack: we should aim for use_bio to be based on a feature bit fed from
the host, and use the module parameter only if we want to override it.

Cheers,
Rusty.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-13 Thread Asias He
This patch introduces bio-based IO path for virtio-blk.

Compared to request-based IO path, bio-based IO path uses driver
provided ->make_request_fn() method to bypasses the IO scheduler. It
handles the bio to device directly without allocating a request in block
layer. This reduces the IO path in guest kernel to achieve high IOPS
and lower latency. The downside is that guest can not use the IO
scheduler to merge and sort requests. However, this is not a big problem
if the backend disk in host side uses faster disk device.

When the bio-based IO path is not enabled, virtio-blk still uses the
original request-based IO path, no performance difference is observed.

Performance evaluation:
-
1) Fio test is performed in a 8 vcpu guest with ramdisk based guest using
kvm tool.

Short version:
 With bio-based IO path, sequential read/write, random read/write
 IOPS boost : 28%, 24%, 21%, 16%
 Latency improvement: 32%, 17%, 21%, 16%

Long version:
 With bio-based IO path:
  seq-read  : io=2048.0MB, bw=116996KB/s, iops=233991 , runt= 17925msec
  seq-write : io=2048.0MB, bw=100829KB/s, iops=201658 , runt= 20799msec
  rand-read : io=3095.7MB, bw=112134KB/s, iops=224268 , runt= 28269msec
  rand-write: io=3095.7MB, bw=96198KB/s,  iops=192396 , runt= 32952msec
clat (usec): min=0 , max=2631.6K, avg=58716.99, stdev=191377.30
clat (usec): min=0 , max=1753.2K, avg=66423.25, stdev=81774.35
clat (usec): min=0 , max=2915.5K, avg=61685.70, stdev=120598.39
clat (usec): min=0 , max=1933.4K, avg=76935.12, stdev=96603.45
  cpu : usr=74.08%, sys=703.84%, ctx=29661403, majf=21354, minf=22460954
  cpu : usr=70.92%, sys=702.81%, ctx=77219828, majf=13980, minf=27713137
  cpu : usr=72.23%, sys=695.37%, ctx=88081059, majf=18475, minf=28177648
  cpu : usr=69.69%, sys=654.13%, ctx=145476035, majf=15867, minf=26176375
 With request-based IO path:
  seq-read  : io=2048.0MB, bw=91074KB/s, iops=182147 , runt= 23027msec
  seq-write : io=2048.0MB, bw=80725KB/s, iops=161449 , runt= 25979msec
  rand-read : io=3095.7MB, bw=92106KB/s, iops=184211 , runt= 34416msec
  rand-write: io=3095.7MB, bw=82815KB/s, iops=165630 , runt= 38277msec
clat (usec): min=0 , max=1932.4K, avg=77824.17, stdev=170339.49
clat (usec): min=0 , max=2510.2K, avg=78023.96, stdev=146949.15
clat (usec): min=0 , max=3037.2K, avg=74746.53, stdev=128498.27
clat (usec): min=0 , max=1363.4K, avg=89830.75, stdev=114279.68
  cpu : usr=53.28%, sys=724.19%, ctx=37988895, majf=17531, minf=23577622
  cpu : usr=49.03%, sys=633.20%, ctx=205935380, majf=18197, minf=27288959
  cpu : usr=55.78%, sys=722.40%, ctx=101525058, majf=19273, minf=28067082
  cpu : usr=56.55%, sys=690.83%, ctx=228205022, majf=18039, minf=26551985

2) Fio test is performed in a 8 vcpu guest with Fusion-IO based guest using
kvm tool.

Short version:
 With bio-based IO path, sequential read/write, random read/write
 IOPS boost : 11%, 11%, 13%, 10%
 Latency improvement: 10%, 10%, 12%, 10%
Long Version:
 With bio-based IO path:
  read : io=2048.0MB, bw=58920KB/s, iops=117840 , runt= 35593msec
  write: io=2048.0MB, bw=64308KB/s, iops=128616 , runt= 32611msec
  read : io=3095.7MB, bw=59633KB/s, iops=119266 , runt= 53157msec
  write: io=3095.7MB, bw=62993KB/s, iops=125985 , runt= 50322msec
clat (usec): min=0 , max=1284.3K, avg=128109.01, stdev=71513.29
clat (usec): min=94 , max=962339 , avg=116832.95, stdev=65836.80
clat (usec): min=0 , max=1846.6K, avg=128509.99, stdev=89575.07
clat (usec): min=0 , max=2256.4K, avg=121361.84, stdev=82747.25
  cpu : usr=56.79%, sys=421.70%, ctx=147335118, majf=21080, minf=19852517
  cpu : usr=61.81%, sys=455.53%, ctx=143269950, majf=16027, minf=24800604
  cpu : usr=63.10%, sys=455.38%, ctx=178373538, majf=16958, minf=24822612
  cpu : usr=62.04%, sys=453.58%, ctx=226902362, majf=16089, minf=23278105
 With request-based IO path:
  read : io=2048.0MB, bw=52896KB/s, iops=105791 , runt= 39647msec
  write: io=2048.0MB, bw=57856KB/s, iops=115711 , runt= 36248msec
  read : io=3095.7MB, bw=52387KB/s, iops=104773 , runt= 60510msec
  write: io=3095.7MB, bw=57310KB/s, iops=114619 , runt= 55312msec
clat (usec): min=0 , max=1532.6K, avg=142085.62, stdev=109196.84
clat (usec): min=0 , max=1487.4K, avg=129110.71, stdev=114973.64
clat (usec): min=0 , max=1388.6K, avg=145049.22, stdev=107232.55
clat (usec): min=0 , max=1465.9K, avg=133585.67, stdev=110322.95
  cpu : usr=44.08%, sys=590.71%, ctx=451812322, majf=14841, minf=17648641
  cpu : usr=48.73%, sys=610.78%, ctx=418953997, majf=22164, minf=26850689
  cpu : usr=45.58%, sys=581.16%, ctx=714079216, majf=21497, minf=22558223
  cpu : usr=48.40%, sys=599.65%, ctx=656089423, majf=16393, minf=23824409

How to use:
-
Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
use_bio=1' to enable ->make_request_fn() based I/O path.

Cc: Rusty Russell 
Cc: "Michael S. Tsirkin" 
Cc: virtualizat...@lists.linux-foundation.org
Cc: 

[PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk

2012-07-13 Thread Asias He
This patch introduces bio-based IO path for virtio-blk.

Compared to request-based IO path, bio-based IO path uses driver
provided -make_request_fn() method to bypasses the IO scheduler. It
handles the bio to device directly without allocating a request in block
layer. This reduces the IO path in guest kernel to achieve high IOPS
and lower latency. The downside is that guest can not use the IO
scheduler to merge and sort requests. However, this is not a big problem
if the backend disk in host side uses faster disk device.

When the bio-based IO path is not enabled, virtio-blk still uses the
original request-based IO path, no performance difference is observed.

Performance evaluation:
-
1) Fio test is performed in a 8 vcpu guest with ramdisk based guest using
kvm tool.

Short version:
 With bio-based IO path, sequential read/write, random read/write
 IOPS boost : 28%, 24%, 21%, 16%
 Latency improvement: 32%, 17%, 21%, 16%

Long version:
 With bio-based IO path:
  seq-read  : io=2048.0MB, bw=116996KB/s, iops=233991 , runt= 17925msec
  seq-write : io=2048.0MB, bw=100829KB/s, iops=201658 , runt= 20799msec
  rand-read : io=3095.7MB, bw=112134KB/s, iops=224268 , runt= 28269msec
  rand-write: io=3095.7MB, bw=96198KB/s,  iops=192396 , runt= 32952msec
clat (usec): min=0 , max=2631.6K, avg=58716.99, stdev=191377.30
clat (usec): min=0 , max=1753.2K, avg=66423.25, stdev=81774.35
clat (usec): min=0 , max=2915.5K, avg=61685.70, stdev=120598.39
clat (usec): min=0 , max=1933.4K, avg=76935.12, stdev=96603.45
  cpu : usr=74.08%, sys=703.84%, ctx=29661403, majf=21354, minf=22460954
  cpu : usr=70.92%, sys=702.81%, ctx=77219828, majf=13980, minf=27713137
  cpu : usr=72.23%, sys=695.37%, ctx=88081059, majf=18475, minf=28177648
  cpu : usr=69.69%, sys=654.13%, ctx=145476035, majf=15867, minf=26176375
 With request-based IO path:
  seq-read  : io=2048.0MB, bw=91074KB/s, iops=182147 , runt= 23027msec
  seq-write : io=2048.0MB, bw=80725KB/s, iops=161449 , runt= 25979msec
  rand-read : io=3095.7MB, bw=92106KB/s, iops=184211 , runt= 34416msec
  rand-write: io=3095.7MB, bw=82815KB/s, iops=165630 , runt= 38277msec
clat (usec): min=0 , max=1932.4K, avg=77824.17, stdev=170339.49
clat (usec): min=0 , max=2510.2K, avg=78023.96, stdev=146949.15
clat (usec): min=0 , max=3037.2K, avg=74746.53, stdev=128498.27
clat (usec): min=0 , max=1363.4K, avg=89830.75, stdev=114279.68
  cpu : usr=53.28%, sys=724.19%, ctx=37988895, majf=17531, minf=23577622
  cpu : usr=49.03%, sys=633.20%, ctx=205935380, majf=18197, minf=27288959
  cpu : usr=55.78%, sys=722.40%, ctx=101525058, majf=19273, minf=28067082
  cpu : usr=56.55%, sys=690.83%, ctx=228205022, majf=18039, minf=26551985

2) Fio test is performed in a 8 vcpu guest with Fusion-IO based guest using
kvm tool.

Short version:
 With bio-based IO path, sequential read/write, random read/write
 IOPS boost : 11%, 11%, 13%, 10%
 Latency improvement: 10%, 10%, 12%, 10%
Long Version:
 With bio-based IO path:
  read : io=2048.0MB, bw=58920KB/s, iops=117840 , runt= 35593msec
  write: io=2048.0MB, bw=64308KB/s, iops=128616 , runt= 32611msec
  read : io=3095.7MB, bw=59633KB/s, iops=119266 , runt= 53157msec
  write: io=3095.7MB, bw=62993KB/s, iops=125985 , runt= 50322msec
clat (usec): min=0 , max=1284.3K, avg=128109.01, stdev=71513.29
clat (usec): min=94 , max=962339 , avg=116832.95, stdev=65836.80
clat (usec): min=0 , max=1846.6K, avg=128509.99, stdev=89575.07
clat (usec): min=0 , max=2256.4K, avg=121361.84, stdev=82747.25
  cpu : usr=56.79%, sys=421.70%, ctx=147335118, majf=21080, minf=19852517
  cpu : usr=61.81%, sys=455.53%, ctx=143269950, majf=16027, minf=24800604
  cpu : usr=63.10%, sys=455.38%, ctx=178373538, majf=16958, minf=24822612
  cpu : usr=62.04%, sys=453.58%, ctx=226902362, majf=16089, minf=23278105
 With request-based IO path:
  read : io=2048.0MB, bw=52896KB/s, iops=105791 , runt= 39647msec
  write: io=2048.0MB, bw=57856KB/s, iops=115711 , runt= 36248msec
  read : io=3095.7MB, bw=52387KB/s, iops=104773 , runt= 60510msec
  write: io=3095.7MB, bw=57310KB/s, iops=114619 , runt= 55312msec
clat (usec): min=0 , max=1532.6K, avg=142085.62, stdev=109196.84
clat (usec): min=0 , max=1487.4K, avg=129110.71, stdev=114973.64
clat (usec): min=0 , max=1388.6K, avg=145049.22, stdev=107232.55
clat (usec): min=0 , max=1465.9K, avg=133585.67, stdev=110322.95
  cpu : usr=44.08%, sys=590.71%, ctx=451812322, majf=14841, minf=17648641
  cpu : usr=48.73%, sys=610.78%, ctx=418953997, majf=22164, minf=26850689
  cpu : usr=45.58%, sys=581.16%, ctx=714079216, majf=21497, minf=22558223
  cpu : usr=48.40%, sys=599.65%, ctx=656089423, majf=16393, minf=23824409

How to use:
-
Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
use_bio=1' to enable -make_request_fn() based I/O path.

Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Michael S. Tsirkin m...@redhat.com
Cc: