Re: [Qemu-devel] [PULL 10/18] block: Move 'discard' option to bdrv_open_common()

[Kevin Wolf]

Am 07.10.2016 um 11:01 hat Gerd Hoffmann geschrieben:
> On Di, 2016-09-27 at 15:53 +0200, Kevin Wolf wrote:
> > This enables its use for nested child nodes. The compatibility
> > between the 'discard' and 'detect-zeroes' setting is checked in
> > bdrv_open_common() now as the former setting isn't available before
> > calling bdrv_open() any more.
> 
> Seems this change makes libvirt not see the discard option any more,
> "virsh start" throws this error now ...
> 
> error: Failed to start domain fedora-org-xhci
> error: unsupported configuration: discard is not supported by this QEMU
> binary
> 
> ... and git bisect found this commit.

The fix is already on the list:

[PATCH] block: Add bdrv_runtime_opts to query-command-line-options

Kevin

Re: [Qemu-devel] [PULL 10/18] block: Move 'discard' option to bdrv_open_common()

[Gerd Hoffmann]

On Di, 2016-09-27 at 15:53 +0200, Kevin Wolf wrote:
> This enables its use for nested child nodes. The compatibility
> between the 'discard' and 'detect-zeroes' setting is checked in
> bdrv_open_common() now as the former setting isn't available before
> calling bdrv_open() any more.

Seems this change makes libvirt not see the discard option any more,
"virsh start" throws this error now ...

error: Failed to start domain fedora-org-xhci
error: unsupported configuration: discard is not supported by this QEMU
binary

... and git bisect found this commit.

cheers,
  Gerd

[Qemu-devel] [PULL 10/18] block: Move 'discard' option to bdrv_open_common()

[Kevin Wolf]

This enables its use for nested child nodes. The compatibility
between the 'discard' and 'detect-zeroes' setting is checked in
bdrv_open_common() now as the former setting isn't available before
calling bdrv_open() any more.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
---
 block.c   | 17 -
 blockdev.c| 25 -
 include/block/block.h |  1 +
 3 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 1f10457..bb1f1ec 100644
--- a/block.c
+++ b/block.c
@@ -765,7 +765,7 @@ static void bdrv_inherited_options(int *child_flags, QDict 
*child_options,
 /* Our block drivers take care to send flushes and respect unmap policy,
  * so we can default to enable both on lower layers regardless of the
  * corresponding parent options. */
-flags |= BDRV_O_UNMAP;
+qdict_set_default_str(child_options, BDRV_OPT_DISCARD, "unmap");
 
 /* Clear flags that only apply to the top layer */
 flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ |
@@ -960,6 +960,11 @@ static QemuOptsList bdrv_runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "try to optimize zero writes (off, on, unmap)",
 },
+{
+.name = "discard",
+.type = QEMU_OPT_STRING,
+.help = "discard operation (ignore/off, unmap/on)",
+},
 { /* end of list */ }
 },
 };
@@ -976,6 +981,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild 
*file,
 const char *filename;
 const char *driver_name = NULL;
 const char *node_name = NULL;
+const char *discard;
 const char *detect_zeroes;
 QemuOpts *opts;
 BlockDriver *drv;
@@ -1045,6 +1051,15 @@ static int bdrv_open_common(BlockDriverState *bs, 
BdrvChild *file,
 }
 }
 
+discard = qemu_opt_get(opts, "discard");
+if (discard != NULL) {
+if (bdrv_parse_discard_flags(discard, >open_flags) != 0) {
+error_setg(errp, "Invalid discard option");
+ret = -EINVAL;
+goto fail_opts;
+}
+}
+
 detect_zeroes = qemu_opt_get(opts, "detect-zeroes");
 if (detect_zeroes) {
 BlockdevDetectZeroesOptions value =
diff --git a/blockdev.c b/blockdev.c
index 7b87bd8..e2ace04 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -356,7 +356,6 @@ static void extract_common_blockdev_options(QemuOpts *opts, 
int *bdrv_flags,
 const char **throttling_group, ThrottleConfig *throttle_cfg,
 BlockdevDetectZeroesOptions *detect_zeroes, Error **errp)
 {
-const char *discard;
 Error *local_error = NULL;
 const char *aio;
 
@@ -365,13 +364,6 @@ static void extract_common_blockdev_options(QemuOpts 
*opts, int *bdrv_flags,
 *bdrv_flags |= BDRV_O_COPY_ON_READ;
 }
 
-if ((discard = qemu_opt_get(opts, "discard")) != NULL) {
-if (bdrv_parse_discard_flags(discard, bdrv_flags) != 0) {
-error_setg(errp, "Invalid discard option");
-return;
-}
-}
-
 if ((aio = qemu_opt_get(opts, "aio")) != NULL) {
 if (!strcmp(aio, "native")) {
 *bdrv_flags |= BDRV_O_NATIVE_AIO;
@@ -449,15 +441,6 @@ static void extract_common_blockdev_options(QemuOpts 
*opts, int *bdrv_flags,
 error_propagate(errp, local_error);
 return;
 }
-
-if (bdrv_flags &&
-*detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
-!(*bdrv_flags & BDRV_O_UNMAP))
-{
-error_setg(errp, "setting detect-zeroes to unmap is not allowed "
- "without setting discard operation to unmap");
-return;
-}
 }
 }
 
@@ -3990,10 +3973,6 @@ QemuOptsList qemu_common_drive_opts = {
 .type = QEMU_OPT_BOOL,
 .help = "enable/disable snapshot mode",
 },{
-.name = "discard",
-.type = QEMU_OPT_STRING,
-.help = "discard operation (ignore/off, unmap/on)",
-},{
 .name = "aio",
 .type = QEMU_OPT_STRING,
 .help = "host AIO implementation (threads, native)",
@@ -4125,10 +4104,6 @@ static QemuOptsList qemu_root_bds_opts = {
 .head = QTAILQ_HEAD_INITIALIZER(qemu_root_bds_opts.head),
 .desc = {
 {
-.name = "discard",
-.type = QEMU_OPT_STRING,
-.help = "discard operation (ignore/off, unmap/on)",
-},{
 .name = "aio",
 .type = QEMU_OPT_STRING,
 .help = "host AIO implementation (threads, native)",
diff --git a/include/block/block.h b/include/block/block.h
index 811b060..107c603 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -108,6 +108,7 @@ typedef struct HDGeometry {
 #define BDRV_OPT_CACHE_DIRECT   "cache.direct"
 #define