Patchwork block: push recursive flushing up from drivers

login
register
mail settings
Submitter Paolo Bonzini
Date March 12, 2012, 4:01 p.m.
Message ID <1331568108-13188-1-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/146142/
State New
Headers show

Comments

Paolo Bonzini - March 12, 2012, 4:01 p.m.
In most cases, bdrv_co_flush_to_disk just needs to flush the underlying
file for protocols.  Do this implicitly in the block layer.

The backing file is also flushed, because it may still be open read-write
in the case of live snapshots.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c          |    9 +++++----
 block/blkdebug.c |    7 -------
 block/cow.c      |    6 ------
 block/qcow.c     |    6 ------
 block/qcow2.c    |    6 ------
 block/qed.c      |    8 --------
 block/raw.c      |    6 ------
 block/vdi.c      |    8 --------
 block/vmdk.c     |    1 -
 block/vpc.c      |    6 ------
 10 files changed, 5 insertions(+), 58 deletions(-)
Paolo Bonzini - March 12, 2012, 4:22 p.m.
Il 12/03/2012 17:22, Kevin Wolf ha scritto:
>> > In most cases, bdrv_co_flush_to_disk just needs to flush the underlying
>> > file for protocols.  Do this implicitly in the block layer.
>> > 
>> > The backing file is also flushed, because it may still be open read-write
>> > in the case of live snapshots.
> Is this an independent change? I'm also not convinced that it's the
> right thing to do because even though it is still opened read-write, we
> don't write to it any more. Once bdrv_reopen() is ready, we'll want to
> change it to read-only after taking the snapshot.

Ok, I'll remove it.

Paolo
Kevin Wolf - March 12, 2012, 4:22 p.m.
Am 12.03.2012 17:01, schrieb Paolo Bonzini:
> In most cases, bdrv_co_flush_to_disk just needs to flush the underlying
> file for protocols.  Do this implicitly in the block layer.
> 
> The backing file is also flushed, because it may still be open read-write
> in the case of live snapshots.

Is this an independent change? I'm also not convinced that it's the
right thing to do because even though it is still opened read-write, we
don't write to it any more. Once bdrv_reopen() is ready, we'll want to
change it to read-only after taking the snapshot.

> @@ -3516,10 +3514,13 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>  {
>      int ret;
>  
> -    if (!bs->drv) {
> +    if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
>          return 0;
>      }
>  
> +    bdrv_co_flush(bs->file);
> +    bdrv_co_flush(bs->backing_hd);

Error handling is missing here.

> +
>      /* Write back cached data to the OS even with cache=unsafe */
>      if (bs->drv->bdrv_co_flush_to_os) {
>          ret = bs->drv->bdrv_co_flush_to_os(bs);

Now you first flush bs->file and then write out the internal caches.
This doesn't look quite right. Probably the recursion should be at the
very end of the function.

Kevin

Patch

diff --git a/block.c b/block.c
index 52ffe14..dac62a1 100644
--- a/block.c
+++ b/block.c
@@ -2327,9 +2327,7 @@  void bdrv_flush_all(void)
     BlockDriverState *bs;
 
     QTAILQ_FOREACH(bs, &bdrv_states, list) {
-        if (!bdrv_is_read_only(bs) && bdrv_is_inserted(bs)) {
-            bdrv_flush(bs);
-        }
+        bdrv_flush(bs);
     }
 }
 
@@ -3516,10 +3514,13 @@  int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 {
     int ret;
 
-    if (!bs->drv) {
+    if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
         return 0;
     }
 
+    bdrv_co_flush(bs->file);
+    bdrv_co_flush(bs->backing_hd);
+
     /* Write back cached data to the OS even with cache=unsafe */
     if (bs->drv->bdrv_co_flush_to_os) {
         ret = bs->drv->bdrv_co_flush_to_os(bs);
diff --git a/block/blkdebug.c b/block/blkdebug.c
index a251802..e56e37d 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -397,12 +397,6 @@  static void blkdebug_close(BlockDriverState *bs)
     }
 }
 
-static BlockDriverAIOCB *blkdebug_aio_flush(BlockDriverState *bs,
-    BlockDriverCompletionFunc *cb, void *opaque)
-{
-    return bdrv_aio_flush(bs->file, cb, opaque);
-}
-
 static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
     BlkdebugVars *old_vars)
 {
@@ -452,7 +446,6 @@  static BlockDriver bdrv_blkdebug = {
 
     .bdrv_aio_readv     = blkdebug_aio_readv,
     .bdrv_aio_writev    = blkdebug_aio_writev,
-    .bdrv_aio_flush     = blkdebug_aio_flush,
 
     .bdrv_debug_event   = blkdebug_debug_event,
 };
diff --git a/block/cow.c b/block/cow.c
index bb5927c..8d3c9f8 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -318,11 +318,6 @@  exit:
     return ret;
 }
 
-static coroutine_fn int cow_co_flush(BlockDriverState *bs)
-{
-    return bdrv_co_flush(bs->file);
-}
-
 static QEMUOptionParameter cow_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -348,7 +343,6 @@  static BlockDriver bdrv_cow = {
 
     .bdrv_read              = cow_co_read,
     .bdrv_write             = cow_co_write,
-    .bdrv_co_flush_to_disk  = cow_co_flush,
     .bdrv_co_is_allocated   = cow_co_is_allocated,
 
     .create_options = cow_create_options,
diff --git a/block/qcow.c b/block/qcow.c
index b1cfe1f..35dff49 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -835,11 +835,6 @@  fail:
     return ret;
 }
 
-static coroutine_fn int qcow_co_flush(BlockDriverState *bs)
-{
-    return bdrv_co_flush(bs->file);
-}
-
 static int qcow_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BDRVQcowState *s = bs->opaque;
@@ -877,7 +872,6 @@  static BlockDriver bdrv_qcow = {
 
     .bdrv_co_readv          = qcow_co_readv,
     .bdrv_co_writev         = qcow_co_writev,
-    .bdrv_co_flush_to_disk  = qcow_co_flush,
     .bdrv_co_is_allocated   = qcow_co_is_allocated,
 
     .bdrv_set_key           = qcow_set_key,
diff --git a/block/qcow2.c b/block/qcow2.c
index eb5ea48..d01baa6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1241,11 +1241,6 @@  static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs)
     return 0;
 }
 
-static coroutine_fn int qcow2_co_flush_to_disk(BlockDriverState *bs)
-{
-    return bdrv_co_flush(bs->file);
-}
-
 static int64_t qcow2_vm_state_offset(BDRVQcowState *s)
 {
 	return (int64_t)s->l1_vm_state_index << (s->cluster_bits + s->l2_bits);
@@ -1365,7 +1360,6 @@  static BlockDriver bdrv_qcow2 = {
     .bdrv_co_readv          = qcow2_co_readv,
     .bdrv_co_writev         = qcow2_co_writev,
     .bdrv_co_flush_to_os    = qcow2_co_flush_to_os,
-    .bdrv_co_flush_to_disk  = qcow2_co_flush_to_disk,
 
     .bdrv_co_discard        = qcow2_co_discard,
     .bdrv_truncate          = qcow2_truncate,
diff --git a/block/qed.c b/block/qed.c
index a041d31..d6c1580 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1350,13 +1350,6 @@  static BlockDriverAIOCB *bdrv_qed_aio_writev(BlockDriverState *bs,
                          opaque, QED_AIOCB_WRITE);
 }
 
-static BlockDriverAIOCB *bdrv_qed_aio_flush(BlockDriverState *bs,
-                                            BlockDriverCompletionFunc *cb,
-                                            void *opaque)
-{
-    return bdrv_aio_flush(bs->file, cb, opaque);
-}
-
 typedef struct {
     Coroutine *co;
     int ret;
@@ -1562,7 +1555,6 @@  static BlockDriver bdrv_qed = {
     .bdrv_make_empty          = bdrv_qed_make_empty,
     .bdrv_aio_readv           = bdrv_qed_aio_readv,
     .bdrv_aio_writev          = bdrv_qed_aio_writev,
-    .bdrv_aio_flush           = bdrv_qed_aio_flush,
     .bdrv_co_write_zeroes     = bdrv_qed_co_write_zeroes,
     .bdrv_truncate            = bdrv_qed_truncate,
     .bdrv_getlength           = bdrv_qed_getlength,
diff --git a/block/raw.c b/block/raw.c
index 1cdac0c..7086e31 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -25,11 +25,6 @@  static void raw_close(BlockDriverState *bs)
 {
 }
 
-static int coroutine_fn raw_co_flush(BlockDriverState *bs)
-{
-    return bdrv_co_flush(bs->file);
-}
-
 static int64_t raw_getlength(BlockDriverState *bs)
 {
     return bdrv_getlength(bs->file);
@@ -113,7 +108,6 @@  static BlockDriver bdrv_raw = {
 
     .bdrv_co_readv          = raw_co_readv,
     .bdrv_co_writev         = raw_co_writev,
-    .bdrv_co_flush_to_disk  = raw_co_flush,
     .bdrv_co_discard        = raw_co_discard,
 
     .bdrv_probe         = raw_probe,
diff --git a/block/vdi.c b/block/vdi.c
index 6a0011f..f8fa865 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -930,13 +930,6 @@  static void vdi_close(BlockDriverState *bs)
     error_free(s->migration_blocker);
 }
 
-static coroutine_fn int vdi_co_flush(BlockDriverState *bs)
-{
-    logout("\n");
-    return bdrv_co_flush(bs->file);
-}
-
-
 static QEMUOptionParameter vdi_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -969,7 +962,6 @@  static BlockDriver bdrv_vdi = {
     .bdrv_open = vdi_open,
     .bdrv_close = vdi_close,
     .bdrv_create = vdi_create,
-    .bdrv_co_flush_to_disk = vdi_co_flush,
     .bdrv_co_is_allocated = vdi_co_is_allocated,
     .bdrv_make_empty = vdi_make_empty,
 
diff --git a/block/vmdk.c b/block/vmdk.c
index 45c003a..c8038cc 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1528,7 +1528,6 @@  static coroutine_fn int vmdk_co_flush(BlockDriverState *bs)
     int i, ret, err;
     BDRVVmdkState *s = bs->opaque;
 
-    ret = bdrv_co_flush(bs->file);
     for (i = 0; i < s->num_extents; i++) {
         err = bdrv_co_flush(s->extents[i].file);
         if (err < 0) {
diff --git a/block/vpc.c b/block/vpc.c
index 6b4816f..706faf3 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -507,11 +507,6 @@  static coroutine_fn int vpc_co_write(BlockDriverState *bs, int64_t sector_num,
     return ret;
 }
 
-static coroutine_fn int vpc_co_flush(BlockDriverState *bs)
-{
-    return bdrv_co_flush(bs->file);
-}
-
 /*
  * Calculates the number of cylinders, heads and sectors per cylinder
  * based on a given number of sectors. This is the algorithm described
@@ -789,7 +784,6 @@  static BlockDriver bdrv_vpc = {
 
     .bdrv_read              = vpc_co_read,
     .bdrv_write             = vpc_co_write,
-    .bdrv_co_flush_to_disk  = vpc_co_flush,
 
     .create_options = vpc_create_options,
 };