[Qemu-devel] [PATCH v2 1/2] block: set cache mode flags in a single place

[Qemu-devel] [PATCH v2 1/2] block: set cache mode flags in a single place

[Stefan Hajnoczi]

This patch introduces bdrv_set_cache_flags() which sets open flags given
a cache mode.  Previously this was duplicated in blockdev.c and
qemu-img.c.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c    |   25 +++++++++++++++++++++++++
 block.h    |    1 +
 blockdev.c |   15 +++------------
 qemu-img.c |   27 +++------------------------
 4 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/block.c b/block.c
index 9549b9e..4aba434 100644
--- a/block.c
+++ b/block.c
@@ -412,6 +412,31 @@ static int refresh_total_sectors(BlockDriverState *bs, int64_t hint)
     return 0;
 }
 
+/**
+ * Set open flags for a given cache mode
+ *
+ * Return 0 on success, -1 if the cache mode was invalid.
+ */
+int bdrv_set_cache_flags(const char *mode, int *flags)
+{
+    *flags &= ~BDRV_O_CACHE_MASK;
+
+    if (!strcmp(mode, "off") || !strcmp(mode, "none")) {
+        *flags |= BDRV_O_NOCACHE | BDRV_O_CACHE_WB;
+    } else if (!strcmp(mode, "writeback")) {
+        *flags |= BDRV_O_CACHE_WB;
+    } else if (!strcmp(mode, "unsafe")) {
+        *flags |= BDRV_O_CACHE_WB;
+        *flags |= BDRV_O_NO_FLUSH;
+    } else if (!strcmp(mode, "writethrough")) {
+        /* this is the default */
+    } else {
+        return -1;
+    }
+
+    return 0;
+}
+
 /*
  * Common part for opening disk images and files
  */
diff --git a/block.h b/block.h
index 59cc410..8303811 100644
--- a/block.h
+++ b/block.h
@@ -68,6 +68,7 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
 BlockDriverState *bdrv_new(const char *device_name);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_delete(BlockDriverState *bs);
+int bdrv_set_cache_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
 int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
               BlockDriver *drv);
diff --git a/blockdev.c b/blockdev.c
index 0b8d3a4..5aae440 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -321,18 +321,9 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     }
 
     if ((buf = qemu_opt_get(opts, "cache")) != NULL) {
-        if (!strcmp(buf, "off") || !strcmp(buf, "none")) {
-            bdrv_flags |= BDRV_O_NOCACHE | BDRV_O_CACHE_WB;
-        } else if (!strcmp(buf, "writeback")) {
-            bdrv_flags |= BDRV_O_CACHE_WB;
-        } else if (!strcmp(buf, "unsafe")) {
-            bdrv_flags |= BDRV_O_CACHE_WB;
-            bdrv_flags |= BDRV_O_NO_FLUSH;
-        } else if (!strcmp(buf, "writethrough")) {
-            /* this is the default */
-        } else {
-           error_report("invalid cache option");
-           return NULL;
+        if (bdrv_set_cache_flags(buf, &bdrv_flags) != 0) {
+            error_report("invalid cache option");
+            return NULL;
         }
     }
 
diff --git a/qemu-img.c b/qemu-img.c
index b205e98..f39c8c5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -183,27 +183,6 @@ static int read_password(char *buf, int buf_size)
 }
 #endif
 
-static int set_cache_flag(const char *mode, int *flags)
-{
-    *flags &= ~BDRV_O_CACHE_MASK;
-
-    if (!strcmp(mode, "none") || !strcmp(mode, "off")) {
-        *flags |= BDRV_O_CACHE_WB;
-        *flags |= BDRV_O_NOCACHE;
-    } else if (!strcmp(mode, "writeback")) {
-        *flags |= BDRV_O_CACHE_WB;
-    } else if (!strcmp(mode, "unsafe")) {
-        *flags |= BDRV_O_CACHE_WB;
-        *flags |= BDRV_O_NO_FLUSH;
-    } else if (!strcmp(mode, "writethrough")) {
-        /* this is the default */
-    } else {
-        return -1;
-    }
-
-    return 0;
-}
-
 static int print_block_option_help(const char *filename, const char *fmt)
 {
     BlockDriver *drv, *proto_drv;
@@ -495,7 +474,7 @@ static int img_commit(int argc, char **argv)
     filename = argv[optind++];
 
     flags = BDRV_O_RDWR;
-    ret = set_cache_flag(cache, &flags);
+    ret = bdrv_set_cache_flags(cache, &flags);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
         return -1;
@@ -819,7 +798,7 @@ static int img_convert(int argc, char **argv)
     }
 
     flags = BDRV_O_RDWR;
-    ret = set_cache_flag(cache, &flags);
+    ret = bdrv_set_cache_flags(cache, &flags);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
         return -1;
@@ -1291,7 +1270,7 @@ static int img_rebase(int argc, char **argv)
     qemu_progress_print(0, 100);
 
     flags = BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0);
-    ret = set_cache_flag(cache, &flags);
+    ret = bdrv_set_cache_flags(cache, &flags);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
         return -1;
-- 
1.7.5.4

[Qemu-devel] [PATCH v2 2/2] block: add cache=directsync parameter to -drive

[Stefan Hajnoczi]

This patch adds -drive cache=directsync for O_DIRECT | O_SYNC host file
I/O with no disk write cache presented to the guest.

This mode is useful when guests may not be sending flushes when
appropriate and therefore leave data at risk in case of power failure.
When cache=directsync is used, write operations are only completed to
the guest when data is safely on disk.

This new mode is like cache=writethrough but it bypasses the host page
cache.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c         |    6 ++++--
 qemu-config.c   |    3 ++-
 qemu-img.c      |    3 ++-
 qemu-options.hx |    8 ++++++--
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 4aba434..bce6b9d 100644
--- a/block.c
+++ b/block.c
@@ -423,6 +423,8 @@ int bdrv_set_cache_flags(const char *mode, int *flags)
 
     if (!strcmp(mode, "off") || !strcmp(mode, "none")) {
         *flags |= BDRV_O_NOCACHE | BDRV_O_CACHE_WB;
+    } else if (!strcmp(mode, "directsync")) {
+        *flags |= BDRV_O_NOCACHE;
     } else if (!strcmp(mode, "writeback")) {
         *flags |= BDRV_O_CACHE_WB;
     } else if (!strcmp(mode, "unsafe")) {
@@ -1125,8 +1127,8 @@ int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
         return ret;
     }
 
-    /* No flush needed for cache=writethrough, it uses O_DSYNC */
-    if ((bs->open_flags & BDRV_O_CACHE_MASK) != 0) {
+    /* No flush needed for cache modes that use O_DSYNC */
+    if ((bs->open_flags & BDRV_O_CACHE_WB) != 0) {
         bdrv_flush(bs);
     }
 
diff --git a/qemu-config.c b/qemu-config.c
index 1eb6b9a..139e077 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -55,7 +55,8 @@ static QemuOptsList qemu_drive_opts = {
         },{
             .name = "cache",
             .type = QEMU_OPT_STRING,
-            .help = "host cache usage (none, writeback, writethrough, unsafe)",
+            .help = "host cache usage (none, writeback, writethrough, "
+                    "directsync, unsafe)",
         },{
             .name = "aio",
             .type = QEMU_OPT_STRING,
diff --git a/qemu-img.c b/qemu-img.c
index f39c8c5..068a600 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -66,7 +66,8 @@ static void help(void)
            "  'filename' is a disk image filename\n"
            "  'fmt' is the disk image format. It is guessed automatically in most cases\n"
            "  'cache' is the cache mode used to write the output disk image, the valid\n"
-           "    options are: 'none', 'writeback' (default), 'writethrough' and 'unsafe'\n"
+           "    options are: 'none', 'writeback' (default), 'writethrough', 'directsync'\n"
+           "    and 'unsafe'\n"
            "  'size' is the disk image size in bytes. Optional suffixes\n"
            "    'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G' (gigabyte, 1024M)\n"
            "    and T (terabyte, 1024G) are supported. 'b' is ignored.\n"
diff --git a/qemu-options.hx b/qemu-options.hx
index d86815d..35d95d1 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -133,7 +133,7 @@ ETEXI
 DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
     "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
-    "       [,cache=writethrough|writeback|none|unsafe][,format=f]\n"
+    "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
     "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
     "       [,readonly=on|off]\n"
     "                use 'file' as a drive image\n", QEMU_ARCH_ALL)
@@ -164,7 +164,7 @@ These options have the same definition as they have in @option{-hdachs}.
 @item snapshot=@var{snapshot}
 @var{snapshot} is "on" or "off" and allows to enable snapshot for given drive (see @option{-snapshot}).
 @item cache=@var{cache}
-@var{cache} is "none", "writeback", "unsafe", or "writethrough" and controls how the host cache is used to access block data.
+@var{cache} is "none", "writeback", "unsafe", "directsync" or "writethrough" and controls how the host cache is used to access block data.
 @item aio=@var{aio}
 @var{aio} is "threads", or "native" and selects between pthread based disk I/O and native Linux AIO.
 @item format=@var{format}
@@ -199,6 +199,10 @@ The host page cache can be avoided entirely with @option{cache=none}.  This will
 attempt to do disk IO directly to the guests memory.  QEMU may still perform
 an internal copy of the data.
 
+The host page cache can be avoided while only sending write notifications to
+the guest when the data has been reported as written by the storage subsystem
+using @option{cache=directsync}.
+
 Some block drivers perform badly with @option{cache=writethrough}, most notably,
 qcow2.  If performance is more important than correctness,
 @option{cache=writeback} should be used with qcow2.
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH v2 1/2] block: set cache mode flags in a single place

[Christoph Hellwig]

On Thu, Aug 04, 2011 at 09:46:33AM +0100, Stefan Hajnoczi wrote:
> +/**
> + * Set open flags for a given cache mode
> + *
> + * Return 0 on success, -1 if the cache mode was invalid.
> + */
> +int bdrv_set_cache_flags(const char *mode, int *flags)

I would call this bdrv_parse_cache_flags.

Re: [Qemu-devel] [PATCH v2 1/2] block: set cache mode flags in a single place

[Stefan Hajnoczi]

On Thu, Aug 4, 2011 at 12:14 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Thu, Aug 04, 2011 at 09:46:33AM +0100, Stefan Hajnoczi wrote:
>> +/**
>> + * Set open flags for a given cache mode
>> + *
>> + * Return 0 on success, -1 if the cache mode was invalid.
>> + */
>> +int bdrv_set_cache_flags(const char *mode, int *flags)
>
> I would call this bdrv_parse_cache_flags.

Fixed in v3.

Stefan