[Qemu-devel] [PATCH] qcow2: make cache=unsafe usable

[Qemu-devel] [PATCH] qcow2: make cache=unsafe usable

[Avi Kivity]

Currently cache=unsafe is unsafe to the point of unusability - the
caches are never written to disk except on exit so anything except
an orderly exit -- including live migration -- leaves the disk image
corrupted.

Fix by interpreting flush requests and doing everything except flushing
the underlying file.  The contents of the metadata cache are transferred
to the host pagecache, so that qemu aborts keep the disk in a consistent
state, and live migration (on the same host, or if using a coherent
filesystem) works.

Signed-off-by: Avi Kivity <avi@redhat.com>
---

Untested - is this the right approach?

 block/qcow2.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index bfff6cd..7ecd096 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -275,6 +275,13 @@ static int qcow2_open(BlockDriverState *bs, int flags)
         ret = -EINVAL;
         goto fail;
     }
+    /*
+     * Request flush callbask so that we can write metadata to the host
+     * pagecache.  Flushes to bs->file will still be ignored.  This keeps
+     * metadata consistent in host pagecache, so we're safe wrt unexpected
+     * exits, but avoids slow disk flushes (and is vulnerable to host crashes)
+     */
+    bs->open_flags &= ~BDRV_O_NO_FLUSH;
 
     /* Initialise locks */
     qemu_co_mutex_init(&s->lock);
-- 
1.7.6.1

Re: [Qemu-devel] [PATCH] qcow2: make cache=unsafe usable

[Alexander Graf]

On 07.09.2011, at 11:24, Avi Kivity wrote:

> Currently cache=unsafe is unsafe to the point of unusability - the
> caches are never written to disk except on exit so anything except
> an orderly exit -- including live migration -- leaves the disk image
> corrupted.
> 
> Fix by interpreting flush requests and doing everything except flushing
> the underlying file.  The contents of the metadata cache are transferred
> to the host pagecache, so that qemu aborts keep the disk in a consistent
> state, and live migration (on the same host, or if using a coherent
> filesystem) works.

Yes, I've seen breakage with cache=unsafe and qcow2 myself. Thus semantically, the patch seems very reasonable to me. However, I'll leave it to Kevin to decide if it's a good idea to just unset random flags in open() or if we want to have something more expressive there :)


Alex

Re: [Qemu-devel] [PATCH] qcow2: make cache=unsafe usable

[Kevin Wolf]

Am 07.09.2011 11:24, schrieb Avi Kivity:
> Currently cache=unsafe is unsafe to the point of unusability - the
> caches are never written to disk except on exit so anything except
> an orderly exit -- including live migration -- leaves the disk image
> corrupted.
> 
> Fix by interpreting flush requests and doing everything except flushing
> the underlying file.  The contents of the metadata cache are transferred
> to the host pagecache, so that qemu aborts keep the disk in a consistent
> state, and live migration (on the same host, or if using a coherent
> filesystem) works.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
> 
> Untested - is this the right approach?

Hm, could work, even though I don't like it very much. The alternative
approach would be something like this (not only untested, but won't even
compile):

diff --git a/block.c b/block.c
index a8c789a..1aa5967 100644
--- a/block.c
+++ b/block.c
@@ -1723,10 +1723,6 @@ const char *bdrv_get_device_name(BlockDriverState
*bs)

 int bdrv_flush(BlockDriverState *bs)
 {
-    if (bs->open_flags & BDRV_O_NO_FLUSH) {
-        return 0;
-    }
-
     if (bs->drv && bdrv_has_async_flush(bs->drv) && qemu_in_coroutine()) {
         return bdrv_co_flush_em(bs);
     }
@@ -2624,10 +2620,6 @@ BlockDriverAIOCB *bdrv_aio_flush(BlockDriverState
*bs,

     trace_bdrv_aio_flush(bs, opaque);

-    if (bs->open_flags & BDRV_O_NO_FLUSH) {
-        return bdrv_aio_noop_em(bs, cb, opaque);
-    }
-
     if (!drv)
         return NULL;
     return drv->bdrv_aio_flush(bs, cb, opaque);
diff --git a/block/raw-posix.c b/block/raw-posix.c
index bcf50b2..bb0c0c5 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -629,6 +629,10 @@ static BlockDriverAIOCB
*raw_aio_flush(BlockDriverState *bs,
 {
     BDRVRawState *s = bs->opaque;

+    if (bs->open_flags & BDRV_O_NO_FLUSH) {
+        return bdrv_aio_noop_em(bs, cb, opaque);
+    }
+
     if (fd_open(bs) < 0)
         return NULL;

@@ -839,6 +843,11 @@ static int raw_create(const char *filename,
QEMUOptionParameter *options)
 static int raw_flush(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
+
+    if (bs->open_flags & BDRV_O_NO_FLUSH) {
+        return 0;
+    }
+
     return qemu_fdatasync(s->fd);
 }

Re: [Qemu-devel] [PATCH] qcow2: make cache=unsafe usable

[Avi Kivity]

On 09/07/2011 12:56 PM, Kevin Wolf wrote:
> Am 07.09.2011 11:24, schrieb Avi Kivity:
> >  Currently cache=unsafe is unsafe to the point of unusability - the
> >  caches are never written to disk except on exit so anything except
> >  an orderly exit -- including live migration -- leaves the disk image
> >  corrupted.
> >
> >  Fix by interpreting flush requests and doing everything except flushing
> >  the underlying file.  The contents of the metadata cache are transferred
> >  to the host pagecache, so that qemu aborts keep the disk in a consistent
> >  state, and live migration (on the same host, or if using a coherent
> >  filesystem) works.
> >
> >  Signed-off-by: Avi Kivity<avi@redhat.com>
> >  ---
> >
> >  Untested - is this the right approach?
>
> Hm, could work, even though I don't like it very much. The alternative
> approach would be something like this

I think that your version is better - it fixes all the layered format 
drivers at once (even though qcow2 is the only one that needs fixing).

-- 
error compiling committee.c: too many arguments to function