Patchwork qemu-img: let qemu-img behave as the manual advertise

login
register
mail settings
Submitter namei.unix@gmail.com
Date April 10, 2012, 6:10 p.m.
Message ID <1334081438-10404-1-git-send-email-namei.unix@gmail.com>
Download mbox | patch
Permalink /patch/151680/
State New
Headers show

Comments

namei.unix@gmail.com - April 10, 2012, 6:10 p.m.
From: Liu Yuan <tailai.ly@taobao.com>

The 'qemu-img convert -h' advertise that the default cache mode is
'writeback', while in fact it is 'unsafe'.

This patch 1) changes the cache mode as 'writeback' and 2) explicitly
calls bdrv_flush() to flush the dirty bits.

2) is needed because some backend storage doesn't have a self-flush
mechanism(for e.g., sheepdog), so we need to call bdrv_flush() to make
sure the image is really writen to the storage instead of hanging around
writeback cache forever.

Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
---
 qemu-img.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Paolo Bonzini - April 10, 2012, 7:40 p.m.
Il 10/04/2012 20:10, Liu Yuan ha scritto:
> From: Liu Yuan <tailai.ly@taobao.com>
> 
> The 'qemu-img convert -h' advertise that the default cache mode is
> 'writeback', while in fact it is 'unsafe'.
> 
> This patch 1) changes the cache mode as 'writeback' and 2) explicitly
> calls bdrv_flush() to flush the dirty bits.
> 
> 2) is needed because some backend storage doesn't have a self-flush
> mechanism(for e.g., sheepdog), so we need to call bdrv_flush() to make
> sure the image is really writen to the storage instead of hanging around
> writeback cache forever.

Shouldn't the close method do that then?

Paolo

> Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
> ---
>  qemu-img.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 6a61ca8..f96230b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -644,7 +644,7 @@ static int img_convert(int argc, char **argv)
>  
>      fmt = NULL;
>      out_fmt = "raw";
> -    cache = "unsafe";
> +    cache = "writeback";
>      out_baseimg = NULL;
>      compress = 0;
>      for(;;) {
> @@ -1036,6 +1036,9 @@ out:
>      free_option_parameters(param);
>      qemu_vfree(buf);
>      if (out_bs) {
> +        if (ret == 0) {
> +            bdrv_flush(out_bs);
> +	}
>          bdrv_delete(out_bs);
>      }
>      if (bs) {
namei.unix@gmail.com - April 11, 2012, 2:42 a.m.
Hi Paolo,
On 04/11/2012 03:40 AM, Paolo Bonzini wrote:

>> From: Liu Yuan <tailai.ly@taobao.com>
>> > 
>> > The 'qemu-img convert -h' advertise that the default cache mode is
>> > 'writeback', while in fact it is 'unsafe'.
>> > 
>> > This patch 1) changes the cache mode as 'writeback' and 2) explicitly
>> > calls bdrv_flush() to flush the dirty bits.
>> > 
>> > 2) is needed because some backend storage doesn't have a self-flush
>> > mechanism(for e.g., sheepdog), so we need to call bdrv_flush() to make
>> > sure the image is really writen to the storage instead of hanging around
>> > writeback cache forever.
> Shouldn't the close method do that then?


I'd insist using bdrv_flush() instead of bdrv_close() because
 1) if we use bdrv_close(), we rely on the assumption that backend
storage will do flushing while interpreting this operation. This
assumption might not always hold, for e.g, current sheepdog doesn't do
flushing for bdrv_close(). So bdrv_flush() will be the safest method to
push the data back.
 2) explicit flushing is more maintainable, we don't need to guess if it
does flushing internally if we use other function that flush implicitly.

Thanks,
Yuan
Paolo Bonzini - April 11, 2012, 7:32 a.m.
Il 11/04/2012 04:42, Liu Yuan ha scritto:
>  1) if we use bdrv_close(), we rely on the assumption that backend
> storage will do flushing while interpreting this operation. This
> assumption might not always hold, for e.g, current sheepdog doesn't do
> flushing for bdrv_close(). So bdrv_flush() will be the safest method to
> push the data back.
>  2) explicit flushing is more maintainable, we don't need to guess if it
> does flushing internally if we use other function that flush implicitly.

I think it is reasonable semantics that closing gets all data to storage.

Paolo
Kevin Wolf - April 11, 2012, 7:49 a.m.
Am 10.04.2012 20:10, schrieb Liu Yuan:
> From: Liu Yuan <tailai.ly@taobao.com>
> 
> The 'qemu-img convert -h' advertise that the default cache mode is
> 'writeback', while in fact it is 'unsafe'.
> 
> This patch 1) changes the cache mode as 'writeback' and 2) explicitly
> calls bdrv_flush() to flush the dirty bits.
> 
> 2) is needed because some backend storage doesn't have a self-flush
> mechanism(for e.g., sheepdog), so we need to call bdrv_flush() to make
> sure the image is really writen to the storage instead of hanging around
> writeback cache forever.
> 
> Signed-off-by: Liu Yuan <tailai.ly@taobao.com>

I don't agree with this patch. If the documentation says that qemu-img
always uses writeback, then the documentation must be fixed.

We really don't care about flushes during an image conversion. It should
just go as fast as it can. If any error happens in the middle, you'll
have to throw the image away anyway, because it contains only half of
what you need. So not having flushed doesn't really make any difference.

As soon as a guest begins using the image with data that we really care
about, it's probably running in a VM with non-unsafe mode, and then it
gets flushed.

Kevin
namei.unix@gmail.com - April 11, 2012, 9:58 a.m.
On 04/11/2012 03:32 PM, Paolo Bonzini wrote:

> Il 11/04/2012 04:42, Liu Yuan ha scritto:
>>  1) if we use bdrv_close(), we rely on the assumption that backend
>> storage will do flushing while interpreting this operation. This
>> assumption might not always hold, for e.g, current sheepdog doesn't do
>> flushing for bdrv_close(). So bdrv_flush() will be the safest method to
>> push the data back.
>>  2) explicit flushing is more maintainable, we don't need to guess if it
>> does flushing internally if we use other function that flush implicitly.
> 
> I think it is reasonable semantics that closing gets all data to storage.
> 


Yes, but if the buggy block driver dose not assure us of this semantics,
we'll risk to lose data.

Thanks,
Yuan
namei.unix@gmail.com - April 11, 2012, 10:05 a.m.
On 04/11/2012 03:49 PM, Kevin Wolf wrote:

> I don't agree with this patch. If the documentation says that qemu-img
> always uses writeback, then the documentation must be fixed.
> 
> We really don't care about flushes during an image conversion. It should
> just go as fast as it can. If any error happens in the middle, you'll
> have to throw the image away anyway, because it contains only half of
> what you need. So not having flushed doesn't really make any difference.
> 
> As soon as a guest begins using the image with data that we really care
> about, it's probably running in a VM with non-unsafe mode, and then it
> gets flushed.


But we can set cache mode for qemu-img convert. So at least if we set
explicitly with other modes(for e.g, writeback) that ask to flush the
dirty bits, I think we should call brdv_flush().

If you agree, I'll update the patch with this modification.

Thanks,
Yuan
Kevin Wolf - April 11, 2012, 10:33 a.m.
Am 11.04.2012 12:05, schrieb Liu Yuan:
> On 04/11/2012 03:49 PM, Kevin Wolf wrote:
> 
>> I don't agree with this patch. If the documentation says that qemu-img
>> always uses writeback, then the documentation must be fixed.
>>
>> We really don't care about flushes during an image conversion. It should
>> just go as fast as it can. If any error happens in the middle, you'll
>> have to throw the image away anyway, because it contains only half of
>> what you need. So not having flushed doesn't really make any difference.
>>
>> As soon as a guest begins using the image with data that we really care
>> about, it's probably running in a VM with non-unsafe mode, and then it
>> gets flushed.
> 
> 
> But we can set cache mode for qemu-img convert. So at least if we set
> explicitly with other modes(for e.g, writeback) that ask to flush the
> dirty bits, I think we should call brdv_flush().
> 
> If you agree, I'll update the patch with this modification.

Yes, adding the flush is fine, I just wouldn't change the default cache
mode.

Kevin
Kevin Wolf - April 11, 2012, 12:27 p.m.
Am 11.04.2012 12:33, schrieb Kevin Wolf:
> Am 11.04.2012 12:05, schrieb Liu Yuan:
>> On 04/11/2012 03:49 PM, Kevin Wolf wrote:
>>
>>> I don't agree with this patch. If the documentation says that qemu-img
>>> always uses writeback, then the documentation must be fixed.
>>>
>>> We really don't care about flushes during an image conversion. It should
>>> just go as fast as it can. If any error happens in the middle, you'll
>>> have to throw the image away anyway, because it contains only half of
>>> what you need. So not having flushed doesn't really make any difference.
>>>
>>> As soon as a guest begins using the image with data that we really care
>>> about, it's probably running in a VM with non-unsafe mode, and then it
>>> gets flushed.
>>
>>
>> But we can set cache mode for qemu-img convert. So at least if we set
>> explicitly with other modes(for e.g, writeback) that ask to flush the
>> dirty bits, I think we should call brdv_flush().
>>
>> If you agree, I'll update the patch with this modification.
> 
> Yes, adding the flush is fine, I just wouldn't change the default cache
> mode.

On second thought, wouldn't make it more sense to add the flush in
bdrv_close(), so that all callers get it?

Kevin
namei.unix@gmail.com - April 11, 2012, 2:08 p.m.
On 04/11/2012 08:27 PM, Kevin Wolf wrote:

> Am 11.04.2012 12:33, schrieb Kevin Wolf:
>> Am 11.04.2012 12:05, schrieb Liu Yuan:
>>> On 04/11/2012 03:49 PM, Kevin Wolf wrote:
>>>
>>>> I don't agree with this patch. If the documentation says that qemu-img
>>>> always uses writeback, then the documentation must be fixed.
>>>>
>>>> We really don't care about flushes during an image conversion. It should
>>>> just go as fast as it can. If any error happens in the middle, you'll
>>>> have to throw the image away anyway, because it contains only half of
>>>> what you need. So not having flushed doesn't really make any difference.
>>>>
>>>> As soon as a guest begins using the image with data that we really care
>>>> about, it's probably running in a VM with non-unsafe mode, and then it
>>>> gets flushed.
>>>
>>>
>>> But we can set cache mode for qemu-img convert. So at least if we set
>>> explicitly with other modes(for e.g, writeback) that ask to flush the
>>> dirty bits, I think we should call brdv_flush().
>>>
>>> If you agree, I'll update the patch with this modification.
>>
>> Yes, adding the flush is fine, I just wouldn't change the default cache
>> mode.
> 
> On second thought, wouldn't make it more sense to add the flush in
> bdrv_close(), so that all callers get it?
> 

Hmm, yes, if high level bdrv_close() calls bdrv_flush() internally, we
can safely call bdrv_close() from users (qemu-img) to ensure data
writeback with proper cache mode.

Thanks,
Yuan
Paolo Bonzini - April 11, 2012, 2:51 p.m.
Il 11/04/2012 11:58, Liu Yuan ha scritto:
>>> >>  2) explicit flushing is more maintainable, we don't need to guess if it
>>> >> does flushing internally if we use other function that flush implicitly.
>> > 
>> > I think it is reasonable semantics that closing gets all data to storage.
>> > 
> 
> Yes, but if the buggy block driver dose not assure us of this semantics,
> we'll risk to lose data.

Sure, fix the driver then, not qemu-img.

Paolo

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 6a61ca8..f96230b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -644,7 +644,7 @@  static int img_convert(int argc, char **argv)
 
     fmt = NULL;
     out_fmt = "raw";
-    cache = "unsafe";
+    cache = "writeback";
     out_baseimg = NULL;
     compress = 0;
     for(;;) {
@@ -1036,6 +1036,9 @@  out:
     free_option_parameters(param);
     qemu_vfree(buf);
     if (out_bs) {
+        if (ret == 0) {
+            bdrv_flush(out_bs);
+	}
         bdrv_delete(out_bs);
     }
     if (bs) {