[Qemu-devel] [PULL 34/48] block: Move enable_write_cache to BB level

From: Kevin Wolf <kwolf@redhat.com>
To: qemu-block@nongnu.org
Cc: kwolf@redhat.com, qemu-devel@nongnu.org
Subject: [Qemu-devel] [PULL 34/48] block: Move enable_write_cache to BB level
Date: Tue, 29 Mar 2016 17:08:34 +0200	[thread overview]
Message-ID: <1459264128-12761-35-git-send-email-kwolf@redhat.com> (raw)
In-Reply-To: <1459264128-12761-1-git-send-email-kwolf@redhat.com>

Whether a write cache is used or not is a decision that concerns the
user (e.g. the guest device) rather than the backend. It was already
logically part of the BB level as bdrv_move_feature_fields() always kept
it on top of the BDS tree; with this patch, the core of it (the actual
flag and the additional flushes) is also implemented there.

Direct callers of bdrv_open() must pass BDRV_O_CACHE_WB now if bs
doesn't have a BlockBackend attached.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c                    | 26 +++++++++++++++++---------
 block/block-backend.c      | 42 +++++++++++++++++++++++++++---------------
 block/io.c                 |  2 +-
 block/iscsi.c              |  2 +-
 include/block/block.h      |  1 +
 include/block/block_int.h  |  3 ---
 tests/qemu-iotests/142     |  4 ++--
 tests/qemu-iotests/142.out |  8 ++++----
 8 files changed, 53 insertions(+), 35 deletions(-)

diff --git a/block.c b/block.c
index 33dc46c..57f1140 100644
--- a/block.c
+++ b/block.c
@@ -2037,6 +2037,11 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
             goto error;
         }
     }
+    if (!reopen_state->bs->blk && !(reopen_state->flags & BDRV_O_CACHE_WB)) {
+        error_setg(errp, "Cannot disable cache.writeback: No BlockBackend");
+        ret = -EINVAL;
+        goto error;
+    }
 
     /* node-name and driver must be unchanged. Put them back into the QDict, so
      * that they are checked at the end of this function. */
@@ -2137,10 +2142,10 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 
     reopen_state->bs->explicit_options   = reopen_state->explicit_options;
     reopen_state->bs->open_flags         = reopen_state->flags;
-    reopen_state->bs->enable_write_cache = !!(reopen_state->flags &
-                                              BDRV_O_CACHE_WB);
     reopen_state->bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
 
+    bdrv_set_enable_write_cache(reopen_state->bs,
+                                !!(reopen_state->flags & BDRV_O_CACHE_WB));
     bdrv_refresh_limits(reopen_state->bs, NULL);
 }
 
@@ -2270,9 +2275,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
                                      BlockDriverState *bs_src)
 {
     /* move some fields that need to stay attached to the device */
-
-    /* dev info */
-    bs_dest->enable_write_cache = bs_src->enable_write_cache;
 }
 
 static void change_parent_backing_link(BlockDriverState *from,
@@ -2752,12 +2754,18 @@ int bdrv_is_sg(BlockDriverState *bs)
 
 int bdrv_enable_write_cache(BlockDriverState *bs)
 {
-    return bs->enable_write_cache;
+    if (bs->blk) {
+        return blk_enable_write_cache(bs->blk);
+    } else {
+        return true;
+    }
 }
 
 void bdrv_set_enable_write_cache(BlockDriverState *bs, bool wce)
 {
-    bs->enable_write_cache = wce;
+    if (bs->blk) {
+        blk_set_enable_write_cache(bs->blk, wce);
+    }
 
     /* so a reopen() will preserve wce */
     if (wce) {
@@ -3617,8 +3625,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
             }
 
             /* backing files always opened read-only */
-            back_flags =
-                flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
+            back_flags = flags | BDRV_O_CACHE_WB;
+            back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
 
             if (backing_fmt) {
                 backing_options = qdict_new();
diff --git a/block/block-backend.c b/block/block-backend.c
index 29ef24e..23f2c27 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -46,6 +46,8 @@ struct BlockBackend {
      * can be used to restore those options in the new BDS on insert) */
     BlockBackendRootState root_state;
 
+    bool enable_write_cache;
+
     /* I/O stats (display with "info blockstats"). */
     BlockAcctStats stats;
 
@@ -698,11 +700,17 @@ static int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
                                       unsigned int bytes, QEMUIOVector *qiov,
                                       BdrvRequestFlags flags)
 {
-    int ret = blk_check_byte_request(blk, offset, bytes);
+    int ret;
+
+    ret = blk_check_byte_request(blk, offset, bytes);
     if (ret < 0) {
         return ret;
     }
 
+    if (!blk->enable_write_cache) {
+        flags |= BDRV_REQ_FUA;
+    }
+
     return bdrv_co_do_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
 }
 
@@ -1209,26 +1217,19 @@ int blk_is_sg(BlockBackend *blk)
 
 int blk_enable_write_cache(BlockBackend *blk)
 {
-    BlockDriverState *bs = blk_bs(blk);
-
-    if (bs) {
-        return bdrv_enable_write_cache(bs);
-    } else {
-        return !!(blk->root_state.open_flags & BDRV_O_CACHE_WB);
-    }
+    return blk->enable_write_cache;
 }
 
 void blk_set_enable_write_cache(BlockBackend *blk, bool wce)
 {
-    BlockDriverState *bs = blk_bs(blk);
+    blk->enable_write_cache = wce;
 
-    if (bs) {
-        bdrv_set_enable_write_cache(bs, wce);
-    } else {
+    /* TODO Remove this when BDRV_O_CACHE_WB isn't used any more */
+    if (blk->root) {
         if (wce) {
-            blk->root_state.open_flags |= BDRV_O_CACHE_WB;
+            blk->root->bs->open_flags |= BDRV_O_CACHE_WB;
         } else {
-            blk->root_state.open_flags &= ~BDRV_O_CACHE_WB;
+            blk->root->bs->open_flags &= ~BDRV_O_CACHE_WB;
         }
     }
 }
@@ -1491,11 +1492,22 @@ int blk_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors)
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
                      int64_t pos, int size)
 {
+    int ret;
+
     if (!blk_is_available(blk)) {
         return -ENOMEDIUM;
     }
 
-    return bdrv_save_vmstate(blk_bs(blk), buf, pos, size);
+    ret = bdrv_save_vmstate(blk_bs(blk), buf, pos, size);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (ret == size && !blk->enable_write_cache) {
+        ret = bdrv_flush(blk_bs(blk));
+    }
+
+    return ret < 0 ? ret : size;
 }
 
 int blk_load_vmstate(BlockBackend *blk, uint8_t *buf, int64_t pos, int size)
diff --git a/block/io.c b/block/io.c
index 9663db6..4c9e3f4 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1158,7 +1158,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
     }
     bdrv_debug_event(bs, BLKDBG_PWRITEV_DONE);
 
-    if (ret == 0 && !bs->enable_write_cache) {
+    if (ret == 0 && (flags & BDRV_REQ_FUA)) {
         ret = bdrv_co_flush(bs);
     }
 
diff --git a/block/iscsi.c b/block/iscsi.c
index 128ea79..3b54536 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -476,7 +476,7 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs,
     num_sectors = sector_qemu2lun(nb_sectors, iscsilun);
     iscsi_co_init_iscsitask(iscsilun, &iTask);
 retry:
-    fua = iscsilun->dpofua && !bs->enable_write_cache;
+    fua = iscsilun->dpofua && !bdrv_enable_write_cache(bs);
     iTask.force_next_flush = !fua;
     if (iscsilun->use_16_for_rw) {
         iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
diff --git a/include/block/block.h b/include/block/block.h
index 8d9bf2f..c645c26 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -64,6 +64,7 @@ typedef enum {
      */
     BDRV_REQ_MAY_UNMAP          = 0x4,
     BDRV_REQ_NO_SERIALISING     = 0x8,
+    BDRV_REQ_FUA                = 0x10,
 } BdrvRequestFlags;
 
 typedef struct BlockSizes {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1177c25..4884609 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -442,9 +442,6 @@ struct BlockDriverState {
     /* Alignment requirement for offset/length of I/O requests */
     unsigned int request_alignment;
 
-    /* do we need to tell the quest if we have a volatile write cache? */
-    int enable_write_cache;
-
     /* the following member gives a name to every node on the bs graph. */
     char node_name[32];
     /* element of the list of named nodes building the graph */
diff --git a/tests/qemu-iotests/142 b/tests/qemu-iotests/142
index 80834b5..517fb30 100755
--- a/tests/qemu-iotests/142
+++ b/tests/qemu-iotests/142
@@ -338,8 +338,8 @@ echo
 # TODO Implement node-name support for 'qemu-io' HMP command for -c
 # Can use only -o to access child node options for now
 
-hmp_cmds="qemu-io none0 \"reopen -o file.cache.writeback=off,file.cache.direct=off,file.cache.no-flush=off\"
-qemu-io none0 \"reopen -o backing.file.cache.writeback=on,backing.file.cache.direct=off,backing.file.cache.no-flush=on\"
+hmp_cmds="qemu-io none0 \"reopen -o file.cache.direct=off,file.cache.no-flush=off\"
+qemu-io none0 \"reopen -o backing.file.cache.direct=off,backing.file.cache.no-flush=on\"
 qemu-io none0 \"reopen -c none\"
 info block image
 info block file
diff --git a/tests/qemu-iotests/142.out b/tests/qemu-iotests/142.out
index 5dd5bd0..32dc802 100644
--- a/tests/qemu-iotests/142.out
+++ b/tests/qemu-iotests/142.out
@@ -132,7 +132,7 @@ cache.direct=on on backing-file
 
 cache.writeback=off on none0
     Cache mode:       writethrough
-    Cache mode:       writethrough
+    Cache mode:       writeback
     Cache mode:       writeback
     Cache mode:       writeback
     Cache mode:       writeback
@@ -342,7 +342,7 @@ cache.direct=on on backing-file
 
 cache.writeback=off on none0
     Cache mode:       writeback, direct
-    Cache mode:       writethrough
+    Cache mode:       writeback
     Cache mode:       writeback
     Cache mode:       writeback
     Cache mode:       writeback
@@ -503,7 +503,7 @@ cache.direct=on on backing-file
 
 cache.writeback=off on blk
     Cache mode:       writethrough
-    Cache mode:       writethrough
+    Cache mode:       writeback
     Cache mode:       writeback
     Cache mode:       writeback
     Cache mode:       writeback
@@ -707,7 +707,7 @@ cache.no-flush=on on backing-file
 --- Change cache mode after reopening child ---
 
     Cache mode:       writeback, direct
-    Cache mode:       writethrough
+    Cache mode:       writeback
     Cache mode:       writeback, direct
     Cache mode:       writeback, ignore flushes
 *** done
-- 
1.8.3.1
