| Message ID | 1334081438-10404-1-git-send-email-namei.unix@gmail.com |
|---|---|
| State | New |
| Headers | show |
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) {
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
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
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
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
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
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
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
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
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
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) {