block: optimize zero writes with bdrv_write_zeroes

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 <[email protected]>

---
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]

blockdev.c +4 -0

+static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp)
+{
+    if (!buf || !strcmp(buf, "off")) {
+        return BDRV_DETECT_ZEROES_OFF;

+++ b/qapi-schema.json

qapi-schema.json +2 -0

@@ -937,6 +937,8 @@
 # @encryption_key_missing: true if the backing device is encrypted but an
 #                          valid encryption key is missing
 #
+# @detect_zeroes: detect and optimize zero writes (Since 2.1)
+#
 # @bps: total throughput limit in bytes per second is specified
 #
 # @bps_rd: read throughput limit in bytes per second is specified

qapi-schema.json +8 -0

+# @unmap:    Enabled and even try to unmap blocks if possible
+#
+# Since: 2.1
+##
+{ 'enum': 'BlockdevDetectZeroesOptions',
+  'data': [ 'off', 'on', 'unmap' ] }
+
+##
 # @BlockdevAioOptions
 #
 # Selects the AIO backend to handle I/O requests

Quoted 19 lines · Peter Lieven

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 <[email protected]>

---
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]

Quoted 4 lines

blockdev.c +4 -0

+static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp)
+{
+    if (!buf || !strcmp(buf, "off")) {
+        return BDRV_DETECT_ZEROES_OFF;

blockdev.c static int parse_block_error_action(const char *buf, bool is_read, Error **errp) · line 304

Quoted 10 lines

blockdev.c +10 -0

+    } 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?

Quoted 12 lines

+++ b/qapi-schema.json

qapi-schema.json +2 -0

@@ -937,6 +937,8 @@
  # @encryption_key_missing: true if the backing device is encrypted but an
  #                          valid encryption key is missing
  #
+# @detect_zeroes: detect and optimize zero writes (Since 2.1)
+#
  # @bps: total throughput limit in bytes per second is specified
  #
  # @bps_rd: read throughput limit in bytes per second is specified

qapi-schema.json line 977

Quoted 4 lines

qapi-schema.json +1 -0

@@ -972,6 +974,7 @@
    'data': { 'file': 'str', '*node-name': 'str', 'ro': 'bool', 'drv': 'str',
              '*backing_file': 'str', 'backing_file_depth': 'int',
              'encrypted': 'bool', 'encryption_key_missing': 'bool',
+            'detect_zeroes': 'str',

For new additions, we try to prefer dash over underscore. Eww - we've already been inconsistent in this struct, with backing_file vs. node-name.

Maybe s/detect_zeroes/detect-zeroes/. I obviously can't argue too strongly in light of the rest of the struct in isolation. However, you DID use detect-zeroes on the input side later in the patch, so being consistent between the two sites would be nice (given that node-name was named consistently).

On the other hand, I _can_ argue strongly that open-coding this as 'str' is wrong. You already added an enum type, so use it:

'detect_zeroes': 'BlockdevDetectZeroesOptions',

(hmm, in this context, it's not really an option, so maybe there is some other name we can bikeshed about; but beyond 'BlockdevDetectZeroes', I don't have any other good ideas)

zero is one of those odd words with two different pluralized spellings both in common use. Code base currently has a 1:2 ratio between 'zeros' vs. 'zeroes', so I guess you're okay; on the other hand, 'man qemu-img' documents that 'convert -S' detects "zeros".

Quoted 7 lines

qapi-schema.json +0 -0

              'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
              'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
              'image': 'ImageInfo',

@@ -4250,6 +4253,20 @@
    'data': [ 'ignore', 'unmap' ] }
 

qapi-schema.json line 4258

Quoted 4 lines

qapi-schema.json +3 -0

  ##
+# @BlockdevDetectZeroesOptions
+#
+# Selects the operation mode for zero write detection.

s/Selects/Describes/ since you are also going to use this enum on the output field.

qapi-schema.json line 4261

Quoted 4 lines

qapi-schema.json +3 -0

+#
+# @off:      Disabled
+# @on:       Enabled

Maybe more details? Seeing this doesn't tell me the tradeoffs involved in tweaking the parameter (one is faster, while the other uses less storage resources - so maybe mention those benefits). I see the documentation later on for the command line, so maybe repeating some of that here would help someone going by just the QMP interface.

Quoted 13 lines

qapi-schema.json +8 -0

+# @unmap:    Enabled and even try to unmap blocks if possible
+#
+# Since: 2.1
+##
+{ 'enum': 'BlockdevDetectZeroesOptions',
+  'data': [ 'off', 'on', 'unmap' ] }
+
+##
  # @BlockdevAioOptions
  #
  # Selects the AIO backend to handle I/O requests

qapi-schema.json line 4350

Quoted 6 lines

qapi-schema.json +2 -1

@@ -4327,7 +4346,8 @@
              '*aio': 'BlockdevAioOptions',
              '*rerror': 'BlockdevOnError',
              '*werror': 'BlockdevOnError',
-            '*read-only': 'bool' } }
+            '*read-only': 'bool',
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }

This part is fine.

qemu-options.hx Open drive @option{file} as read-only. Guest write attempts will fail. · line 483

Quoted 7 lines

qemu-options.hx +5 -0

  @var{copy-on-read} is "on" or "off" and enables whether to copy read backing
  file sectors into the image file.
+@item detect-zeroes=@var{detect-zeroes}
+@var{detect-zeroes} is "off", "on" or "unmap" and enables the automatic
+conversion of plain zero writes by the OS to driver specific optimized
+zero write commands. If "unmap" is chosen and @var{discard} is "on"
+a zero write may even be converted to an UNMAP operation.

This looks good.

Quoted 19 lines · Peter Lieven

Quoted 18 lines

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 <[email protected]>

---
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]

Quoted 16 lines

Quoted 4 lines

blockdev.c +4 -0

+static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp)
+{
+    if (!buf || !strcmp(buf, "off")) {
+        return BDRV_DETECT_ZEROES_OFF;

blockdev.c static int parse_block_error_action(const char *buf, bool is_read, Error **errp) · line 304

Quoted 10 lines

blockdev.c +10 -0

+    } 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.

Am 07.05.2014 um 16:26 hat Peter Lieven geschrieben:

Quoted 36 lines · Eric Blake

Quoted 35 lines · Peter Lieven

Quoted 22 lines

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 <[email protected]>

---
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]

blockdev.c +4 -0

+static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp)
+{
+    if (!buf || !strcmp(buf, "off")) {
+        return BDRV_DETECT_ZEROES_OFF;

blockdev.c static int parse_block_error_action(const char *buf, bool is_read, Error **errp) · line 304

Quoted 10 lines

blockdev.c +10 -0

+    } 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?

Quoted 4 lines

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.

Kevin

Quoted 41 lines

Am 07.05.2014 um 16:26 hat Peter Lieven geschrieben:

Quoted 40 lines · Eric Blake

Quoted 19 lines · Peter Lieven

Quoted 18 lines

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 <[email protected]>

---
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]

Quoted 16 lines

Quoted 4 lines

blockdev.c +4 -0

+static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp)
+{
+    if (!buf || !strcmp(buf, "off")) {
+        return BDRV_DETECT_ZEROES_OFF;

blockdev.c static int parse_block_error_action(const char *buf, bool is_read, Error **errp) · line 304

Quoted 10 lines

blockdev.c +10 -0

+    } 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.

Quoted 4 lines

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?

Am 07.05.2014 um 17:26 hat Peter Lieven geschrieben:

Quoted 46 lines · Kevin Wolf

Quoted 45 lines

Am 07.05.2014 um 16:26 hat Peter Lieven geschrieben:

Quoted 36 lines · Eric Blake

Quoted 19 lines · Peter Lieven

Quoted 18 lines

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 <[email protected]>

---
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]

Quoted 16 lines

Quoted 4 lines

blockdev.c +4 -0

+static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp)
+{
+    if (!buf || !strcmp(buf, "off")) {
+        return BDRV_DETECT_ZEROES_OFF;

blockdev.c static int parse_block_error_action(const char *buf, bool is_read, Error **errp) · line 304

Quoted 10 lines

blockdev.c +10 -0

+    } 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?

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 47 lines · Kevin Wolf

Am 07.05.2014 um 17:26 hat Peter Lieven geschrieben:

Quoted 46 lines · Kevin Wolf

Quoted 45 lines

Am 07.05.2014 um 16:26 hat Peter Lieven geschrieben:

Quoted 36 lines · Eric Blake

Quoted 19 lines · Peter Lieven

Quoted 18 lines

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 <[email protected]>

---
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]

Quoted 16 lines

Quoted 4 lines

(quoted diff) +4 -0

+static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp)
+{
+    if (!buf || !strcmp(buf, "off")) {
+        return BDRV_DETECT_ZEROES_OFF;

Review

Quoted 10 lines

(quoted diff) +10 -0

+    } 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?

Quoted 4 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

I am in blockdev_init and want to set dinfo->bdrv->detect_zeroes to the correct ID. Do you have a fast solution? Everything else in this function is not using QAPI calls.

Quoted 48 lines · Kevin Wolf

Quoted 47 lines

Am 07.05.2014 um 17:26 hat Peter Lieven geschrieben:

Quoted 46 lines · Kevin Wolf

Quoted 45 lines

Am 07.05.2014 um 16:26 hat Peter Lieven geschrieben:

Quoted 36 lines · Eric Blake

Quoted 19 lines · Peter Lieven

Quoted 18 lines

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 <[email protected]>

---
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]

Quoted 16 lines

Quoted 4 lines

(quoted diff) +4 -0

+static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp)
+{
+    if (!buf || !strcmp(buf, "off")) {
+        return BDRV_DETECT_ZEROES_OFF;

Review

Quoted 10 lines

(quoted diff) +10 -0

+    } 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?

Quoted 4 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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.

I am in blockdev_init and want to set dinfo->bdrv->detect_zeroes to the correct ID. Do you have a fast solution? Everything else in this function is not using QAPI calls.

Actually, there is already a manual parsing for the werror and rerror in this function.

I think for the future there might be need for a generic function that maps a string to the ID of an enum object.

If you don't have objections I would leave the parsing function as is for the moment.

Am 07.05.2014 um 17:26 hat Peter Lieven geschrieben:

Quoted 46 lines · Kevin Wolf

Quoted 45 lines

Am 07.05.2014 um 16:26 hat Peter Lieven geschrieben:

Quoted 36 lines · Eric Blake

Quoted 19 lines · Peter Lieven

Quoted 18 lines

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 <[email protected]>

---
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]

Quoted 16 lines

Quoted 4 lines

(quoted diff) +4 -0

+static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp)
+{
+    if (!buf || !strcmp(buf, "off")) {
+        return BDRV_DETECT_ZEROES_OFF;

Review

Quoted 10 lines

(quoted diff) +10 -0

+    } 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?

Quoted 4 lines

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.

Quoted 2 lines

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