[PATCH v4 0/6] Allow changing bs->file on reopen

[PATCH v4 0/6] Allow changing bs->file on reopen

[Alberto Garcia]

Based-on: <20210317143529.615584-1-vsementsov@virtuozzo.com>

Hello,

this is the same as v3, but rebased on top of Vladimir's "block:
update graph permissions update v3", which you can get here:

git: https://src.openvz.org/scm/~vsementsov/qemu.git
tag: up-block-topologic-perm-v3

Tip: you may find it easier to review patch 4 if you use 'git diff -w'
since a big part of the changes that you see in
qmp_x_blockdev_reopen() are just indentation changes.

Berto

v4:
- Rebase on top of version 3 of Vladimir's branch
v3: https://lists.gnu.org/archive/html/qemu-block/2021-03/msg00553.html
v2: https://lists.gnu.org/archive/html/qemu-block/2021-02/msg00623.html
v1: https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00437.html

Output of git backport-diff against v3:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/6:[----] [--] 'block: Add bdrv_reopen_queue_free()'
002/6:[0018] [FC] 'block: Allow changing bs->file on reopen'
003/6:[----] [--] 'iotests: Test replacing files with x-blockdev-reopen'
004/6:[0071] [FC] 'block: Support multiple reopening with x-blockdev-reopen'
005/6:[----] [--] 'iotests: Test reopening multiple devices at the same time'
006/6:[----] [-C] 'block: Make blockdev-reopen stable API'

Alberto Garcia (6):
  block: Add bdrv_reopen_queue_free()
  block: Allow changing bs->file on reopen
  iotests: Test replacing files with x-blockdev-reopen
  block: Support multiple reopening with x-blockdev-reopen
  iotests: Test reopening multiple devices at the same time
  block: Make blockdev-reopen stable API

 qapi/block-core.json       |  24 ++---
 include/block/block.h      |   2 +
 block.c                    | 135 ++++++++++++++++----------
 blockdev.c                 |  78 +++++++++------
 tests/qemu-iotests/155     |   9 +-
 tests/qemu-iotests/165     |   4 +-
 tests/qemu-iotests/245     | 190 +++++++++++++++++++++++++++++++++----
 tests/qemu-iotests/245.out |  11 ++-
 tests/qemu-iotests/248     |   4 +-
 tests/qemu-iotests/248.out |   2 +-
 tests/qemu-iotests/296     |  11 ++-
 tests/qemu-iotests/298     |   4 +-
 12 files changed, 351 insertions(+), 123 deletions(-)

-- 
2.20.1

[PATCH v4 1/6] block: Add bdrv_reopen_queue_free()

[Alberto Garcia]

Move the code to free a BlockReopenQueue to a separate function.
It will be used in a subsequent patch.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 include/block/block.h |  1 +
 block.c               | 16 ++++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 8d5b3ecebd..5eb1e4cab9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -385,6 +385,7 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     BlockDriverState *bs, QDict *options,
                                     bool keep_old_opts);
+void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue);
 int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
 int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
                               Error **errp);
diff --git a/block.c b/block.c
index 95d8246d92..764cdbec7d 100644
--- a/block.c
+++ b/block.c
@@ -3985,6 +3985,17 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                    NULL, 0, keep_old_opts);
 }
 
+void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue)
+{
+    if (bs_queue) {
+        BlockReopenQueueEntry *bs_entry, *next;
+        QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+            g_free(bs_entry);
+        }
+        g_free(bs_queue);
+    }
+}
+
 /*
  * Reopen multiple BlockDriverStates atomically & transactionally.
  *
@@ -4088,10 +4099,7 @@ abort:
     }
 
 cleanup:
-    QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
-        g_free(bs_entry);
-    }
-    g_free(bs_queue);
+    bdrv_reopen_queue_free(bs_queue);
 
     return ret;
 }
-- 
2.20.1

Re: [PATCH v4 1/6] block: Add bdrv_reopen_queue_free()

[Vladimir Sementsov-Ogievskiy]

17.03.2021 20:15, Alberto Garcia wrote:
> Move the code to free a BlockReopenQueue to a separate function.
> It will be used in a subsequent patch.
> 
> Signed-off-by: Alberto Garcia<berto@igalia.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH v4 2/6] block: Allow changing bs->file on reopen

[Alberto Garcia]

When the x-blockdev-reopen was added it allowed reconfiguring the
graph by replacing backing files, but changing the 'file' option was
forbidden. Because of this restriction some operations are not
possible, notably inserting and removing block filters.

This patch adds support for replacing the 'file' option. This is
similar to replacing the backing file and the user is likewise
responsible for the correctness of the resulting graph, otherwise this
can lead to data corruption.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 include/block/block.h  |   1 +
 block.c                | 119 ++++++++++++++++++++++++++---------------
 tests/qemu-iotests/245 |   9 ++--
 3 files changed, 81 insertions(+), 48 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 5eb1e4cab9..e2732a0187 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -209,6 +209,7 @@ typedef struct BDRVReopenState {
     bool backing_missing;
     bool replace_backing_bs;  /* new_backing_bs is ignored if this is false */
     BlockDriverState *old_backing_bs; /* keep pointer for permissions update */
+    BlockDriverState *old_file_bs;    /* keep pointer for permissions update */
     QDict *options;
     QDict *explicit_options;
     void *opaque;
diff --git a/block.c b/block.c
index 764cdbec7d..8ff0afd77b 100644
--- a/block.c
+++ b/block.c
@@ -98,7 +98,7 @@ static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,
 
 static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
                                BlockReopenQueue *queue,
-                               Transaction *set_backings_tran, Error **errp);
+                               Transaction *tran, Error **errp);
 static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 
@@ -4049,6 +4049,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
             refresh_list = bdrv_topological_dfs(refresh_list, found,
                                                 state->old_backing_bs);
         }
+        if (state->old_file_bs) {
+            refresh_list = bdrv_topological_dfs(refresh_list, found,
+                                                state->old_file_bs);
+        }
     }
 
     /*
@@ -4161,65 +4165,77 @@ static bool bdrv_reopen_can_attach(BlockDriverState *parent,
  *
  * Return 0 on success, otherwise return < 0 and set @errp.
  */
-static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
-                                     Transaction *set_backings_tran,
-                                     Error **errp)
+static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
+                                             bool parse_file, Transaction *tran,
+                                             Error **errp)
 {
     BlockDriverState *bs = reopen_state->bs;
-    BlockDriverState *overlay_bs, *below_bs, *new_backing_bs;
+    BlockDriverState *overlay_bs, *below_bs, *new_child_bs;
+    BdrvChild *child = parse_file ? bs->file : bs->backing;
     QObject *value;
     const char *str;
 
-    value = qdict_get(reopen_state->options, "backing");
+    value = qdict_get(reopen_state->options, parse_file ? "file" : "backing");
     if (value == NULL) {
         return 0;
     }
 
     switch (qobject_type(value)) {
     case QTYPE_QNULL:
-        new_backing_bs = NULL;
+        assert(!parse_file); /* The 'file' option does not allow a null value */
+        new_child_bs = NULL;
         break;
     case QTYPE_QSTRING:
         str = qstring_get_str(qobject_to(QString, value));
-        new_backing_bs = bdrv_lookup_bs(NULL, str, errp);
-        if (new_backing_bs == NULL) {
+        new_child_bs = bdrv_lookup_bs(NULL, str, errp);
+        if (new_child_bs == NULL) {
             return -EINVAL;
-        } else if (bdrv_recurse_has_child(new_backing_bs, bs)) {
-            error_setg(errp, "Making '%s' a backing file of '%s' "
-                       "would create a cycle", str, bs->node_name);
+        } else if (bdrv_recurse_has_child(new_child_bs, bs)) {
+            error_setg(errp, "Making '%s' a %s of '%s' would create a cycle",
+                       str, parse_file ? "file" : "backing file",
+                       bs->node_name);
             return -EINVAL;
         }
         break;
     default:
-        /* 'backing' does not allow any other data type */
+        /* The options QDict has been flattened, so 'backing' and 'file'
+         * do not allow any other data type here. */
         g_assert_not_reached();
     }
 
-    /*
-     * Check AioContext compatibility so that the bdrv_set_backing_hd() call in
-     * bdrv_reopen_commit() won't fail.
-     */
-    if (new_backing_bs) {
-        if (!bdrv_reopen_can_attach(bs, bs->backing, new_backing_bs, errp)) {
+    /* If 'file' points to the current child then there's nothing to do */
+    if (child_bs(child) == new_child_bs) {
+        return 0;
+    }
+
+    /* Check AioContext compatibility */
+    if (new_child_bs) {
+        if (!bdrv_reopen_can_attach(bs, child, new_child_bs, errp)) {
             return -EINVAL;
         }
     }
 
-    /*
-     * Ensure that @bs can really handle backing files, because we are
-     * about to give it one (or swap the existing one)
-     */
-    if (bs->drv->is_filter) {
-        /* Filters always have a file or a backing child */
-        if (!bs->backing) {
-            error_setg(errp, "'%s' is a %s filter node that does not support a "
-                       "backing child", bs->node_name, bs->drv->format_name);
+    if (parse_file) {
+        assert(child && child->bs);
+    } else {
+        /*
+         * Ensure that @bs can really handle backing files, because we are
+         * about to give it one (or swap the existing one)
+         */
+        if (bs->drv->is_filter) {
+            /* Filters always have a file or a backing child */
+            if (!bs->backing) {
+                error_setg(errp, "'%s' is a %s filter node "
+                           "that does not support a backing child",
+                           bs->node_name, bs->drv->format_name);
+                return -EINVAL;
+            }
+        } else if (!bs->drv->supports_backing) {
+            error_setg(errp, "Driver '%s' of node '%s' "
+                       "does not support backing files",
+                       bs->drv->format_name, bs->node_name);
             return -EINVAL;
         }
-    } else if (!bs->drv->supports_backing) {
-        error_setg(errp, "Driver '%s' of node '%s' does not support backing "
-                   "files", bs->drv->format_name, bs->node_name);
-        return -EINVAL;
     }
 
     /*
@@ -4238,13 +4254,13 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
     }
 
     /* If we want to replace the backing file we need some extra checks */
-    if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) {
+    if (new_child_bs != bdrv_filter_or_cow_bs(overlay_bs)) {
         int ret;
 
         /* Check for implicit nodes between bs and its backing file */
         if (bs != overlay_bs) {
-            error_setg(errp, "Cannot change backing link if '%s' has "
-                       "an implicit backing file", bs->node_name);
+            error_setg(errp, "Cannot change %s link if '%s' has an implicit "
+                       "child", parse_file ? "file" : "backing", bs->node_name);
             return -EPERM;
         }
         /*
@@ -4256,16 +4272,24 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
          * with bs->drv->supports_backing == true.
          */
         if (bdrv_is_backing_chain_frozen(overlay_bs,
-                                         child_bs(overlay_bs->backing), errp))
+                                         bdrv_filter_or_cow_bs(overlay_bs),
+                                         errp))
         {
             return -EPERM;
         }
-        reopen_state->replace_backing_bs = true;
-        reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL;
-        ret = bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran,
-                                      errp);
-        if (ret < 0) {
-            return ret;
+        if (parse_file) {
+            /* Store the old file bs, we'll need to refresh its permissions */
+            reopen_state->old_file_bs = bs->file->bs;
+
+            /* And finally replace the child */
+            bdrv_replace_child(bs->file, new_child_bs, tran);
+        } else {
+            reopen_state->replace_backing_bs = true;
+            reopen_state->old_backing_bs = child_bs(bs->backing);
+            ret = bdrv_set_backing_noperm(bs, new_child_bs, tran, errp);
+            if (ret < 0) {
+                return ret;
+            }
         }
     }
 
@@ -4291,7 +4315,7 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
  */
 static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
                                BlockReopenQueue *queue,
-                               Transaction *set_backings_tran, Error **errp)
+                               Transaction *tran, Error **errp)
 {
     int ret = -1;
     int old_flags;
@@ -4411,12 +4435,19 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
      * either a reference to an existing node (using its node name)
      * or NULL to simply detach the current backing file.
      */
-    ret = bdrv_reopen_parse_backing(reopen_state, set_backings_tran, errp);
+    ret = bdrv_reopen_parse_file_or_backing(reopen_state, false, tran, errp);
     if (ret < 0) {
         goto error;
     }
     qdict_del(reopen_state->options, "backing");
 
+    /* Allow changing the 'file' option. In this case NULL is not allowed */
+    ret = bdrv_reopen_parse_file_or_backing(reopen_state, true, tran, errp);
+    if (ret < 0) {
+        goto error;
+    }
+    qdict_del(reopen_state->options, "file");
+
     /* Options that are not handled are only okay if they are unchanged
      * compared to the old state. It is expected that some options are only
      * used for the initial open, but not reopen (e.g. filename) */
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index fc5297e268..a4d0b10e9d 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -146,8 +146,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'")
         self.reopen(opts, {'driver': ''}, "Invalid parameter ''")
         self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string")
-        self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'")
-        self.reopen(opts, {'file': ''}, "Cannot change the option 'file'")
+        self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'")
+        self.reopen(opts, {'file': ''}, "Cannot find device='' nor node-name=''")
         self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef")
         self.reopen(opts, {'file.node-name': 'newname'}, "Cannot change the option 'node-name'")
         self.reopen(opts, {'file.driver': 'host_device'}, "Cannot change the option 'driver'")
@@ -455,7 +455,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         # More illegal operations
         self.reopen(opts[2], {'backing': 'hd1'},
                     "Making 'hd1' a backing file of 'hd2' would create a cycle")
-        self.reopen(opts[2], {'file': 'hd0-file'}, "Cannot change the option 'file'")
+        self.reopen(opts[2], {'file': 'hd0-file'},
+                    "Conflicts with use by hd2 as 'file', which does not allow 'write, resize' on hd0-file")
 
         result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd2')
         self.assert_qmp(result, 'error/class', 'GenericError')
@@ -969,7 +970,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 
         # We can't remove hd1 while the commit job is ongoing
         opts['backing'] = None
-        self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit backing file")
+        self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit child")
 
         # hd2 <- hd0
         self.vm.run_job('commit0', auto_finalize = False, auto_dismiss = True)
-- 
2.20.1

Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen

[Vladimir Sementsov-Ogievskiy]

17.03.2021 20:15, Alberto Garcia wrote:
> When the x-blockdev-reopen was added it allowed reconfiguring the
> graph by replacing backing files, but changing the 'file' option was
> forbidden. Because of this restriction some operations are not
> possible, notably inserting and removing block filters.
> 
> This patch adds support for replacing the 'file' option. This is
> similar to replacing the backing file and the user is likewise
> responsible for the correctness of the resulting graph, otherwise this
> can lead to data corruption.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

In general patch looks OK for me, some comments below.

> ---
>   include/block/block.h  |   1 +
>   block.c                | 119 ++++++++++++++++++++++++++---------------
>   tests/qemu-iotests/245 |   9 ++--
>   3 files changed, 81 insertions(+), 48 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 5eb1e4cab9..e2732a0187 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -209,6 +209,7 @@ typedef struct BDRVReopenState {
>       bool backing_missing;
>       bool replace_backing_bs;  /* new_backing_bs is ignored if this is false */
>       BlockDriverState *old_backing_bs; /* keep pointer for permissions update */
> +    BlockDriverState *old_file_bs;    /* keep pointer for permissions update */
>       QDict *options;
>       QDict *explicit_options;
>       void *opaque;
> diff --git a/block.c b/block.c
> index 764cdbec7d..8ff0afd77b 100644
> --- a/block.c
> +++ b/block.c
> @@ -98,7 +98,7 @@ static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,
>   
>   static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>                                  BlockReopenQueue *queue,
> -                               Transaction *set_backings_tran, Error **errp);
> +                               Transaction *tran, Error **errp);

I'd not call it just "tran" to not interfere with transaction actions. Of course, reopen should be finally refactored to work cleanly on Transaction API, but that is not done yet. And here we pass a transaction pointer only to keep children modification.. So, let's make it change_child_tran, or something like this.

>   static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
>   static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
>   
> @@ -4049,6 +4049,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
>               refresh_list = bdrv_topological_dfs(refresh_list, found,
>                                                   state->old_backing_bs);
>           }
> +        if (state->old_file_bs) {
> +            refresh_list = bdrv_topological_dfs(refresh_list, found,
> +                                                state->old_file_bs);
> +        }
>       }
>   
>       /*
> @@ -4161,65 +4165,77 @@ static bool bdrv_reopen_can_attach(BlockDriverState *parent,
>    *
>    * Return 0 on success, otherwise return < 0 and set @errp.
>    */
> -static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
> -                                     Transaction *set_backings_tran,
> -                                     Error **errp)
> +static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
> +                                             bool parse_file, Transaction *tran,
> +                                             Error **errp)
>   {
>       BlockDriverState *bs = reopen_state->bs;
> -    BlockDriverState *overlay_bs, *below_bs, *new_backing_bs;
> +    BlockDriverState *overlay_bs, *below_bs, *new_child_bs;
> +    BdrvChild *child = parse_file ? bs->file : bs->backing;
>       QObject *value;
>       const char *str;
>   
> -    value = qdict_get(reopen_state->options, "backing");
> +    value = qdict_get(reopen_state->options, parse_file ? "file" : "backing");
>       if (value == NULL) {
>           return 0;
>       }
>   
>       switch (qobject_type(value)) {
>       case QTYPE_QNULL:
> -        new_backing_bs = NULL;
> +        assert(!parse_file); /* The 'file' option does not allow a null value */
> +        new_child_bs = NULL;
>           break;
>       case QTYPE_QSTRING:
>           str = qstring_get_str(qobject_to(QString, value));
> -        new_backing_bs = bdrv_lookup_bs(NULL, str, errp);
> -        if (new_backing_bs == NULL) {
> +        new_child_bs = bdrv_lookup_bs(NULL, str, errp);
> +        if (new_child_bs == NULL) {
>               return -EINVAL;
> -        } else if (bdrv_recurse_has_child(new_backing_bs, bs)) {
> -            error_setg(errp, "Making '%s' a backing file of '%s' "
> -                       "would create a cycle", str, bs->node_name);
> +        } else if (bdrv_recurse_has_child(new_child_bs, bs)) {
> +            error_setg(errp, "Making '%s' a %s of '%s' would create a cycle",
> +                       str, parse_file ? "file" : "backing file",

maybe s/"file"/"file child"/

> +                       bs->node_name);
>               return -EINVAL;
>           }
>           break;
>       default:
> -        /* 'backing' does not allow any other data type */
> +        /* The options QDict has been flattened, so 'backing' and 'file'
> +         * do not allow any other data type here. */

checkpatch should complain that you didn't fix style of the comment...

>           g_assert_not_reached();
>       }
>   
> -    /*
> -     * Check AioContext compatibility so that the bdrv_set_backing_hd() call in
> -     * bdrv_reopen_commit() won't fail.
> -     */
> -    if (new_backing_bs) {
> -        if (!bdrv_reopen_can_attach(bs, bs->backing, new_backing_bs, errp)) {
> +    /* If 'file' points to the current child then there's nothing to do */
> +    if (child_bs(child) == new_child_bs) {
> +        return 0;
> +    }
> +
> +    /* Check AioContext compatibility */
> +    if (new_child_bs) {
> +        if (!bdrv_reopen_can_attach(bs, child, new_child_bs, errp)) {
>               return -EINVAL;
>           }
>       }
>   
> -    /*
> -     * Ensure that @bs can really handle backing files, because we are
> -     * about to give it one (or swap the existing one)
> -     */
> -    if (bs->drv->is_filter) {
> -        /* Filters always have a file or a backing child */
> -        if (!bs->backing) {
> -            error_setg(errp, "'%s' is a %s filter node that does not support a "
> -                       "backing child", bs->node_name, bs->drv->format_name);
> +    if (parse_file) {
> +        assert(child && child->bs);

I'm not sure, that we can't get children without a bs at some point.. And we have so many checks about it in the code. Probably we can drop them all? But I don't want to care to much. If this assertion fires, we'll fix a bug.

> +    } else {
> +        /*
> +         * Ensure that @bs can really handle backing files, because we are
> +         * about to give it one (or swap the existing one)
> +         */
> +        if (bs->drv->is_filter) {
> +            /* Filters always have a file or a backing child */

Probably we can assert bs->backing, as otherwise backing option should be unsupported [preexisting, not about this patch]

> +            if (!bs->backing) {
> +                error_setg(errp, "'%s' is a %s filter node "
> +                           "that does not support a backing child",
> +                           bs->node_name, bs->drv->format_name);
> +                return -EINVAL;
> +            }
> +        } else if (!bs->drv->supports_backing) {

Probably we can assert bs->drv->supports_backing, as otherwise backing option should be unsupported [preexisting, not about this patch]

> +            error_setg(errp, "Driver '%s' of node '%s' "
> +                       "does not support backing files",
> +                       bs->drv->format_name, bs->node_name);
>               return -EINVAL;
>           }
> -    } else if (!bs->drv->supports_backing) {
> -        error_setg(errp, "Driver '%s' of node '%s' does not support backing "
> -                   "files", bs->drv->format_name, bs->node_name);
> -        return -EINVAL;
>       }
>   
>       /*
> @@ -4238,13 +4254,13 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
>       }
>   
>       /* If we want to replace the backing file we need some extra checks */

You didn't update the comment.

> -    if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) {
> +    if (new_child_bs != bdrv_filter_or_cow_bs(overlay_bs)) {
>           int ret;
>   
>           /* Check for implicit nodes between bs and its backing file */
>           if (bs != overlay_bs) {
> -            error_setg(errp, "Cannot change backing link if '%s' has "
> -                       "an implicit backing file", bs->node_name);
> +            error_setg(errp, "Cannot change %s link if '%s' has an implicit "
> +                       "child", parse_file ? "file" : "backing", bs->node_name);
>               return -EPERM;
>           }
>           /*
> @@ -4256,16 +4272,24 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
>            * with bs->drv->supports_backing == true.
>            */
>           if (bdrv_is_backing_chain_frozen(overlay_bs,
> -                                         child_bs(overlay_bs->backing), errp))
> +                                         bdrv_filter_or_cow_bs(overlay_bs),
> +                                         errp))
>           {
>               return -EPERM;
>           }
> -        reopen_state->replace_backing_bs = true;
> -        reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL;
> -        ret = bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran,
> -                                      errp);
> -        if (ret < 0) {
> -            return ret;
> +        if (parse_file) {
> +            /* Store the old file bs, we'll need to refresh its permissions */
> +            reopen_state->old_file_bs = bs->file->bs;
> +
> +            /* And finally replace the child */
> +            bdrv_replace_child(bs->file, new_child_bs, tran);

I think that actually, we need also to update inherits_from and do refresh_limits like in bdrv_set_backing_noperm().

Probably, bdrv_replace_child should do it. Probably not (there are still a lot of things to refactor in block.c :)..

Hm. Also, using blockdev-reopen probably means that we are in a blockdev word, so we should not care about inherits_from here.

But at least calling bdrv_refresh_limits(bs, tran, NULL) will not hurt. (or we can check an error code and honestly return it as well).


Also, you don't create reopen_state->replace_file_bs, like for backing.. On bdrv_reopen_comnmit replace_backing_bs is used to remove corresponding options.. Shouldn't we do the same with file options?

> +        } else {
> +            reopen_state->replace_backing_bs = true;
> +            reopen_state->old_backing_bs = child_bs(bs->backing);
> +            ret = bdrv_set_backing_noperm(bs, new_child_bs, tran, errp);
> +            if (ret < 0) {
> +                return ret;
> +            }
>           }
>       }
>   
> @@ -4291,7 +4315,7 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
>    */
>   static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>                                  BlockReopenQueue *queue,
> -                               Transaction *set_backings_tran, Error **errp)
> +                               Transaction *tran, Error **errp)
>   {
>       int ret = -1;
>       int old_flags;
> @@ -4411,12 +4435,19 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>        * either a reference to an existing node (using its node name)
>        * or NULL to simply detach the current backing file.
>        */
> -    ret = bdrv_reopen_parse_backing(reopen_state, set_backings_tran, errp);
> +    ret = bdrv_reopen_parse_file_or_backing(reopen_state, false, tran, errp);
>       if (ret < 0) {
>           goto error;
>       }
>       qdict_del(reopen_state->options, "backing");
>   
> +    /* Allow changing the 'file' option. In this case NULL is not allowed */
> +    ret = bdrv_reopen_parse_file_or_backing(reopen_state, true, tran, errp);
> +    if (ret < 0) {
> +        goto error;
> +    }
> +    qdict_del(reopen_state->options, "file");
> +
>       /* Options that are not handled are only okay if they are unchanged
>        * compared to the old state. It is expected that some options are only
>        * used for the initial open, but not reopen (e.g. filename) */
> diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
> index fc5297e268..a4d0b10e9d 100755
> --- a/tests/qemu-iotests/245
> +++ b/tests/qemu-iotests/245
> @@ -146,8 +146,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
>           self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'")
>           self.reopen(opts, {'driver': ''}, "Invalid parameter ''")
>           self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string")
> -        self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'")
> -        self.reopen(opts, {'file': ''}, "Cannot change the option 'file'")
> +        self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'")

Interesting that error-message say about device='', not 'not-found'...

> +        self.reopen(opts, {'file': ''}, "Cannot find device='' nor node-name=''")
>           self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef")
>           self.reopen(opts, {'file.node-name': 'newname'}, "Cannot change the option 'node-name'")
>           self.reopen(opts, {'file.driver': 'host_device'}, "Cannot change the option 'driver'")
> @@ -455,7 +455,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
>           # More illegal operations
>           self.reopen(opts[2], {'backing': 'hd1'},
>                       "Making 'hd1' a backing file of 'hd2' would create a cycle")
> -        self.reopen(opts[2], {'file': 'hd0-file'}, "Cannot change the option 'file'")
> +        self.reopen(opts[2], {'file': 'hd0-file'},
> +                    "Conflicts with use by hd2 as 'file', which does not allow 'write, resize' on hd0-file")
>   
>           result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd2')
>           self.assert_qmp(result, 'error/class', 'GenericError')
> @@ -969,7 +970,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
>   
>           # We can't remove hd1 while the commit job is ongoing
>           opts['backing'] = None
> -        self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit backing file")
> +        self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit child")
>   
>           # hd2 <- hd0
>           self.vm.run_job('commit0', auto_finalize = False, auto_dismiss = True)
> 

-- 
Best regards,
Vladimir

Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen

[Alberto Garcia]

On Thu 18 Mar 2021 03:25:07 PM CET, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>   static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>>                                  BlockReopenQueue *queue,
>> -                               Transaction *set_backings_tran, Error **errp);
>> +                               Transaction *tran, Error **errp);
>
> I'd not call it just "tran" to not interfere with transaction
> actions. Of course, reopen should be finally refactored to work
> cleanly on Transaction API, but that is not done yet. And here we pass
> a transaction pointer only to keep children modification.. So, let's
> make it change_child_tran, or something like this.

The name change looks good to me.

>> +        } else if (bdrv_recurse_has_child(new_child_bs, bs)) {
>> +            error_setg(errp, "Making '%s' a %s of '%s' would create a cycle",
>> +                       str, parse_file ? "file" : "backing file",
>
> maybe s/"file"/"file child"/

Ok.

>>       default:
>> -        /* 'backing' does not allow any other data type */
>> +        /* The options QDict has been flattened, so 'backing' and 'file'
>> +         * do not allow any other data type here. */
>
> checkpatch should complain that you didn't fix style of the comment...

I actually don't like to use the proposed style for 2-line comments in
many cases. I think it makes sense for big comment blocks but adds noise
for shorter comments.

>> +    } else {
>> +        /*
>> +         * Ensure that @bs can really handle backing files, because we are
>> +         * about to give it one (or swap the existing one)
>> +         */
>> +        if (bs->drv->is_filter) {
>> +            /* Filters always have a file or a backing child */
>
> Probably we can assert bs->backing, as otherwise backing option should
> be unsupported [preexisting, not about this patch]

Yes, I see that this was added in commit 1d42f48c3a, maybe Max has good
reasons to keep it this way?

>>           if (bdrv_is_backing_chain_frozen(overlay_bs,
>> -                                         child_bs(overlay_bs->backing), errp))
>> +                                         bdrv_filter_or_cow_bs(overlay_bs),
>> +                                         errp))
>>           {
>>               return -EPERM;
>>           }

I just realized that this part is probably not ok if you want to change
bs->file on a node that is not a filter, because this would check
bs->backing->frozen and not bs->file->frozen.

>> +        if (parse_file) {
>> +            /* Store the old file bs, we'll need to refresh its permissions */
>> +            reopen_state->old_file_bs = bs->file->bs;
>> +
>> +            /* And finally replace the child */
>> +            bdrv_replace_child(bs->file, new_child_bs, tran);
>
> I think that actually, we need also to update inherits_from and do
> refresh_limits like in bdrv_set_backing_noperm().

Yes, I think you're right.

> Probably, bdrv_replace_child should do it. Probably not (there are
> still a lot of things to refactor in block.c :)..
>
> Hm. Also, using blockdev-reopen probably means that we are in a
>blockdev word, so we should not care about inherits_from here.

But with blockdev-reopen we do update inherits_from for backing files,
don't we?

> Also, you don't create reopen_state->replace_file_bs, like for
> backing.. On bdrv_reopen_comnmit replace_backing_bs is used to remove
> corresponding options.. Shouldn't we do the same with file options?

I think you're right.

>> -        self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'")
>> -        self.reopen(opts, {'file': ''}, "Cannot change the option 'file'")
>> +        self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'")
>
> Interesting that error-message say about device='', not 'not-found'...

That's because 'file' refers to a node name.

Thanks for reviewing,

Berto

Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen

[Vladimir Sementsov-Ogievskiy]

24.03.2021 15:25, Alberto Garcia wrote:
> On Thu 18 Mar 2021 03:25:07 PM CET, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>    static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>>>                                   BlockReopenQueue *queue,
>>> -                               Transaction *set_backings_tran, Error **errp);
>>> +                               Transaction *tran, Error **errp);
>>
>> I'd not call it just "tran" to not interfere with transaction
>> actions. Of course, reopen should be finally refactored to work
>> cleanly on Transaction API, but that is not done yet. And here we pass
>> a transaction pointer only to keep children modification.. So, let's
>> make it change_child_tran, or something like this.
> 
> The name change looks good to me.
> 
>>> +        } else if (bdrv_recurse_has_child(new_child_bs, bs)) {
>>> +            error_setg(errp, "Making '%s' a %s of '%s' would create a cycle",
>>> +                       str, parse_file ? "file" : "backing file",
>>
>> maybe s/"file"/"file child"/
> 
> Ok.
> 
>>>        default:
>>> -        /* 'backing' does not allow any other data type */
>>> +        /* The options QDict has been flattened, so 'backing' and 'file'
>>> +         * do not allow any other data type here. */
>>
>> checkpatch should complain that you didn't fix style of the comment...
> 
> I actually don't like to use the proposed style for 2-line comments in
> many cases. I think it makes sense for big comment blocks but adds noise
> for shorter comments.
> 
>>> +    } else {
>>> +        /*
>>> +         * Ensure that @bs can really handle backing files, because we are
>>> +         * about to give it one (or swap the existing one)
>>> +         */
>>> +        if (bs->drv->is_filter) {
>>> +            /* Filters always have a file or a backing child */
>>
>> Probably we can assert bs->backing, as otherwise backing option should
>> be unsupported [preexisting, not about this patch]
> 
> Yes, I see that this was added in commit 1d42f48c3a, maybe Max has good
> reasons to keep it this way?
> 
>>>            if (bdrv_is_backing_chain_frozen(overlay_bs,
>>> -                                         child_bs(overlay_bs->backing), errp))
>>> +                                         bdrv_filter_or_cow_bs(overlay_bs),
>>> +                                         errp))
>>>            {
>>>                return -EPERM;
>>>            }
> 
> I just realized that this part is probably not ok if you want to change
> bs->file on a node that is not a filter, because this would check
> bs->backing->frozen and not bs->file->frozen.
> 
>>> +        if (parse_file) {
>>> +            /* Store the old file bs, we'll need to refresh its permissions */
>>> +            reopen_state->old_file_bs = bs->file->bs;
>>> +
>>> +            /* And finally replace the child */
>>> +            bdrv_replace_child(bs->file, new_child_bs, tran);
>>
>> I think that actually, we need also to update inherits_from and do
>> refresh_limits like in bdrv_set_backing_noperm().
> 
> Yes, I think you're right.
> 
>> Probably, bdrv_replace_child should do it. Probably not (there are
>> still a lot of things to refactor in block.c :)..
>>
>> Hm. Also, using blockdev-reopen probably means that we are in a
>> blockdev word, so we should not care about inherits_from here.
> 
> But with blockdev-reopen we do update inherits_from for backing files,
> don't we?

We do.. But as I understand resent Kevin's explanation on my "[PATCH RFC 0/3] block: drop inherits_from", inherits_from exists to support pre-blockdev era..

Anyway, better to support it and don't care, and drop all inherits_from logic at some bright future point.

> 
>> Also, you don't create reopen_state->replace_file_bs, like for
>> backing.. On bdrv_reopen_comnmit replace_backing_bs is used to remove
>> corresponding options.. Shouldn't we do the same with file options?
> 
> I think you're right.
> 
>>> -        self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'")
>>> -        self.reopen(opts, {'file': ''}, "Cannot change the option 'file'")
>>> +        self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'")
>>
>> Interesting that error-message say about device='', not 'not-found'...
> 
> That's because 'file' refers to a node name.
> 
> Thanks for reviewing,
> 
> Berto
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen

[Kevin Wolf]

Am 17.03.2021 um 18:15 hat Alberto Garcia geschrieben:
> When the x-blockdev-reopen was added it allowed reconfiguring the
> graph by replacing backing files, but changing the 'file' option was
> forbidden. Because of this restriction some operations are not
> possible, notably inserting and removing block filters.
> 
> This patch adds support for replacing the 'file' option. This is
> similar to replacing the backing file and the user is likewise
> responsible for the correctness of the resulting graph, otherwise this
> can lead to data corruption.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

> @@ -4238,13 +4254,13 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
>      }
>  
>      /* If we want to replace the backing file we need some extra checks */
> -    if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) {
> +    if (new_child_bs != bdrv_filter_or_cow_bs(overlay_bs)) {

I may be missing something, but I don't see how this whole block makes
sense for changing 'file'.

overlay_bs was found by going down the backing chain, so of course it
will be different from new_child_bs (unless you use the same node as
'backing' and as 'file'). So we run all this code that seems to be
concerned only with backing files.

Probably overlay_bs should be found by starting from child and using
bdrv_filter_or_cow_bs() only for the following loop iterations.

>          int ret;
>  
>          /* Check for implicit nodes between bs and its backing file */
>          if (bs != overlay_bs) {
> -            error_setg(errp, "Cannot change backing link if '%s' has "
> -                       "an implicit backing file", bs->node_name);
> +            error_setg(errp, "Cannot change %s link if '%s' has an implicit "
> +                       "child", parse_file ? "file" : "backing", bs->node_name);
>              return -EPERM;
>          }

With fixed overlay_bs, this check makes sense, though the comment needs
an update.

>          /*
> @@ -4256,16 +4272,24 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
>           * with bs->drv->supports_backing == true.
>           */
>          if (bdrv_is_backing_chain_frozen(overlay_bs,
> -                                         child_bs(overlay_bs->backing), errp))
> +                                         bdrv_filter_or_cow_bs(overlay_bs),
> +                                         errp))
>          {
>              return -EPERM;
>          }

This checks if bs->backing is frozen (overlay_bs == bs because of the
check above). What we really want to check is if child is frozen (i.e.
bs->backing for updating 'backing', bs->file for updating 'file). So
maybe this should be just written as that:

    if (child && child->frozen) {
        error_setg(errp, ...);
        return -EPERM;
    }

Or factor this part out from bdrv_is_backing_chain_frozen() into a
bdrv_is_child_frozen() or something like that.

Either way, this might make the whole (outdated) comment above
unnecessary because things would become a lot clearer.

> -        reopen_state->replace_backing_bs = true;
> -        reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL;
> -        ret = bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran,
> -                                      errp);
> -        if (ret < 0) {
> -            return ret;
> +        if (parse_file) {
> +            /* Store the old file bs, we'll need to refresh its permissions */
> +            reopen_state->old_file_bs = bs->file->bs;
> +
> +            /* And finally replace the child */
> +            bdrv_replace_child(bs->file, new_child_bs, tran);
> +        } else {
> +            reopen_state->replace_backing_bs = true;
> +            reopen_state->old_backing_bs = child_bs(bs->backing);
> +            ret = bdrv_set_backing_noperm(bs, new_child_bs, tran, errp);
> +            if (ret < 0) {
> +                return ret;
> +            }
>          }
>      }

Kevin

Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen

[Vladimir Sementsov-Ogievskiy]

17.03.2021 20:15, Alberto Garcia wrote:
> When the x-blockdev-reopen was added it allowed reconfiguring the
> graph by replacing backing files, but changing the 'file' option was
> forbidden. Because of this restriction some operations are not
> possible, notably inserting and removing block filters.


I now started to work on making backup-top filter public..

And I think, we'll need separate commands to insert/remove filters anyway.. As blockdev-reopen has the following problems:

1. It can't append filter above top node, connected to block-device. (but bdrv_append() can do it)

2. It can't simultaneously create new node and append it. This is important for backup-top filter, which unshares write even when has no writing parent. Now bdrv_append() works in a smart way for it: it first do both graph modification (add child to filter, and replace original node by filter) and then update graph permissions. So, we'll need a command which in one roll create filter node and inserts it where needed.

3. blockdev-reopen requires to specify all options (otherwise, they will be changed to default). So it requires passing a lot of information. But we don't need to touch any option of original bs parent to insert a filter between parent and bs. In other words, we don't want to reopen something. We want to insert filter.


===

Hmm, another mentioned use of blockdev-reopen was reopening some RO node to RW, to modify bitmaps.. And here again, blockdev-reopen is not very convenient:

1. Again, it requires to specify all options (at least, that was not default on node open).. And this only to change one property: read-only. Seems overcomplicated.

2. Bitmaps modifications are usually done in transactions. Adding a clean transaction support for small command that reopens only to RW, and back to RO on transaction finalization seems simpler, than for entire generic blockdev-reopen.


===

Hmm, interesting. x-blockdev-reopen says that not specified options are reset to default. x-blockdev-reopen works through bdrv_reopen_multiple, so I think bdrv_reopen_mutliple should reset options to default as well. Now look at bdrv_reopen_set_read_only(): it specifies only one option: "read-only". This means that all other options will be reset to default. But for sure, callers of bdrv_reopen_set_read_only() want only to change read-only status of node, not all other options. Do we have a bug here?

-- 
Best regards,
Vladimir

Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen

[Kevin Wolf]

Am 07.05.2021 um 09:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 17.03.2021 20:15, Alberto Garcia wrote:
> > When the x-blockdev-reopen was added it allowed reconfiguring the
> > graph by replacing backing files, but changing the 'file' option was
> > forbidden. Because of this restriction some operations are not
> > possible, notably inserting and removing block filters.
> 
> 
> I now started to work on making backup-top filter public..
> 
> And I think, we'll need separate commands to insert/remove filters
> anyway.. As blockdev-reopen has the following problems:
> 
> 1. It can't append filter above top node, connected to block-device.
> (but bdrv_append() can do it)

We once had some patches that made the 'drive' qdev property runtime
writable. What happened to them?

> 2. It can't simultaneously create new node and append it. This is
> important for backup-top filter, which unshares write even when has no
> writing parent. Now bdrv_append() works in a smart way for it: it
> first do both graph modification (add child to filter, and replace
> original node by filter) and then update graph permissions. So, we'll
> need a command which in one roll create filter node and inserts it
> where needed.

What backup-top could do, however, is enabling restrictions only if it
has a parent (no matter whether that parent requires writing or not).

> 3. blockdev-reopen requires to specify all options (otherwise, they
> will be changed to default). So it requires passing a lot of
> information. But we don't need to touch any option of original bs
> parent to insert a filter between parent and bs. In other words, we
> don't want to reopen something. We want to insert filter.

Yeah, but this was a decision we made consciously because otherwise we'd
have a hard time telling which options should be updated and which
shouldn't, and we would need separate QAPI types for open and reopen.

If we now say that this is a reason for avoiding blockdev-reopen even
though changing some option is the goal, that would be inconsistent.

> 
> ===
> 
> Hmm, another mentioned use of blockdev-reopen was reopening some RO
> node to RW, to modify bitmaps.. And here again, blockdev-reopen is not
> very convenient:
> 
> 1. Again, it requires to specify all options (at least, that was not
> default on node open).. And this only to change one property:
> read-only. Seems overcomplicated.
> 
> 2. Bitmaps modifications are usually done in transactions. Adding a
> clean transaction support for small command that reopens only to RW,
> and back to RO on transaction finalization seems simpler, than for
> entire generic blockdev-reopen.
> 
> 
> ===
> 
> Hmm, interesting. x-blockdev-reopen says that not specified options
> are reset to default. x-blockdev-reopen works through
> bdrv_reopen_multiple, so I think bdrv_reopen_mutliple should reset
> options to default as well. Now look at bdrv_reopen_set_read_only():
> it specifies only one option: "read-only". This means that all other
> options will be reset to default. But for sure, callers of
> bdrv_reopen_set_read_only() want only to change read-only status of
> node, not all other options. Do we have a bug here?

The difference between these cases is the keep_old_opts parameter to
bdrv_reopen_queue(). It is false for x-blockdev-reopen, but true in
bdrv_reopen_set_read_only().

Kevin

Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen

[Vladimir Sementsov-Ogievskiy]

07.05.2021 17:09, Kevin Wolf wrote:
> Am 07.05.2021 um 09:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 17.03.2021 20:15, Alberto Garcia wrote:
>>> When the x-blockdev-reopen was added it allowed reconfiguring the
>>> graph by replacing backing files, but changing the 'file' option was
>>> forbidden. Because of this restriction some operations are not
>>> possible, notably inserting and removing block filters.
>>
>>
>> I now started to work on making backup-top filter public..
>>
>> And I think, we'll need separate commands to insert/remove filters
>> anyway.. As blockdev-reopen has the following problems:
>>
>> 1. It can't append filter above top node, connected to block-device.
>> (but bdrv_append() can do it)
> 
> We once had some patches that made the 'drive' qdev property runtime
> writable. What happened to them?
> 
>> 2. It can't simultaneously create new node and append it. This is
>> important for backup-top filter, which unshares write even when has no
>> writing parent. Now bdrv_append() works in a smart way for it: it
>> first do both graph modification (add child to filter, and replace
>> original node by filter) and then update graph permissions. So, we'll
>> need a command which in one roll create filter node and inserts it
>> where needed.
> 
> What backup-top could do, however, is enabling restrictions only if it
> has a parent (no matter whether that parent requires writing or not).
> 
>> 3. blockdev-reopen requires to specify all options (otherwise, they
>> will be changed to default). So it requires passing a lot of
>> information. But we don't need to touch any option of original bs
>> parent to insert a filter between parent and bs. In other words, we
>> don't want to reopen something. We want to insert filter.
> 
> Yeah, but this was a decision we made consciously because otherwise we'd
> have a hard time telling which options should be updated and which
> shouldn't, and we would need separate QAPI types for open and reopen.
> 
> If we now say that this is a reason for avoiding blockdev-reopen even
> though changing some option is the goal, that would be inconsistent.
> 
>>
>> ===
>>
>> Hmm, another mentioned use of blockdev-reopen was reopening some RO
>> node to RW, to modify bitmaps.. And here again, blockdev-reopen is not
>> very convenient:
>>
>> 1. Again, it requires to specify all options (at least, that was not
>> default on node open).. And this only to change one property:
>> read-only. Seems overcomplicated.
>>
>> 2. Bitmaps modifications are usually done in transactions. Adding a
>> clean transaction support for small command that reopens only to RW,
>> and back to RO on transaction finalization seems simpler, than for
>> entire generic blockdev-reopen.
>>
>>
>> ===
>>
>> Hmm, interesting. x-blockdev-reopen says that not specified options
>> are reset to default. x-blockdev-reopen works through
>> bdrv_reopen_multiple, so I think bdrv_reopen_mutliple should reset
>> options to default as well. Now look at bdrv_reopen_set_read_only():
>> it specifies only one option: "read-only". This means that all other
>> options will be reset to default. But for sure, callers of
>> bdrv_reopen_set_read_only() want only to change read-only status of
>> node, not all other options. Do we have a bug here?
> 
> The difference between these cases is the keep_old_opts parameter to
> bdrv_reopen_queue(). It is false for x-blockdev-reopen, but true in
> bdrv_reopen_set_read_only().
> 


Thanks for explanations, seems I panicked too early :) Will try your suggestions.


-- 
Best regards,
Vladimir

[PATCH v4 3/6] iotests: Test replacing files with x-blockdev-reopen

[Alberto Garcia]

This patch adds new tests in which we use x-blockdev-reopen to change
bs->file

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/245     | 109 ++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/245.out |  11 +++-
 2 files changed, 117 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index a4d0b10e9d..caebef4ac8 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -79,7 +79,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         for line in log.split("\n"):
             if line.startswith("Pattern verification failed"):
                 raise Exception("%s (command #%d)" % (line, found))
-            if re.match("read .*/.* bytes at offset", line):
+            if re.match("(read|wrote) .*/.* bytes at offset", line):
                 found += 1
         self.assertEqual(found, self.total_io_cmds,
                          "Expected output of %d qemu-io commands, found %d" %
@@ -537,6 +537,113 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'bv')
         self.assert_qmp(result, 'return', {})
 
+    # Replace the protocol layer ('file' parameter) of a disk image
+    def test_replace_file(self):
+        # Create two small raw images and add them to a running VM
+        qemu_img('create', '-f', 'raw', hd_path[0], '10k')
+        qemu_img('create', '-f', 'raw', hd_path[1], '10k')
+
+        hd0_opts = {'driver': 'file', 'node-name': 'hd0-file', 'filename':  hd_path[0] }
+        hd1_opts = {'driver': 'file', 'node-name': 'hd1-file', 'filename':  hd_path[1] }
+
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts)
+        self.assert_qmp(result, 'return', {})
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **hd1_opts)
+        self.assert_qmp(result, 'return', {})
+
+        # Add a raw format layer that uses hd0-file as its protocol layer
+        opts = {'driver': 'raw', 'node-name': 'hd', 'file': 'hd0-file'}
+
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+        self.assert_qmp(result, 'return', {})
+
+        # Fill the image with data
+        self.run_qemu_io("hd", "read  -P 0 0 10k")
+        self.run_qemu_io("hd", "write -P 0xa0 0 10k")
+
+        # Replace hd0-file with hd1-file and fill it with (different) data
+        self.reopen(opts, {'file': 'hd1-file'})
+        self.run_qemu_io("hd", "read  -P 0 0 10k")
+        self.run_qemu_io("hd", "write -P 0xa1 0 10k")
+
+        # Use hd0-file again and check that it contains the expected data
+        self.reopen(opts, {'file': 'hd0-file'})
+        self.run_qemu_io("hd", "read  -P 0xa0 0 10k")
+
+        # And finally do the same with hd1-file
+        self.reopen(opts, {'file': 'hd1-file'})
+        self.run_qemu_io("hd", "read  -P 0xa1 0 10k")
+
+    # Insert (and remove) a throttle filter
+    def test_insert_throttle_filter(self):
+        # Add an image to the VM
+        hd0_opts = hd_opts(0)
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts)
+        self.assert_qmp(result, 'return', {})
+
+        # Create a throttle-group object
+        opts = { 'qom-type': 'throttle-group', 'id': 'group0',
+                 'limits': { 'iops-total': 1000 } }
+        result = self.vm.qmp('object-add', conv_keys = False, **opts)
+        self.assert_qmp(result, 'return', {})
+
+        # Add a throttle filter with the group that we just created.
+        # The filter is not used by anyone yet
+        opts = { 'driver': 'throttle', 'node-name': 'throttle0',
+                 'throttle-group': 'group0', 'file': 'hd0-file' }
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+        self.assert_qmp(result, 'return', {})
+
+        # Insert the throttle filter between hd0 and hd0-file
+        self.reopen(hd0_opts, {'file': 'throttle0'})
+
+        # Remove the throttle filter from hd0
+        self.reopen(hd0_opts, {'file': 'hd0-file'})
+
+    # Insert (and remove) a compress filter
+    def test_insert_compress_filter(self):
+        # Add an image to the VM: hd (raw) -> hd0 (qcow2) -> hd0-file (file)
+        opts = {'driver': 'raw', 'node-name': 'hd', 'file': hd_opts(0)}
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+        self.assert_qmp(result, 'return', {})
+
+        # Add a 'compress' filter
+        filter_opts = {'driver': 'compress',
+                       'node-name': 'compress0',
+                       'file': 'hd0'}
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **filter_opts)
+        self.assert_qmp(result, 'return', {})
+
+        # Unmap the beginning of the image (we cannot write compressed
+        # data to an allocated cluster)
+        self.run_qemu_io("hd", "write -z -u 0 128k")
+
+        # Write data to the first cluster
+        self.run_qemu_io("hd", "write -P 0xa0 0 64k")
+
+        # Insert the filter then write to the second cluster
+        # hd -> compress0 -> hd0 -> hd0-file
+        self.reopen(opts, {'file': 'compress0'})
+        self.run_qemu_io("hd", "write -P 0xa1 64k 64k")
+
+        # Remove the filter then write to the third cluster
+        # hd -> hd0 -> hd0-file
+        self.reopen(opts, {'file': 'hd0'})
+        self.run_qemu_io("hd", "write -P 0xa2 128k 64k")
+
+        # Verify the data that we just wrote
+        self.run_qemu_io("hd", "read -P 0xa0    0 64k")
+        self.run_qemu_io("hd", "read -P 0xa1  64k 64k")
+        self.run_qemu_io("hd", "read -P 0xa2 128k 64k")
+
+        self.vm.shutdown()
+
+        # Check the first byte of the first three L2 entries and verify that
+        # the second one is compressed (0x40) while the others are not (0x80)
+        iotests.qemu_io_log('-f', 'raw', '-c', 'read -P 0x80 0x40000 1',
+                                         '-c', 'read -P 0x40 0x40008 1',
+                                         '-c', 'read -P 0x80 0x40010 1', hd_path[0])
+
     # Misc reopen tests with different block drivers
     @iotests.skip_if_unsupported(['quorum', 'throttle'])
     def test_misc_drivers(self):
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 4b33dcaf5c..6ea1b2798f 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -10,8 +10,15 @@
 {"return": {}}
 {"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-.....................
+read 1/1 bytes at offset 262144
+1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1/1 bytes at offset 262152
+1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1/1 bytes at offset 262160
+1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+........................
 ----------------------------------------------------------------------
-Ran 21 tests
+Ran 24 tests
 
 OK
-- 
2.20.1

Re: [PATCH v4 3/6] iotests: Test replacing files with x-blockdev-reopen

[Kevin Wolf]

Am 17.03.2021 um 18:15 hat Alberto Garcia geschrieben:
> This patch adds new tests in which we use x-blockdev-reopen to change
> bs->file
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

Looks good, but I wonder if we should also test the error paths. I think
we're testing them for backing files, but as patch 2 shows, we need to
check different things for update the file link.

Kevin

[PATCH v4 4/6] block: Support multiple reopening with x-blockdev-reopen

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 qapi/block-core.json       | 18 +++++----
 blockdev.c                 | 78 +++++++++++++++++++++++---------------
 tests/qemu-iotests/155     |  9 +++--
 tests/qemu-iotests/165     |  4 +-
 tests/qemu-iotests/245     | 27 +++++++------
 tests/qemu-iotests/248     |  2 +-
 tests/qemu-iotests/248.out |  2 +-
 tests/qemu-iotests/296     |  9 +++--
 tests/qemu-iotests/298     |  4 +-
 9 files changed, 92 insertions(+), 61 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9f555d5c1d..9150f765da 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4181,13 +4181,15 @@
 ##
 # @x-blockdev-reopen:
 #
-# Reopens a block device using the given set of options. Any option
-# not specified will be reset to its default value regardless of its
-# previous status. If an option cannot be changed or a particular
+# Reopens one or more block devices using the given set of options.
+# Any option not specified will be reset to its default value regardless
+# of its previous status. If an option cannot be changed or a particular
 # driver does not support reopening then the command will return an
-# error.
+# error. All devices in the list are reopened in one transaction, so
+# if one of them fails then the whole transaction is cancelled.
 #
-# The top-level @node-name option (from BlockdevOptions) must be
+# The command receives a list of block devices to reopen. For each one
+# of them, the top-level @node-name option (from BlockdevOptions) must be
 # specified and is used to select the block device to be reopened.
 # Other @node-name options must be either omitted or set to the
 # current name of the appropriate node. This command won't change any
@@ -4207,8 +4209,8 @@
 #
 #  4) NULL: the current child (if any) is detached.
 #
-# Options (1) and (2) are supported in all cases, but at the moment
-# only @backing allows replacing or detaching an existing child.
+# Options (1) and (2) are supported in all cases. Option (3) is
+# supported for @file and @backing, and option (4) for @backing only.
 #
 # Unlike with blockdev-add, the @backing option must always be present
 # unless the node being reopened does not have a backing file and its
@@ -4218,7 +4220,7 @@
 # Since: 4.0
 ##
 { 'command': 'x-blockdev-reopen',
-  'data': 'BlockdevOptions', 'boxed': true }
+  'data': { 'options': ['BlockdevOptions'] } }
 
 ##
 # @blockdev-del:
diff --git a/blockdev.c b/blockdev.c
index 825d40aa11..7019397b05 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3580,46 +3580,64 @@ fail:
     visit_free(v);
 }
 
-void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
+void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
 {
-    BlockDriverState *bs;
-    AioContext *ctx;
-    QObject *obj;
-    Visitor *v = qobject_output_visitor_new(&obj);
-    BlockReopenQueue *queue;
-    QDict *qdict;
+    BlockReopenQueue *queue = NULL;
+    GSList *aio_ctxs = NULL;
+    GSList *visitors = NULL;
+    GSList *drained = NULL;
 
-    /* Check for the selected node name */
-    if (!options->has_node_name) {
-        error_setg(errp, "node-name not specified");
-        goto fail;
-    }
+    /* Add each one of the BDS that we want to reopen to the queue */
+    for (; reopen_list != NULL; reopen_list = reopen_list->next) {
+        BlockdevOptions *options = reopen_list->value;
+        BlockDriverState *bs;
+        AioContext *ctx;
+        QObject *obj;
+        Visitor *v;
+        QDict *qdict;
 
-    bs = bdrv_find_node(options->node_name);
-    if (!bs) {
-        error_setg(errp, "Failed to find node with node-name='%s'",
+        /* Check for the selected node name */
+        if (!options->has_node_name) {
+            error_setg(errp, "node-name not specified");
+            goto fail;
+        }
+
+        bs = bdrv_find_node(options->node_name);
+        if (!bs) {
+            error_setg(errp, "Failed to find node with node-name='%s'",
                    options->node_name);
-        goto fail;
+            goto fail;
+        }
+
+        v = qobject_output_visitor_new(&obj);
+        visitors = g_slist_prepend(visitors, v);
+
+        /* Put all options in a QDict and flatten it */
+        visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
+        visit_complete(v, &obj);
+        qdict = qobject_to(QDict, obj);
+
+        qdict_flatten(qdict);
+
+        ctx = bdrv_get_aio_context(bs);
+        if (!g_slist_find(aio_ctxs, ctx)) {
+            aio_ctxs = g_slist_prepend(aio_ctxs, ctx);
+            aio_context_acquire(ctx);
+        }
+        bdrv_subtree_drained_begin(bs);
+        queue = bdrv_reopen_queue(queue, bs, qdict, false);
+        drained = g_slist_prepend(drained, bs);
     }
 
-    /* Put all options in a QDict and flatten it */
-    visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
-    visit_complete(v, &obj);
-    qdict = qobject_to(QDict, obj);
-
-    qdict_flatten(qdict);
-
     /* Perform the reopen operation */
-    ctx = bdrv_get_aio_context(bs);
-    aio_context_acquire(ctx);
-    bdrv_subtree_drained_begin(bs);
-    queue = bdrv_reopen_queue(NULL, bs, qdict, false);
     bdrv_reopen_multiple(queue, errp);
-    bdrv_subtree_drained_end(bs);
-    aio_context_release(ctx);
+    queue = NULL;
 
 fail:
-    visit_free(v);
+    bdrv_reopen_queue_free(queue);
+    g_slist_free_full(drained, (GDestroyNotify) bdrv_subtree_drained_end);
+    g_slist_free_full(aio_ctxs, (GDestroyNotify) aio_context_release);
+    g_slist_free_full(visitors, (GDestroyNotify) visit_free);
 }
 
 void qmp_blockdev_del(const char *node_name, Error **errp)
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index bafef9dd9a..3400b0312a 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -261,9 +261,12 @@ class TestBlockdevMirrorReopen(MirrorBaseClass):
             result = self.vm.qmp('blockdev-add', node_name="backing",
                                  driver="null-co")
             self.assert_qmp(result, 'return', {})
-            result = self.vm.qmp('x-blockdev-reopen', node_name="target",
-                                 driver=iotests.imgfmt, file="target-file",
-                                 backing="backing")
+            result = self.vm.qmp('x-blockdev-reopen', options = [{
+                                     'node-name': "target",
+                                     'driver': iotests.imgfmt,
+                                     'file': "target-file",
+                                     'backing': "backing"
+                                 }])
             self.assert_qmp(result, 'return', {})
 
 class TestBlockdevMirrorReopenIothread(TestBlockdevMirrorReopen):
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index abc4ffadd5..57aa88ecae 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -137,7 +137,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
         assert sha256_1 == self.getSha256()
 
         # Reopen to RW
-        result = self.vm.qmp('x-blockdev-reopen', **{
+        result = self.vm.qmp('x-blockdev-reopen', options = [{
             'node-name': 'node0',
             'driver': iotests.imgfmt,
             'file': {
@@ -145,7 +145,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
                 'filename': disk
             },
             'read-only': False
-        })
+        }])
         self.assert_qmp(result, 'return', {})
 
         # Check that bitmap is reopened to RW and we can write to it.
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index caebef4ac8..c27557fa6b 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -85,8 +85,18 @@ class TestBlockdevReopen(iotests.QMPTestCase):
                          "Expected output of %d qemu-io commands, found %d" %
                          (found, self.total_io_cmds))
 
-    # Run x-blockdev-reopen with 'opts' but applying 'newopts'
-    # on top of it. The original 'opts' dict is unmodified
+    # Run x-blockdev-reopen on a list of block devices
+    def reopenMultiple(self, opts, errmsg = None):
+        result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options = opts)
+        if errmsg:
+            self.assert_qmp(result, 'error/class', 'GenericError')
+            self.assert_qmp(result, 'error/desc', errmsg)
+        else:
+            self.assert_qmp(result, 'return', {})
+
+    # Run x-blockdev-reopen on a single block device (specified by
+    # 'opts') but applying 'newopts' on top of it. The original 'opts'
+    # dict is unmodified
     def reopen(self, opts, newopts = {}, errmsg = None):
         opts = copy.deepcopy(opts)
 
@@ -101,12 +111,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
                 subdict = opts[prefix]
             subdict[key] = value
 
-        result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, **opts)
-        if errmsg:
-            self.assert_qmp(result, 'error/class', 'GenericError')
-            self.assert_qmp(result, 'error/desc', errmsg)
-        else:
-            self.assert_qmp(result, 'return', {})
+        self.reopenMultiple([ opts ], errmsg)
 
 
     # Run query-named-block-nodes and return the specified entry
@@ -142,10 +147,10 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         # We cannot change any of these
         self.reopen(opts, {'node-name': 'not-found'}, "Failed to find node with node-name='not-found'")
         self.reopen(opts, {'node-name': ''}, "Failed to find node with node-name=''")
-        self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'node-name', expected: string")
+        self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'options[0].node-name', expected: string")
         self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'")
         self.reopen(opts, {'driver': ''}, "Invalid parameter ''")
-        self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string")
+        self.reopen(opts, {'driver': None}, "Invalid parameter type for 'options[0].driver', expected: string")
         self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'")
         self.reopen(opts, {'file': ''}, "Cannot find device='' nor node-name=''")
         self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef")
@@ -154,7 +159,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         self.reopen(opts, {'file.filename': hd_path[1]}, "Cannot change the option 'filename'")
         self.reopen(opts, {'file.aio': 'native'}, "Cannot change the option 'aio'")
         self.reopen(opts, {'file.locking': 'off'}, "Cannot change the option 'locking'")
-        self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'file.filename', expected: string")
+        self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'options[0].file.filename', expected: string")
 
         # node-name is optional in BlockdevOptions, but x-blockdev-reopen needs it
         del opts['node-name']
diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248
index 4daaed1530..03911333c4 100755
--- a/tests/qemu-iotests/248
+++ b/tests/qemu-iotests/248
@@ -63,7 +63,7 @@ vm.get_qmp_events()
 
 del blockdev_opts['file']['size']
 vm.qmp_log('x-blockdev-reopen', filters=[filter_qmp_testfiles],
-           **blockdev_opts)
+           options = [ blockdev_opts ])
 
 vm.qmp_log('block-job-resume', device='drive0')
 vm.event_wait('JOB_STATUS_CHANGE', timeout=1.0,
diff --git a/tests/qemu-iotests/248.out b/tests/qemu-iotests/248.out
index 369b25bf26..893f625347 100644
--- a/tests/qemu-iotests/248.out
+++ b/tests/qemu-iotests/248.out
@@ -2,7 +2,7 @@
 {"return": {}}
 {"execute": "blockdev-mirror", "arguments": {"device": "drive0", "on-target-error": "enospc", "sync": "full", "target": "target"}}
 {"return": {}}
-{"execute": "x-blockdev-reopen", "arguments": {"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}}
+{"execute": "x-blockdev-reopen", "arguments": {"options": [{"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}]}}
 {"return": {}}
 {"execute": "block-job-resume", "arguments": {"device": "drive0"}}
 {"return": {}}
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index 7c65e987a1..74b74511b6 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -120,8 +120,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
 
         command = 'x-blockdev-reopen' if reOpen else 'blockdev-add'
 
-        result = vm.qmp(command, **
-            {
+        opts = {
                 'driver': iotests.imgfmt,
                 'node-name': id,
                 'read-only': readOnly,
@@ -131,7 +130,11 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
                     'filename': test_img,
                 }
             }
-        )
+
+        if reOpen:
+            result = vm.qmp(command, options = [ opts ])
+        else:
+            result = vm.qmp(command, **opts)
         self.assert_qmp(result, 'return', {})
 
 
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
index d535946b5f..4efdb35b91 100755
--- a/tests/qemu-iotests/298
+++ b/tests/qemu-iotests/298
@@ -98,7 +98,7 @@ class TestPreallocateFilter(TestPreallocateBase):
         self.check_big()
 
     def test_reopen_opts(self):
-        result = self.vm.qmp('x-blockdev-reopen', **{
+        result = self.vm.qmp('x-blockdev-reopen', options = [{
             'node-name': 'disk',
             'driver': iotests.imgfmt,
             'file': {
@@ -112,7 +112,7 @@ class TestPreallocateFilter(TestPreallocateBase):
                     'filename': disk
                 }
             }
-        })
+        }])
         self.assert_qmp(result, 'return', {})
 
         self.vm.hmp_qemu_io('drive0', 'write 0 1M')
-- 
2.20.1

Re: [PATCH v4 4/6] block: Support multiple reopening with x-blockdev-reopen

[Vladimir Sementsov-Ogievskiy]

17.03.2021 20:15, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   qapi/block-core.json       | 18 +++++----
>   blockdev.c                 | 78 +++++++++++++++++++++++---------------
>   tests/qemu-iotests/155     |  9 +++--
>   tests/qemu-iotests/165     |  4 +-
>   tests/qemu-iotests/245     | 27 +++++++------
>   tests/qemu-iotests/248     |  2 +-
>   tests/qemu-iotests/248.out |  2 +-
>   tests/qemu-iotests/296     |  9 +++--
>   tests/qemu-iotests/298     |  4 +-
>   9 files changed, 92 insertions(+), 61 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9f555d5c1d..9150f765da 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4181,13 +4181,15 @@
>   ##
>   # @x-blockdev-reopen:
>   #
> -# Reopens a block device using the given set of options. Any option
> -# not specified will be reset to its default value regardless of its
> -# previous status. If an option cannot be changed or a particular
> +# Reopens one or more block devices using the given set of options.
> +# Any option not specified will be reset to its default value regardless
> +# of its previous status. If an option cannot be changed or a particular
>   # driver does not support reopening then the command will return an
> -# error.
> +# error. All devices in the list are reopened in one transaction, so
> +# if one of them fails then the whole transaction is cancelled.
>   #
> -# The top-level @node-name option (from BlockdevOptions) must be
> +# The command receives a list of block devices to reopen. For each one
> +# of them, the top-level @node-name option (from BlockdevOptions) must be
>   # specified and is used to select the block device to be reopened.
>   # Other @node-name options must be either omitted or set to the
>   # current name of the appropriate node. This command won't change any
> @@ -4207,8 +4209,8 @@
>   #
>   #  4) NULL: the current child (if any) is detached.
>   #
> -# Options (1) and (2) are supported in all cases, but at the moment
> -# only @backing allows replacing or detaching an existing child.
> +# Options (1) and (2) are supported in all cases. Option (3) is
> +# supported for @file and @backing, and option (4) for @backing only.

A bit of it should be already updated in "[PATCH v4 2/6] block: Allow changing bs->file on reopen"

>   #
>   # Unlike with blockdev-add, the @backing option must always be present
>   # unless the node being reopened does not have a backing file and its
> @@ -4218,7 +4220,7 @@
>   # Since: 4.0
>   ##
>   { 'command': 'x-blockdev-reopen',
> -  'data': 'BlockdevOptions', 'boxed': true }
> +  'data': { 'options': ['BlockdevOptions'] } }
>   
>   ##
>   # @blockdev-del:
> diff --git a/blockdev.c b/blockdev.c
> index 825d40aa11..7019397b05 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3580,46 +3580,64 @@ fail:
>       visit_free(v);
>   }
>   
> -void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
> +void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
>   {
> -    BlockDriverState *bs;
> -    AioContext *ctx;
> -    QObject *obj;
> -    Visitor *v = qobject_output_visitor_new(&obj);
> -    BlockReopenQueue *queue;
> -    QDict *qdict;
> +    BlockReopenQueue *queue = NULL;
> +    GSList *aio_ctxs = NULL;
> +    GSList *visitors = NULL;
> +    GSList *drained = NULL;
>   
> -    /* Check for the selected node name */
> -    if (!options->has_node_name) {
> -        error_setg(errp, "node-name not specified");
> -        goto fail;
> -    }
> +    /* Add each one of the BDS that we want to reopen to the queue */
> +    for (; reopen_list != NULL; reopen_list = reopen_list->next) {
> +        BlockdevOptions *options = reopen_list->value;
> +        BlockDriverState *bs;
> +        AioContext *ctx;
> +        QObject *obj;
> +        Visitor *v;
> +        QDict *qdict;
>   
> -    bs = bdrv_find_node(options->node_name);
> -    if (!bs) {
> -        error_setg(errp, "Failed to find node with node-name='%s'",
> +        /* Check for the selected node name */
> +        if (!options->has_node_name) {
> +            error_setg(errp, "node-name not specified");
> +            goto fail;
> +        }
> +
> +        bs = bdrv_find_node(options->node_name);
> +        if (!bs) {
> +            error_setg(errp, "Failed to find node with node-name='%s'",
>                      options->node_name);
> -        goto fail;
> +            goto fail;
> +        }
> +
> +        v = qobject_output_visitor_new(&obj);
> +        visitors = g_slist_prepend(visitors, v);

I'd better just call visit_free inside the block instead of putting v to list be freed later after the block..

> +
> +        /* Put all options in a QDict and flatten it */
> +        visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
> +        visit_complete(v, &obj);
> +        qdict = qobject_to(QDict, obj);
> +
> +        qdict_flatten(qdict);
> +
> +        ctx = bdrv_get_aio_context(bs);
> +        if (!g_slist_find(aio_ctxs, ctx)) {
> +            aio_ctxs = g_slist_prepend(aio_ctxs, ctx);
> +            aio_context_acquire(ctx);
> +        }
> +        bdrv_subtree_drained_begin(bs);

I expect Kevin will complain that aquiring several context and drain them all is a bad idea as it leads to deadlocks..
For more information look at the branches
   [PATCH v2 29/36] blockdev: qmp_x_blockdev_reopen: acquire all contexts
amd
   [PATCH v2 30/36] block: bdrv_reopen_multiple: refresh permissions on updated graph

So, probably here we should acquire context in a loop to call bdrv_reopen_queue() (which I think shoud not require drained section).

And then, bdrv_reopen_multiple() is called with no aio context acquired, and no drained section.. And it shoud be refactored to properly operate with acquiring and realeasing the contexts and drained sections when needed...

(note preexisting problem of reopen, that during reopen the whole tree may be moved to another aio context, but we continue operations with acquired old aio context which is wrong).


> +        queue = bdrv_reopen_queue(queue, bs, qdict, false);
> +        drained = g_slist_prepend(drained, bs);
>       }
>   
> -    /* Put all options in a QDict and flatten it */
> -    visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
> -    visit_complete(v, &obj);
> -    qdict = qobject_to(QDict, obj);
> -
> -    qdict_flatten(qdict);
> -
>       /* Perform the reopen operation */
> -    ctx = bdrv_get_aio_context(bs);
> -    aio_context_acquire(ctx);
> -    bdrv_subtree_drained_begin(bs);
> -    queue = bdrv_reopen_queue(NULL, bs, qdict, false);
>       bdrv_reopen_multiple(queue, errp);
> -    bdrv_subtree_drained_end(bs);
> -    aio_context_release(ctx);
> +    queue = NULL;
>   
>   fail:
> -    visit_free(v);
> +    bdrv_reopen_queue_free(queue);
> +    g_slist_free_full(drained, (GDestroyNotify) bdrv_subtree_drained_end);
> +    g_slist_free_full(aio_ctxs, (GDestroyNotify) aio_context_release);
> +    g_slist_free_full(visitors, (GDestroyNotify) visit_free);
>   }
>   


-- 
Best regards,
Vladimir

Re: [PATCH v4 4/6] block: Support multiple reopening with x-blockdev-reopen

[Kevin Wolf]

Am 18.03.2021 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 17.03.2021 20:15, Alberto Garcia wrote:
> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> > ---
> >   qapi/block-core.json       | 18 +++++----
> >   blockdev.c                 | 78 +++++++++++++++++++++++---------------
> >   tests/qemu-iotests/155     |  9 +++--
> >   tests/qemu-iotests/165     |  4 +-
> >   tests/qemu-iotests/245     | 27 +++++++------
> >   tests/qemu-iotests/248     |  2 +-
> >   tests/qemu-iotests/248.out |  2 +-
> >   tests/qemu-iotests/296     |  9 +++--
> >   tests/qemu-iotests/298     |  4 +-
> >   9 files changed, 92 insertions(+), 61 deletions(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 9f555d5c1d..9150f765da 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4181,13 +4181,15 @@
> >   ##
> >   # @x-blockdev-reopen:
> >   #
> > -# Reopens a block device using the given set of options. Any option
> > -# not specified will be reset to its default value regardless of its
> > -# previous status. If an option cannot be changed or a particular
> > +# Reopens one or more block devices using the given set of options.
> > +# Any option not specified will be reset to its default value regardless
> > +# of its previous status. If an option cannot be changed or a particular
> >   # driver does not support reopening then the command will return an
> > -# error.
> > +# error. All devices in the list are reopened in one transaction, so
> > +# if one of them fails then the whole transaction is cancelled.
> >   #
> > -# The top-level @node-name option (from BlockdevOptions) must be
> > +# The command receives a list of block devices to reopen. For each one
> > +# of them, the top-level @node-name option (from BlockdevOptions) must be
> >   # specified and is used to select the block device to be reopened.
> >   # Other @node-name options must be either omitted or set to the
> >   # current name of the appropriate node. This command won't change any
> > @@ -4207,8 +4209,8 @@
> >   #
> >   #  4) NULL: the current child (if any) is detached.
> >   #
> > -# Options (1) and (2) are supported in all cases, but at the moment
> > -# only @backing allows replacing or detaching an existing child.
> > +# Options (1) and (2) are supported in all cases. Option (3) is
> > +# supported for @file and @backing, and option (4) for @backing only.
> 
> A bit of it should be already updated in "[PATCH v4 2/6] block: Allow
> changing bs->file on reopen"
> 
> >   #
> >   # Unlike with blockdev-add, the @backing option must always be present
> >   # unless the node being reopened does not have a backing file and its
> > @@ -4218,7 +4220,7 @@
> >   # Since: 4.0
> >   ##
> >   { 'command': 'x-blockdev-reopen',
> > -  'data': 'BlockdevOptions', 'boxed': true }
> > +  'data': { 'options': ['BlockdevOptions'] } }
> >   ##
> >   # @blockdev-del:
> > diff --git a/blockdev.c b/blockdev.c
> > index 825d40aa11..7019397b05 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3580,46 +3580,64 @@ fail:
> >       visit_free(v);
> >   }
> > -void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
> > +void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
> >   {
> > -    BlockDriverState *bs;
> > -    AioContext *ctx;
> > -    QObject *obj;
> > -    Visitor *v = qobject_output_visitor_new(&obj);
> > -    BlockReopenQueue *queue;
> > -    QDict *qdict;
> > +    BlockReopenQueue *queue = NULL;
> > +    GSList *aio_ctxs = NULL;
> > +    GSList *visitors = NULL;
> > +    GSList *drained = NULL;
> > -    /* Check for the selected node name */
> > -    if (!options->has_node_name) {
> > -        error_setg(errp, "node-name not specified");
> > -        goto fail;
> > -    }
> > +    /* Add each one of the BDS that we want to reopen to the queue */
> > +    for (; reopen_list != NULL; reopen_list = reopen_list->next) {
> > +        BlockdevOptions *options = reopen_list->value;
> > +        BlockDriverState *bs;
> > +        AioContext *ctx;
> > +        QObject *obj;
> > +        Visitor *v;
> > +        QDict *qdict;
> > -    bs = bdrv_find_node(options->node_name);
> > -    if (!bs) {
> > -        error_setg(errp, "Failed to find node with node-name='%s'",
> > +        /* Check for the selected node name */
> > +        if (!options->has_node_name) {
> > +            error_setg(errp, "node-name not specified");
> > +            goto fail;
> > +        }
> > +
> > +        bs = bdrv_find_node(options->node_name);
> > +        if (!bs) {
> > +            error_setg(errp, "Failed to find node with node-name='%s'",
> >                      options->node_name);
> > -        goto fail;
> > +            goto fail;
> > +        }
> > +
> > +        v = qobject_output_visitor_new(&obj);
> > +        visitors = g_slist_prepend(visitors, v);
> 
> I'd better just call visit_free inside the block instead of putting v
> to list be freed later after the block..

Yes, I had the same thought.

> > +
> > +        /* Put all options in a QDict and flatten it */
> > +        visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
> > +        visit_complete(v, &obj);
> > +        qdict = qobject_to(QDict, obj);
> > +
> > +        qdict_flatten(qdict);
> > +
> > +        ctx = bdrv_get_aio_context(bs);
> > +        if (!g_slist_find(aio_ctxs, ctx)) {
> > +            aio_ctxs = g_slist_prepend(aio_ctxs, ctx);
> > +            aio_context_acquire(ctx);
> > +        }
> > +        bdrv_subtree_drained_begin(bs);
> 
> I expect Kevin will complain that aquiring several context and drain
> them all is a bad idea as it leads to deadlocks..

No need for me to complain when you already complained. :-)

> For more information look at the branches
>   [PATCH v2 29/36] blockdev: qmp_x_blockdev_reopen: acquire all contexts
> amd
>   [PATCH v2 30/36] block: bdrv_reopen_multiple: refresh permissions on updated graph
> 
> So, probably here we should acquire context in a loop to call
> bdrv_reopen_queue() (which I think shoud not require drained section).

I think the aio_context_acquire() is right (it is needed for drain) and
we can even keep it across bdrv_reopen_queue() if we want (though there
is probably no reason to do so), but we need to release it again after
the loop iteration (i.e. after bdrv_reopen_queue) so that we won't cause
deadlocks in the next loop iteration.

> And then, bdrv_reopen_multiple() is called with no aio context
> acquired, and no drained section.. And it shoud be refactored to
> properly operate with acquiring and realeasing the contexts and
> drained sections when needed...

The drained section shouldn't be a problem, we can keep this. In fact,
we need this, it is a documented requirement of bdrv_reopen_multiple().
We just need to drop the AioContext lock after drained_begin.

bdrv_reopen_multiple() should really document its requirements regarding
AioContext locks. It probably doesn't really care, but requires that
it be called from the main thread.

> (note preexisting problem of reopen, that during reopen the whole tree
> may be moved to another aio context, but we continue operations with
> acquired old aio context which is wrong).

Do we even acquire the old AioContext?

> > +        queue = bdrv_reopen_queue(queue, bs, qdict, false);
> > +        drained = g_slist_prepend(drained, bs);
> >       }
> > -    /* Put all options in a QDict and flatten it */
> > -    visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
> > -    visit_complete(v, &obj);
> > -    qdict = qobject_to(QDict, obj);
> > -
> > -    qdict_flatten(qdict);
> > -
> >       /* Perform the reopen operation */
> > -    ctx = bdrv_get_aio_context(bs);
> > -    aio_context_acquire(ctx);
> > -    bdrv_subtree_drained_begin(bs);
> > -    queue = bdrv_reopen_queue(NULL, bs, qdict, false);
> >       bdrv_reopen_multiple(queue, errp);
> > -    bdrv_subtree_drained_end(bs);
> > -    aio_context_release(ctx);
> > +    queue = NULL;
> >   fail:
> > -    visit_free(v);
> > +    bdrv_reopen_queue_free(queue);

This has the same leak that we just fixed in Vladimir's series:

        qobject_unref(bs_entry->state.explicit_options);
        qobject_unref(bs_entry->state.options);

This part from abort in bdrv_reopen_multiple() is required even before
calling prepare, these QDicts are created in bdrv_reopen_queue().

We can't just move it unconditionally to bdrv_reopen_queue_free(),
though, because commit makes use of them and then we can't just unref
them. Either commit needs to take an extra reference or we'd need a bool
parameter in bdrv_reopen_queue_free(). I guess taking the extra
reference is cleanest.

> > +    g_slist_free_full(drained, (GDestroyNotify) bdrv_subtree_drained_end);
> > +    g_slist_free_full(aio_ctxs, (GDestroyNotify) aio_context_release);
> > +    g_slist_free_full(visitors, (GDestroyNotify) visit_free);
> >   }

Kevin

Re: [PATCH v4 4/6] block: Support multiple reopening with x-blockdev-reopen

[Vladimir Sementsov-Ogievskiy]

05.05.2021 19:18, Kevin Wolf wrote:
>> And then, bdrv_reopen_multiple() is called with no aio context
>> acquired, and no drained section.. And it shoud be refactored to
>> properly operate with acquiring and realeasing the contexts and
>> drained sections when needed...
> The drained section shouldn't be a problem, we can keep this. In fact,
> we need this, it is a documented requirement of bdrv_reopen_multiple().
> We just need to drop the AioContext lock after drained_begin.
> 
> bdrv_reopen_multiple() should really document its requirements regarding
> AioContext locks. It probably doesn't really care, but requires that
> it be called from the main thread.
> 
>> (note preexisting problem of reopen, that during reopen the whole tree
>> may be moved to another aio context, but we continue operations with
>> acquired old aio context which is wrong).
> Do we even acquire the old AioContext?
> 

I think, I mean aio_context_acquire(ctx);  in current qmp_x_blockdev_reopen()..

And I remember, that you said about that problem. Still, may be I misunderstood (or misremember). Doesn't worth digging, I'd prefer just review v5 with a fresh eye when it comes.


-- 
Best regards,
Vladimir

[PATCH v4 5/6] iotests: Test reopening multiple devices at the same time

[Alberto Garcia]

This test swaps the images used by two active block devices.

This is now possible thanks to the new ability to run
x-blockdev-reopen on multiple devices at the same time.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/245     | 41 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/245.out |  4 ++--
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index c27557fa6b..1a4413e6ab 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -649,6 +649,47 @@ class TestBlockdevReopen(iotests.QMPTestCase):
                                          '-c', 'read -P 0x40 0x40008 1',
                                          '-c', 'read -P 0x80 0x40010 1', hd_path[0])
 
+    # Swap the disk images of two active block devices
+    def test_swap_files(self):
+        # Add hd0 and hd2 (none of them with backing files)
+        opts0 = hd_opts(0)
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **opts0)
+        self.assert_qmp(result, 'return', {})
+
+        opts2 = hd_opts(2)
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **opts2)
+        self.assert_qmp(result, 'return', {})
+
+        # Write different data to both block devices
+        self.run_qemu_io("hd0", "write -P 0xa0 0 1k")
+        self.run_qemu_io("hd2", "write -P 0xa2 0 1k")
+
+        # Check that the data reads correctly
+        self.run_qemu_io("hd0", "read  -P 0xa0 0 1k")
+        self.run_qemu_io("hd2", "read  -P 0xa2 0 1k")
+
+        # It's not possible to make a block device use an image that
+        # is already being used by the other device.
+        self.reopen(opts0, {'file': 'hd2-file'},
+                    "Conflicts with use by hd0 as 'file', which does not allow 'write, resize' on hd2-file")
+        self.reopen(opts2, {'file': 'hd0-file'},
+                    "Conflicts with use by hd2 as 'file', which does not allow 'write, resize' on hd0-file")
+
+        # But we can swap the images if we reopen both devices at the
+        # same time
+        opts0['file'] = 'hd2-file'
+        opts2['file'] = 'hd0-file'
+        self.reopenMultiple([opts0, opts2])
+        self.run_qemu_io("hd0", "read  -P 0xa2 0 1k")
+        self.run_qemu_io("hd2", "read  -P 0xa0 0 1k")
+
+        # And we can of course come back to the original state
+        opts0['file'] = 'hd0-file'
+        opts2['file'] = 'hd2-file'
+        self.reopenMultiple([opts0, opts2])
+        self.run_qemu_io("hd0", "read  -P 0xa0 0 1k")
+        self.run_qemu_io("hd2", "read  -P 0xa2 0 1k")
+
     # Misc reopen tests with different block drivers
     @iotests.skip_if_unsupported(['quorum', 'throttle'])
     def test_misc_drivers(self):
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 6ea1b2798f..c4230b51d1 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -17,8 +17,8 @@ read 1/1 bytes at offset 262152
 read 1/1 bytes at offset 262160
 1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
-........................
+.........................
 ----------------------------------------------------------------------
-Ran 24 tests
+Ran 25 tests
 
 OK
-- 
2.20.1

[PATCH v4 6/6] block: Make blockdev-reopen stable API

[Alberto Garcia]

This patch drops the 'x-' prefix from x-blockdev-reopen.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 qapi/block-core.json       |  6 +++---
 blockdev.c                 |  2 +-
 tests/qemu-iotests/155     |  2 +-
 tests/qemu-iotests/165     |  2 +-
 tests/qemu-iotests/245     | 10 +++++-----
 tests/qemu-iotests/248     |  2 +-
 tests/qemu-iotests/248.out |  2 +-
 tests/qemu-iotests/296     |  2 +-
 tests/qemu-iotests/298     |  2 +-
 9 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9150f765da..93fb0606e9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4179,7 +4179,7 @@
 { 'command': 'blockdev-add', 'data': 'BlockdevOptions', 'boxed': true }
 
 ##
-# @x-blockdev-reopen:
+# @blockdev-reopen:
 #
 # Reopens one or more block devices using the given set of options.
 # Any option not specified will be reset to its default value regardless
@@ -4217,9 +4217,9 @@
 # image does not have a default backing file name as part of its
 # metadata.
 #
-# Since: 4.0
+# Since: 6.0
 ##
-{ 'command': 'x-blockdev-reopen',
+{ 'command': 'blockdev-reopen',
   'data': { 'options': ['BlockdevOptions'] } }
 
 ##
diff --git a/blockdev.c b/blockdev.c
index 7019397b05..d697db8417 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3580,7 +3580,7 @@ fail:
     visit_free(v);
 }
 
-void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
+void qmp_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
 {
     BlockReopenQueue *queue = NULL;
     GSList *aio_ctxs = NULL;
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index 3400b0312a..fec43d662d 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -261,7 +261,7 @@ class TestBlockdevMirrorReopen(MirrorBaseClass):
             result = self.vm.qmp('blockdev-add', node_name="backing",
                                  driver="null-co")
             self.assert_qmp(result, 'return', {})
-            result = self.vm.qmp('x-blockdev-reopen', options = [{
+            result = self.vm.qmp('blockdev-reopen', options = [{
                                      'node-name': "target",
                                      'driver': iotests.imgfmt,
                                      'file': "target-file",
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index 57aa88ecae..92a431315b 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -137,7 +137,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
         assert sha256_1 == self.getSha256()
 
         # Reopen to RW
-        result = self.vm.qmp('x-blockdev-reopen', options = [{
+        result = self.vm.qmp('blockdev-reopen', options = [{
             'node-name': 'node0',
             'driver': iotests.imgfmt,
             'file': {
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 1a4413e6ab..e7498b5b3e 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1,7 +1,7 @@
 #!/usr/bin/env python3
 # group: rw
 #
-# Test cases for the QMP 'x-blockdev-reopen' command
+# Test cases for the QMP 'blockdev-reopen' command
 #
 # Copyright (C) 2018-2019 Igalia, S.L.
 # Author: Alberto Garcia <berto@igalia.com>
@@ -85,16 +85,16 @@ class TestBlockdevReopen(iotests.QMPTestCase):
                          "Expected output of %d qemu-io commands, found %d" %
                          (found, self.total_io_cmds))
 
-    # Run x-blockdev-reopen on a list of block devices
+    # Run blockdev-reopen on a list of block devices
     def reopenMultiple(self, opts, errmsg = None):
-        result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options = opts)
+        result = self.vm.qmp('blockdev-reopen', conv_keys = False, options = opts)
         if errmsg:
             self.assert_qmp(result, 'error/class', 'GenericError')
             self.assert_qmp(result, 'error/desc', errmsg)
         else:
             self.assert_qmp(result, 'return', {})
 
-    # Run x-blockdev-reopen on a single block device (specified by
+    # Run blockdev-reopen on a single block device (specified by
     # 'opts') but applying 'newopts' on top of it. The original 'opts'
     # dict is unmodified
     def reopen(self, opts, newopts = {}, errmsg = None):
@@ -161,7 +161,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         self.reopen(opts, {'file.locking': 'off'}, "Cannot change the option 'locking'")
         self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'options[0].file.filename', expected: string")
 
-        # node-name is optional in BlockdevOptions, but x-blockdev-reopen needs it
+        # node-name is optional in BlockdevOptions, but blockdev-reopen needs it
         del opts['node-name']
         self.reopen(opts, {}, "node-name not specified")
 
diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248
index 03911333c4..2ec2416e8a 100755
--- a/tests/qemu-iotests/248
+++ b/tests/qemu-iotests/248
@@ -62,7 +62,7 @@ vm.event_wait('JOB_STATUS_CHANGE', timeout=3.0,
 vm.get_qmp_events()
 
 del blockdev_opts['file']['size']
-vm.qmp_log('x-blockdev-reopen', filters=[filter_qmp_testfiles],
+vm.qmp_log('blockdev-reopen', filters=[filter_qmp_testfiles],
            options = [ blockdev_opts ])
 
 vm.qmp_log('block-job-resume', device='drive0')
diff --git a/tests/qemu-iotests/248.out b/tests/qemu-iotests/248.out
index 893f625347..66e94ccd7e 100644
--- a/tests/qemu-iotests/248.out
+++ b/tests/qemu-iotests/248.out
@@ -2,7 +2,7 @@
 {"return": {}}
 {"execute": "blockdev-mirror", "arguments": {"device": "drive0", "on-target-error": "enospc", "sync": "full", "target": "target"}}
 {"return": {}}
-{"execute": "x-blockdev-reopen", "arguments": {"options": [{"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}]}}
+{"execute": "blockdev-reopen", "arguments": {"options": [{"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}]}}
 {"return": {}}
 {"execute": "block-job-resume", "arguments": {"device": "drive0"}}
 {"return": {}}
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index 74b74511b6..9206ddb954 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -118,7 +118,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
     def openImageQmp(self, vm, id, file, secret,
                      readOnly = False, reOpen = False):
 
-        command = 'x-blockdev-reopen' if reOpen else 'blockdev-add'
+        command = 'blockdev-reopen' if reOpen else 'blockdev-add'
 
         opts = {
                 'driver': iotests.imgfmt,
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
index 4efdb35b91..b4d8bd9b55 100755
--- a/tests/qemu-iotests/298
+++ b/tests/qemu-iotests/298
@@ -98,7 +98,7 @@ class TestPreallocateFilter(TestPreallocateBase):
         self.check_big()
 
     def test_reopen_opts(self):
-        result = self.vm.qmp('x-blockdev-reopen', options = [{
+        result = self.vm.qmp('blockdev-reopen', options = [{
             'node-name': 'disk',
             'driver': iotests.imgfmt,
             'file': {
-- 
2.20.1

Re: [PATCH v4 0/6] Allow changing bs->file on reopen

[Vladimir Sementsov-Ogievskiy]

Hi Alberto!

What are your plans for v5? I'm now finishing a new series which makes backup-top filter public, and I want to base it on your series (otherwise I can't add a test).

17.03.2021 20:15, Alberto Garcia wrote:
> Based-on: <20210317143529.615584-1-vsementsov@virtuozzo.com>
> 
> Hello,
> 
> this is the same as v3, but rebased on top of Vladimir's "block:
> update graph permissions update v3", which you can get here:
> 
> git: https://src.openvz.org/scm/~vsementsov/qemu.git
> tag: up-block-topologic-perm-v3
> 
> Tip: you may find it easier to review patch 4 if you use 'git diff -w'
> since a big part of the changes that you see in
> qmp_x_blockdev_reopen() are just indentation changes.
> 
> Berto
> 
> v4:
> - Rebase on top of version 3 of Vladimir's branch
> v3: https://lists.gnu.org/archive/html/qemu-block/2021-03/msg00553.html
> v2: https://lists.gnu.org/archive/html/qemu-block/2021-02/msg00623.html
> v1: https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00437.html
> 
> Output of git backport-diff against v3:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/6:[----] [--] 'block: Add bdrv_reopen_queue_free()'
> 002/6:[0018] [FC] 'block: Allow changing bs->file on reopen'
> 003/6:[----] [--] 'iotests: Test replacing files with x-blockdev-reopen'
> 004/6:[0071] [FC] 'block: Support multiple reopening with x-blockdev-reopen'
> 005/6:[----] [--] 'iotests: Test reopening multiple devices at the same time'
> 006/6:[----] [-C] 'block: Make blockdev-reopen stable API'
> 
> Alberto Garcia (6):
>    block: Add bdrv_reopen_queue_free()
>    block: Allow changing bs->file on reopen
>    iotests: Test replacing files with x-blockdev-reopen
>    block: Support multiple reopening with x-blockdev-reopen
>    iotests: Test reopening multiple devices at the same time
>    block: Make blockdev-reopen stable API
> 
>   qapi/block-core.json       |  24 ++---
>   include/block/block.h      |   2 +
>   block.c                    | 135 ++++++++++++++++----------
>   blockdev.c                 |  78 +++++++++------
>   tests/qemu-iotests/155     |   9 +-
>   tests/qemu-iotests/165     |   4 +-
>   tests/qemu-iotests/245     | 190 +++++++++++++++++++++++++++++++++----
>   tests/qemu-iotests/245.out |  11 ++-
>   tests/qemu-iotests/248     |   4 +-
>   tests/qemu-iotests/248.out |   2 +-
>   tests/qemu-iotests/296     |  11 ++-
>   tests/qemu-iotests/298     |   4 +-
>   12 files changed, 351 insertions(+), 123 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 0/6] Allow changing bs->file on reopen

[Kevin Wolf]

Am 14.05.2021 um 17:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi Alberto!
> 
> What are your plans for v5? I'm now finishing a new series which makes
> backup-top filter public, and I want to base it on your series
> (otherwise I can't add a test).

Berto, where are we with this? I see that Vladimir picked up one or two
patches for his series, but I think we still need a v5 at least for the
rest?

If you can't find the time, should someone else pick up all patches?

Kevin

> 17.03.2021 20:15, Alberto Garcia wrote:
> > Based-on: <20210317143529.615584-1-vsementsov@virtuozzo.com>
> > 
> > Hello,
> > 
> > this is the same as v3, but rebased on top of Vladimir's "block:
> > update graph permissions update v3", which you can get here:
> > 
> > git: https://src.openvz.org/scm/~vsementsov/qemu.git
> > tag: up-block-topologic-perm-v3
> > 
> > Tip: you may find it easier to review patch 4 if you use 'git diff -w'
> > since a big part of the changes that you see in
> > qmp_x_blockdev_reopen() are just indentation changes.
> > 
> > Berto
> > 
> > v4:
> > - Rebase on top of version 3 of Vladimir's branch
> > v3: https://lists.gnu.org/archive/html/qemu-block/2021-03/msg00553.html
> > v2: https://lists.gnu.org/archive/html/qemu-block/2021-02/msg00623.html
> > v1: https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00437.html
> > 
> > Output of git backport-diff against v3:
> > 
> > Key:
> > [----] : patches are identical
> > [####] : number of functional differences between upstream/downstream patch
> > [down] : patch is downstream-only
> > The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> > 
> > 001/6:[----] [--] 'block: Add bdrv_reopen_queue_free()'
> > 002/6:[0018] [FC] 'block: Allow changing bs->file on reopen'
> > 003/6:[----] [--] 'iotests: Test replacing files with x-blockdev-reopen'
> > 004/6:[0071] [FC] 'block: Support multiple reopening with x-blockdev-reopen'
> > 005/6:[----] [--] 'iotests: Test reopening multiple devices at the same time'
> > 006/6:[----] [-C] 'block: Make blockdev-reopen stable API'
> > 
> > Alberto Garcia (6):
> >    block: Add bdrv_reopen_queue_free()
> >    block: Allow changing bs->file on reopen
> >    iotests: Test replacing files with x-blockdev-reopen
> >    block: Support multiple reopening with x-blockdev-reopen
> >    iotests: Test reopening multiple devices at the same time
> >    block: Make blockdev-reopen stable API
> > 
> >   qapi/block-core.json       |  24 ++---
> >   include/block/block.h      |   2 +
> >   block.c                    | 135 ++++++++++++++++----------
> >   blockdev.c                 |  78 +++++++++------
> >   tests/qemu-iotests/155     |   9 +-
> >   tests/qemu-iotests/165     |   4 +-
> >   tests/qemu-iotests/245     | 190 +++++++++++++++++++++++++++++++++----
> >   tests/qemu-iotests/245.out |  11 ++-
> >   tests/qemu-iotests/248     |   4 +-
> >   tests/qemu-iotests/248.out |   2 +-
> >   tests/qemu-iotests/296     |  11 ++-
> >   tests/qemu-iotests/298     |   4 +-
> >   12 files changed, 351 insertions(+), 123 deletions(-)
> > 
> 
> 
> -- 
> Best regards,
> Vladimir
> 

Re: [PATCH v4 0/6] Allow changing bs->file on reopen

[Vladimir Sementsov-Ogievskiy]

09.06.2021 18:53, Kevin Wolf wrote:
> Am 14.05.2021 um 17:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Hi Alberto!
>>
>> What are your plans for v5? I'm now finishing a new series which makes
>> backup-top filter public, and I want to base it on your series
>> (otherwise I can't add a test).
> 
> Berto, where are we with this? I see that Vladimir picked up one or two
> patches for his series, but I think we still need a v5 at least for the
> rest?
> 
> If you can't find the time, should someone else pick up all patches?
> 
> Kevin

My "[PATCH v5 0/9] Allow changing bs->file on reopen" supersedes the "subject" part of the series. I think we now should start from taking it. Hmm, and I should check, does it conflict with recently merged block-permission-folloup and with beginning of "[PATCH v4 00/35] block: publish backup-top filter" which is already almost reviewed by Max and should land soon I hope (ohh, seems I should issue v5 for python conflictes).

So, I propose the following plan:

1. I'll rebase and send "block: publish backup-top filter" series today-tomorrow. It's big, and mostly reviewed, let's not lose r-bs by rebases.

2. I'll rebase and send if needed (if it conflicts with master and/or [1]) "[PATCH v5 0/9] Allow changing bs->file on reopen"

3. Then we'll decide what to do with the rest. Finally, I can take it if I have some time (the head is spinning from the number of tasks ;)

I also think that we can drop x- prefix even without supporting of multiple reopen, and implement it later as an option. QAPI interface is powerful enough for such enhancements.

> 
>> 17.03.2021 20:15, Alberto Garcia wrote:
>>> Based-on: <20210317143529.615584-1-vsementsov@virtuozzo.com>
>>>
>>> Hello,
>>>
>>> this is the same as v3, but rebased on top of Vladimir's "block:
>>> update graph permissions update v3", which you can get here:
>>>
>>> git: https://src.openvz.org/scm/~vsementsov/qemu.git
>>> tag: up-block-topologic-perm-v3
>>>
>>> Tip: you may find it easier to review patch 4 if you use 'git diff -w'
>>> since a big part of the changes that you see in
>>> qmp_x_blockdev_reopen() are just indentation changes.
>>>
>>> Berto
>>>
>>> v4:
>>> - Rebase on top of version 3 of Vladimir's branch
>>> v3: https://lists.gnu.org/archive/html/qemu-block/2021-03/msg00553.html
>>> v2: https://lists.gnu.org/archive/html/qemu-block/2021-02/msg00623.html
>>> v1: https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00437.html
>>>
>>> Output of git backport-diff against v3:
>>>
>>> Key:
>>> [----] : patches are identical
>>> [####] : number of functional differences between upstream/downstream patch
>>> [down] : patch is downstream-only
>>> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
>>>
>>> 001/6:[----] [--] 'block: Add bdrv_reopen_queue_free()'
>>> 002/6:[0018] [FC] 'block: Allow changing bs->file on reopen'
>>> 003/6:[----] [--] 'iotests: Test replacing files with x-blockdev-reopen'
>>> 004/6:[0071] [FC] 'block: Support multiple reopening with x-blockdev-reopen'
>>> 005/6:[----] [--] 'iotests: Test reopening multiple devices at the same time'
>>> 006/6:[----] [-C] 'block: Make blockdev-reopen stable API'
>>>
>>> Alberto Garcia (6):
>>>     block: Add bdrv_reopen_queue_free()
>>>     block: Allow changing bs->file on reopen
>>>     iotests: Test replacing files with x-blockdev-reopen
>>>     block: Support multiple reopening with x-blockdev-reopen
>>>     iotests: Test reopening multiple devices at the same time
>>>     block: Make blockdev-reopen stable API
>>>
>>>    qapi/block-core.json       |  24 ++---
>>>    include/block/block.h      |   2 +
>>>    block.c                    | 135 ++++++++++++++++----------
>>>    blockdev.c                 |  78 +++++++++------
>>>    tests/qemu-iotests/155     |   9 +-
>>>    tests/qemu-iotests/165     |   4 +-
>>>    tests/qemu-iotests/245     | 190 +++++++++++++++++++++++++++++++++----
>>>    tests/qemu-iotests/245.out |  11 ++-
>>>    tests/qemu-iotests/248     |   4 +-
>>>    tests/qemu-iotests/248.out |   2 +-
>>>    tests/qemu-iotests/296     |  11 ++-
>>>    tests/qemu-iotests/298     |   4 +-
>>>    12 files changed, 351 insertions(+), 123 deletions(-)
>>>
>>
>>
>> -- 
>> Best regards,
>> Vladimir
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 0/6] Allow changing bs->file on reopen

[Vladimir Sementsov-Ogievskiy]

09.06.2021 19:40, Vladimir Sementsov-Ogievskiy wrote:
> 09.06.2021 18:53, Kevin Wolf wrote:
>> Am 14.05.2021 um 17:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> Hi Alberto!
>>>
>>> What are your plans for v5? I'm now finishing a new series which makes
>>> backup-top filter public, and I want to base it on your series
>>> (otherwise I can't add a test).
>>
>> Berto, where are we with this? I see that Vladimir picked up one or two
>> patches for his series, but I think we still need a v5 at least for the
>> rest?
>>
>> If you can't find the time, should someone else pick up all patches?
>>
>> Kevin
> 
> My "[PATCH v5 0/9] Allow changing bs->file on reopen" supersedes the "subject" part of the series. I think we now should start from taking it. Hmm, and I should check, does it conflict with recently merged block-permission-folloup and with beginning of "[PATCH v4 00/35] block: publish backup-top filter" which is already almost reviewed by Max and should land soon I hope (ohh, seems I should issue v5 for python conflictes).
> 
> So, I propose the following plan:
> 
> 1. I'll rebase and send "block: publish backup-top filter" series today-tomorrow. It's big, and mostly reviewed, let's not lose r-bs by rebases.
> 
> 2. I'll rebase and send if needed (if it conflicts with master and/or [1]) "[PATCH v5 0/9] Allow changing bs->file on reopen"
> 
> 3. Then we'll decide what to do with the rest. Finally, I can take it if I have some time (the head is spinning from the number of tasks ;)
> 
> I also think that we can drop x- prefix even without supporting of multiple reopen, and implement it later as an option. QAPI interface is powerful enough for such enhancements.
> 

[1] and [2] done, patches sent. Finally, "[PATCH v6 0/9] Allow changing bs->file on reopen" depends only on two first simple patches of "[PATCH v5 00/35] block: publish backup-top filter", so the series may go in parallel.

-- 
Best regards,
Vladimir