Re: [Qemu-devel] [PATCH] qemu-img: let qemu-img behave as the manual advertise
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
Re: [Qemu-devel] [PATCH] qemu-img: let qemu-img behave as the manual advertise
Am 10.04.2012 20:10, schrieb Liu Yuan: From: Liu Yuan tailai...@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...@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
Re: [Qemu-devel] [PATCH] qemu-img: let qemu-img behave as the manual advertise
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
Re: [Qemu-devel] [PATCH] qemu-img: let qemu-img behave as the manual advertise
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
Re: [Qemu-devel] [PATCH] qemu-img: let qemu-img behave as the manual advertise
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
Re: [Qemu-devel] [PATCH] qemu-img: let qemu-img behave as the manual advertise
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
Re: [Qemu-devel] [PATCH] qemu-img: let qemu-img behave as the manual advertise
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
Re: [Qemu-devel] [PATCH] qemu-img: let qemu-img behave as the manual advertise
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
Re: [Qemu-devel] [PATCH] qemu-img: let qemu-img behave as the manual advertise
Il 10/04/2012 20:10, Liu Yuan ha scritto:
From: Liu Yuan tailai...@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...@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) {
Re: [Qemu-devel] [PATCH] qemu-img: let qemu-img behave as the manual advertise
Hi Paolo, On 04/11/2012 03:40 AM, Paolo Bonzini wrote: From: Liu Yuan tailai...@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
