Re: [Qemu-devel] [PATCHv3] block: optimize zero writes with bdrv_write_zeroes

[Peter Lieven]

On 07.05.2014 17:38, Kevin Wolf wrote:
> Am 07.05.2014 um 17:26 hat Peter Lieven geschrieben:
> > On 07.05.2014 17:19, Kevin Wolf wrote:
> > > Am 07.05.2014 um 16:26 hat Peter Lieven geschrieben:
> > > > On 07.05.2014 04:52, Eric Blake wrote:
> > > > > On 05/06/2014 06:23 PM, Peter Lieven wrote:
> > > > > > this patch tries to optimize zero write requests
> > > > > > by automatically using bdrv_write_zeroes if it is
> > > > > > supported by the format.
> > > > > >
> > > > > > This significantly speeds up file system initialization and
> > > > > > should speed zero write test used to test backend storage
> > > > > > performance.
> > > > > >
> > > > > > Signed-off-by: Peter Lieven <address@hidden>
> > > > > > ---
> > > > > > v2->v3: - moved parameter parsing to blockdev_init
> > > > > >          - added per device detect_zeroes status to
> > > > > >            hmp (info block -v) and qmp (query-block) [Eric]
> > > > > >          - added support to enable detect-zeroes also
> > > > > >            for hot added devices [Eric]
> > > > > >          - added missing entry to qemu_common_drive_opts
> > > > > >          - fixed description of qemu_iovec_is_zero [Fam]
> > > > > >
> > > > > > +static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp)
> > > > > > +{
> > > > > > +    if (!buf || !strcmp(buf, "off")) {
> > > > > > +        return BDRV_DETECT_ZEROES_OFF;
> > > > > > +    } else if (!strcmp(buf, "on")) {
> > > > > > +        return BDRV_DETECT_ZEROES_ON;
> > > > > > +    } else if (!strcmp(buf, "unmap")) {
> > > > > > +        return BDRV_DETECT_ZEROES_UNMAP;
> > > > > > +    } else {
> > > > > > +        error_setg(errp, "invalid value for detect-zeroes: %s",
> > > > > > +                   buf);
> > > > > > +    }
> > > > > > +    return BDRV_DETECT_ZEROES_OFF;
> > > > > > +}
> > > > > Isn't there QAPI generated code that you can use instead of open-coding
> > > > > this conversion between string and enum values?
> > > > Actually I have no idea. As you pointed out in the qapi patch I sent
> > > > it was quite hard for me to crawl through the whole stuff as one who is not
> > > > familiar with it. Can somebody advise here? Anyhow, I wonder
> > > > how this would work since qapi doesn't know the C Macros.
> > > QAPI does generate C enums, so you should take whatever identifier it
> > > uses instead of defining your own macros. You may need to include
> > > qapi-types.h for this. It also creates a *_lookup array that maps enum
> > > IDs to strings.
> > Ah, cool stuff, thank you. I found the enum and the lookup array,
> > but is there also a function that maps a string to an enum ID?
> I don't think so, but if you need it, you're probably doing something
> wrong because QAPI already calls you with an enum parameter and not a
> char* one.
>
> Kevin

This works for me:

diff --git a/blockdev.c b/blockdev.c
index 7810e9f..955bd49 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -288,6 +288,23 @@ static int parse_block_error_action(const char *buf, bool 
is_read, Error **errp)
     }
 }

+static int parse_enum_option(const char *lookup[], const char *buf, int max,
+                             int def, Error **errp)
+{
+    int i;
+    if (!buf) {
+        return def;
+    }
+    for (i = 0; i < max; i++) {
+        if (!strcmp(buf, lookup[i])) {
+            return i;
+        }
+    }
+    error_setg(errp, "invalid parameter value: %s",
+               buf);
+    return def;
+}
+
 static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
 {
     if (throttle_conflicting(cfg)) {
@@ -324,6 +341,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
*bs_opts,
     QemuOpts *opts;
     const char *id;
     bool has_driver_specific_opts;
+    BlockdevDetectZeroesOptions detect_zeroes;
     BlockDriver *drv = NULL;

     /* Check common options by copying from bs_opts to opts, all other options
@@ -452,6 +470,17 @@ static DriveInfo *blockdev_init(const char *file, QDict 
*bs_opts,
         }
     }

+    detect_zeroes =
+        parse_enum_option(BlockdevDetectZeroesOptions_lookup,
+                          qemu_opt_get(opts, "detect-zeroes"),
+                          BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
+                          BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
+                          &error);
+    if (error) {
+        error_propagate(errp, error);
+        goto early_err;
+    }
+
     /* init */
     dinfo = g_malloc0(sizeof(*dinfo));
     dinfo->id = g_strdup(qemu_opts_id(opts));