Re: [Qemu-devel] [PATCH 0/2] block: Write out internal caches even with cache=unsafe

[Paolo Bonzini]

On 10/24/2011 11:53 AM, Kevin Wolf wrote:
>> >  
>> >  I'm talking about the internal driver API only.  The external API is
>> >  fine as is.
> Ok, so external callers don't force us to do it.
> 
> Yes, we could split bdrv_flush internally into two functions for "flush
> one level to the OS" and "flush all the way down to the disk", but I'm
> not sure if this really buys us anything or just adds complexity.

I would say that:

1) the "safe" NBD and iSCSI drivers are not in-tree, but you would have to
convert those too.  iSCSI uses aio, so it would be non-trivial.

2) long-term you wanted to convert raw-posix to coroutines, but in the
meanwhile your patch introduces yet another partial transition;

3) your two patches are more complex compared to something like this:

diff --git a/block.c b/block.c
index 11c7f91..1e06a8a 100644
--- a/block.c
+++ b/block.c
@@ -2908,9 +2908,19 @@ static void coroutine_fn bdrv_flush_co_entry(void 
*opaque)
 
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 {
-    if (bs->open_flags & BDRV_O_NO_FLUSH) {
+    if (!bs->drv) {
         return 0;
-    } else if (!bs->drv) {
+    }
+
+    /* First send everything to the OS.  */
+    if (bs->drv->bdrv_co_flush_metadata) {
+        int ret = bs->drv->bdrv_co_flush_metadata(bs);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    /* Now flush the host cache to disk if we need to.  */
+    if (bs->open_flags & BDRV_O_NO_FLUSH) {
         return 0;
     } else if (bs->drv->bdrv_co_flush) {
         return bs->drv->bdrv_co_flush(bs);
diff --git a/block/qcow2.c b/block/qcow2.c
index a181932..cd13452 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1105,7 +1105,7 @@ fail:
     return ret;
 }
 
-static int qcow2_co_flush(BlockDriverState *bs)
+static int qcow2_co_flush_metadata(BlockDriverState *bs)
 {
     BDRVQcowState *s = bs->opaque;
     int ret;
@@ -1121,7 +1121,11 @@ static int qcow2_co_flush(BlockDriverState *bs)
         return ret;
     }
     qemu_co_mutex_unlock(&s->lock);
+    return 0;
+}
 
+static int qcow2_co_flush(BlockDriverState *bs)
+{
     return bdrv_co_flush(bs->file);
 }
 
@@ -1244,6 +1248,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_co_readv      = qcow2_co_readv,
     .bdrv_co_writev     = qcow2_co_writev,
     .bdrv_co_flush      = qcow2_co_flush,
+    .bdrv_co_flush_metadata = qcow2_co_flush_metadata,
 
     .bdrv_co_discard        = qcow2_co_discard,
     .bdrv_truncate          = qcow2_truncate,
diff --git a/block_int.h b/block_int.h
index dac00f5..ab4ceea 100644
--- a/block_int.h
+++ b/block_int.h
@@ -84,6 +84,7 @@ struct BlockDriver {
     int coroutine_fn (*bdrv_co_writev)(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
     int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
+    int coroutine_fn (*bdrv_co_flush_metadata)(BlockDriverState *bs);
     int coroutine_fn (*bdrv_co_discard)(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors);
 
Paolo