[RFC PATCH v2 0/4] Allow changing bs->file on reopen

[RFC PATCH v2 0/4] Allow changing bs->file on reopen

[Alberto Garcia]

Hi,

this series allows changing bs->file using x-blockdev-reopen. Read
here for more details:

   https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00437.html

Version 2 of the series introduces a very significant change:
x-blockdev-reopen now receives a list of BlockdevOptions instead of
just one, so it is possible to reopen multiple block devices using a
single transaction.

This is still an RFC, I haven't updated the documentation and the
structure of the patches will probably change in the future, but I'd
like to know your opinion about the approach.

These patches apply on top of Vladimir's branch:

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

Regards,

Berto

Alberto Garcia (4):
  block: Allow changing bs->file on reopen
  iotests: Update 245 to support replacing files with x-blockdev-reopen
  block: Support multiple reopening with x-blockdev-reopen
  iotests: Test reopening multiple devices at the same time

 qapi/block-core.json       |   2 +-
 include/block/block.h      |   2 +
 block.c                    |  81 +++++++++++++++++++++--
 blockdev.c                 |  85 +++++++++++++-----------
 tests/qemu-iotests/155     |   9 ++-
 tests/qemu-iotests/165     |   4 +-
 tests/qemu-iotests/245     | 128 ++++++++++++++++++++++++++++++++-----
 tests/qemu-iotests/245.out |   4 +-
 tests/qemu-iotests/248     |   2 +-
 tests/qemu-iotests/248.out |   2 +-
 tests/qemu-iotests/298     |   4 +-
 11 files changed, 254 insertions(+), 69 deletions(-)

-- 
2.20.1

[RFC PATCH v2 1/4] 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                | 65 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/245 |  7 +++--
 3 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 82271d9ccd..6dd687a69e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -196,6 +196,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 */
     uint64_t perm, shared_perm;
     QDict *options;
     QDict *explicit_options;
diff --git a/block.c b/block.c
index 576b145cbf..19b62da4af 100644
--- a/block.c
+++ b/block.c
@@ -3978,6 +3978,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);
+        }
     }
 
     ret = bdrv_list_refresh_perms(refresh_list, bs_queue, &tran, errp);
@@ -4196,6 +4200,61 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
     return 0;
 }
 
+static int bdrv_reopen_parse_file(BDRVReopenState *reopen_state,
+                                  GSList **tran,
+                                  Error **errp)
+{
+    BlockDriverState *bs = reopen_state->bs;
+    BlockDriverState *new_file_bs;
+    QObject *value;
+    const char *str;
+
+    value = qdict_get(reopen_state->options, "file");
+    if (value == NULL) {
+        return 0;
+    }
+
+    /* The 'file' option only allows strings */
+    assert(qobject_type(value) == QTYPE_QSTRING);
+
+    str = qobject_get_try_str(value);
+    new_file_bs = bdrv_lookup_bs(NULL, str, errp);
+    if (new_file_bs == NULL) {
+        return -EINVAL;
+    } else if (bdrv_recurse_has_child(new_file_bs, bs)) {
+        error_setg(errp, "Making '%s' a file of '%s' "
+                   "would create a cycle", str, bs->node_name);
+        return -EINVAL;
+    }
+
+    assert(bs->file && bs->file->bs);
+
+    /* If 'file' points to the current child then there's nothing to do */
+    if (bs->file->bs == new_file_bs) {
+        return 0;
+    }
+
+    if (bs->file->frozen) {
+        error_setg(errp, "Cannot change the 'file' link of '%s' "
+                   "from '%s' to '%s'", bs->node_name,
+                   bs->file->bs->node_name, new_file_bs->node_name);
+        return -EPERM;
+    }
+
+    /* Check AioContext compatibility */
+    if (!bdrv_reopen_can_attach(bs, bs->file, new_file_bs, errp)) {
+        return -EINVAL;
+    }
+
+    /* Store the old file bs because 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_file_bs, tran);
+
+    return 0;
+}
+
 /*
  * Prepares a BlockDriverState for reopen. All changes are staged in the
  * 'opaque' field of the BDRVReopenState, which is used and allocated by
@@ -4347,6 +4406,12 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
     }
     qdict_del(reopen_state->options, "backing");
 
+    ret = bdrv_reopen_parse_file(reopen_state, set_backings_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 e60c8326d3..f9d68b3958 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -145,8 +145,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'")
@@ -454,7 +454,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')
-- 
2.20.1

Re: [RFC PATCH v2 1/4] block: Allow changing bs->file on reopen

[Vladimir Sementsov-Ogievskiy]

08.02.2021 21:44, 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>
> ---
>   include/block/block.h  |  1 +
>   block.c                | 65 ++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/245 |  7 +++--
>   3 files changed, 70 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 82271d9ccd..6dd687a69e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -196,6 +196,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 */
>       uint64_t perm, shared_perm;
>       QDict *options;
>       QDict *explicit_options;
> diff --git a/block.c b/block.c
> index 576b145cbf..19b62da4af 100644
> --- a/block.c
> +++ b/block.c
> @@ -3978,6 +3978,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);
> +        }
>       }
>   
>       ret = bdrv_list_refresh_perms(refresh_list, bs_queue, &tran, errp);
> @@ -4196,6 +4200,61 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
>       return 0;
>   }
>   
> +static int bdrv_reopen_parse_file(BDRVReopenState *reopen_state,
> +                                  GSList **tran,
> +                                  Error **errp)
> +{
> +    BlockDriverState *bs = reopen_state->bs;
> +    BlockDriverState *new_file_bs;
> +    QObject *value;
> +    const char *str;
> +
> +    value = qdict_get(reopen_state->options, "file");
> +    if (value == NULL) {
> +        return 0;
> +    }
> +
> +    /* The 'file' option only allows strings */
> +    assert(qobject_type(value) == QTYPE_QSTRING);
> +
> +    str = qobject_get_try_str(value);
> +    new_file_bs = bdrv_lookup_bs(NULL, str, errp);
> +    if (new_file_bs == NULL) {
> +        return -EINVAL;
> +    } else if (bdrv_recurse_has_child(new_file_bs, bs)) {
> +        error_setg(errp, "Making '%s' a file of '%s' "
> +                   "would create a cycle", str, bs->node_name);
> +        return -EINVAL;
> +    }
> +
> +    assert(bs->file && bs->file->bs);
> +
> +    /* If 'file' points to the current child then there's nothing to do */
> +    if (bs->file->bs == new_file_bs) {
> +        return 0;
> +    }
> +
> +    if (bs->file->frozen) {
> +        error_setg(errp, "Cannot change the 'file' link of '%s' "
> +                   "from '%s' to '%s'", bs->node_name,
> +                   bs->file->bs->node_name, new_file_bs->node_name);
> +        return -EPERM;
> +    }
> +
> +    /* Check AioContext compatibility */
> +    if (!bdrv_reopen_can_attach(bs, bs->file, new_file_bs, errp)) {
> +        return -EINVAL;
> +    }
> +
> +    /* Store the old file bs because 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_file_bs, tran);
> +
> +    return 0;
> +}

The function mostly do the same that bdrv_reopen_parse_backing().. I don't think that they
should really differ. Probably it should be one function.
At least, they should work absolutely the same way for backing-child
based and file-child based filters. And you lose bdrv_is_backing_chain_frozen() check

> +
>   /*
>    * Prepares a BlockDriverState for reopen. All changes are staged in the
>    * 'opaque' field of the BDRVReopenState, which is used and allocated by
> @@ -4347,6 +4406,12 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>       }
>       qdict_del(reopen_state->options, "backing");
>   
> +    ret = bdrv_reopen_parse_file(reopen_state, set_backings_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 e60c8326d3..f9d68b3958 100755
> --- a/tests/qemu-iotests/245
> +++ b/tests/qemu-iotests/245
> @@ -145,8 +145,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'")
> @@ -454,7 +454,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')
> 


-- 
Best regards,
Vladimir

Re: [RFC PATCH v2 1/4] block: Allow changing bs->file on reopen

[Kevin Wolf]

Am 08.02.2021 um 19:44 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>
> ---
>  include/block/block.h  |  1 +
>  block.c                | 65 ++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/245 |  7 +++--
>  3 files changed, 70 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 82271d9ccd..6dd687a69e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -196,6 +196,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 */
>      uint64_t perm, shared_perm;
>      QDict *options;
>      QDict *explicit_options;
> diff --git a/block.c b/block.c
> index 576b145cbf..19b62da4af 100644
> --- a/block.c
> +++ b/block.c
> @@ -3978,6 +3978,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);
> +        }
>      }
>  
>      ret = bdrv_list_refresh_perms(refresh_list, bs_queue, &tran, errp);
> @@ -4196,6 +4200,61 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
>      return 0;
>  }
>  
> +static int bdrv_reopen_parse_file(BDRVReopenState *reopen_state,
> +                                  GSList **tran,
> +                                  Error **errp)
> +{
> +    BlockDriverState *bs = reopen_state->bs;
> +    BlockDriverState *new_file_bs;
> +    QObject *value;
> +    const char *str;
> +
> +    value = qdict_get(reopen_state->options, "file");
> +    if (value == NULL) {
> +        return 0;
> +    }
> +
> +    /* The 'file' option only allows strings */
> +    assert(qobject_type(value) == QTYPE_QSTRING);

This is true, but not entirely obvious: The QAPI schema has BlockdevRef,
which can be either a string or a dict. However, we're dealing with a
flattened options dict here, so no more nested dicts.

qemu-io doesn't go through the schema, but its parser represents all
scalars as strings, so it's correct even in this case.

> +
> +    str = qobject_get_try_str(value);

This function doesn't exist in master any more, but we already know that
we have a string here, so it's easy enough to replace:

str = qstring_get_str(qobject_to(QString, value));

> +    new_file_bs = bdrv_lookup_bs(NULL, str, errp);
> +    if (new_file_bs == NULL) {
> +        return -EINVAL;
> +    } else if (bdrv_recurse_has_child(new_file_bs, bs)) {
> +        error_setg(errp, "Making '%s' a file of '%s' "
> +                   "would create a cycle", str, bs->node_name);
> +        return -EINVAL;
> +    }
> +
> +    assert(bs->file && bs->file->bs);
> +
> +    /* If 'file' points to the current child then there's nothing to do */
> +    if (bs->file->bs == new_file_bs) {
> +        return 0;
> +    }
> +
> +    if (bs->file->frozen) {
> +        error_setg(errp, "Cannot change the 'file' link of '%s' "
> +                   "from '%s' to '%s'", bs->node_name,
> +                   bs->file->bs->node_name, new_file_bs->node_name);
> +        return -EPERM;
> +    }
> +
> +    /* Check AioContext compatibility */
> +    if (!bdrv_reopen_can_attach(bs, bs->file, new_file_bs, errp)) {
> +        return -EINVAL;
> +    }
> +
> +    /* Store the old file bs because 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_file_bs, tran);
> +
> +    return 0;
> +}

As Vladimir said, it would be nice to avoid some duplication with the
backing file switching code (especially when you consider that we might
get more of these cases, think of qcow2 data files or VMDK extents), but
generally this patch makes sense to me.

Kevin

Re: [RFC PATCH v2 1/4] block: Allow changing bs->file on reopen

[Alberto Garcia]

On Wed 10 Feb 2021 05:52:47 PM CET, Kevin Wolf wrote:
>> +    /* The 'file' option only allows strings */
>> +    assert(qobject_type(value) == QTYPE_QSTRING);
>
> This is true, but not entirely obvious: The QAPI schema has
> BlockdevRef, which can be either a string or a dict. However, we're
> dealing with a flattened options dict here, so no more nested dicts.

You're right, I'll update the comment.

Berto

[RFC PATCH v2 2/4] iotests: Update 245 to support replacing files with x-blockdev-reopen

[Alberto Garcia]

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

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index f9d68b3958..bad6911f0c 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -78,7 +78,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" %
@@ -536,6 +536,58 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'bv')
         self.assert_qmp(result, 'return', {})
 
+    def test_replace_file(self):
+        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] }
+
+        opts = {'driver': 'raw', 'node-name': 'hd', 'file': 'hd0-file'}
+
+        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', {})
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+        self.assert_qmp(result, 'return', {})
+
+        self.run_qemu_io("hd", "read  -P 0 0 10k")
+        self.run_qemu_io("hd", "write -P 0xa0 0 10k")
+
+        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")
+
+        self.reopen(opts, {'file': 'hd0-file'})
+        self.run_qemu_io("hd", "read  -P 0xa0 0 10k")
+
+        self.reopen(opts, {'file': 'hd1-file'})
+        self.run_qemu_io("hd", "read  -P 0xa1 0 10k")
+
+    def test_insert_throttle_filter(self):
+        hd0_opts = hd_opts(0)
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts)
+        self.assert_qmp(result, 'return', {})
+
+        opts = { 'qom-type': 'throttle-group', 'id': 'group0',
+                 'props': { 'limits': { 'iops-total': 1000 } } }
+        result = self.vm.qmp('object-add', conv_keys = False, **opts)
+        self.assert_qmp(result, 'return', {})
+
+        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', {})
+
+        self.reopen(hd0_opts, {'file': 'throttle0'})
+
+        self.reopen(hd0_opts, {'file': 'hd0-file'})
+
     # 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..537a2b5b63 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -10,8 +10,8 @@
 {"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"}}
-.....................
+.......................
 ----------------------------------------------------------------------
-Ran 21 tests
+Ran 23 tests
 
 OK
-- 
2.20.1

Re: [RFC PATCH v2 2/4] iotests: Update 245 to support replacing files with x-blockdev-reopen

[Kevin Wolf]

Am 08.02.2021 um 19:44 hat Alberto Garcia geschrieben:
> Signed-off-by: Alberto Garcia <berto@igalia.com>

> +    def test_insert_throttle_filter(self):
> +        hd0_opts = hd_opts(0)
> +        result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts)
> +        self.assert_qmp(result, 'return', {})
> +
> +        opts = { 'qom-type': 'throttle-group', 'id': 'group0',
> +                 'props': { 'limits': { 'iops-total': 1000 } } }

Please don't add new users of 'props', it's deprecated. Instead, specify
'limits' on the top level.

Kevin

[RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 qapi/block-core.json       |  2 +-
 include/block/block.h      |  1 +
 block.c                    | 16 +++++--
 blockdev.c                 | 85 +++++++++++++++++++++-----------------
 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/298     |  4 +-
 10 files changed, 89 insertions(+), 63 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index c0e7c23331..b9fcf20a81 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4177,7 +4177,7 @@
 # Since: 4.0
 ##
 { 'command': 'x-blockdev-reopen',
-  'data': 'BlockdevOptions', 'boxed': true }
+  'data': { 'options': ['BlockdevOptions'] } }
 
 ##
 # @blockdev-del:
diff --git a/include/block/block.h b/include/block/block.h
index 6dd687a69e..fe4a220da9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -372,6 +372,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 19b62da4af..b4fef2308f 100644
--- a/block.c
+++ b/block.c
@@ -3933,6 +3933,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.
  *
@@ -4024,10 +4035,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;
 }
diff --git a/blockdev.c b/blockdev.c
index 098a05709d..6b688c0f73 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3528,38 +3528,16 @@ 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;
-    QObject *obj;
-    Visitor *v = qobject_output_visitor_new(&obj);
-    BlockReopenQueue *queue;
-    QDict *qdict;
-
-    /* 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, "Cannot find node named '%s'", options->node_name);
-        goto fail;
-    }
-
-    /* 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 */
+    BlockReopenQueue *queue = NULL;
+    GSList *aio_ctxs = NULL;
+    GSList *visitors = NULL;
+    GSList *drained = NULL;
     BdrvNextIterator it;
-    GSList *aio_ctxs = NULL, *ctx;
     BlockDriverState *it_bs;
 
+    /* Acquire all AIO contexts */
     for (it_bs = bdrv_first(&it); it_bs; it_bs = bdrv_next(&it)) {
         AioContext *aio_context = bdrv_get_aio_context(it_bs);
 
@@ -3569,19 +3547,50 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
         }
     }
 
-    bdrv_subtree_drained_begin(bs);
-    queue = bdrv_reopen_queue(NULL, bs, qdict, false);
+    /* 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;
+        QDict *qdict;
+        Visitor *v;
+        BlockDriverState *bs;
+        QObject *obj;
+
+        /* 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, "Cannot find node named '%s'", options->node_name);
+            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);
+
+        bdrv_subtree_drained_begin(bs);
+        queue = bdrv_reopen_queue(queue, bs, qdict, false);
+        drained = g_slist_prepend(drained, bs);
+    }
+
+    /* Perform the reopen operation */
     bdrv_reopen_multiple(queue, errp);
-    bdrv_subtree_drained_end(bs);
-
-    for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) {
-        AioContext *aio_context = ctx->data;
-        aio_context_release(aio_context);
-    }
-    g_slist_free(aio_ctxs);
+    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 988f986144..5271f9541f 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -260,9 +260,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 fb56a769b4..32db5086e1 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -136,7 +136,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': {
@@ -144,7 +144,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 bad6911f0c..850c9f070b 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -84,8 +84,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)
 
@@ -100,12 +110,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
@@ -141,10 +146,10 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         # We cannot change any of these
         self.reopen(opts, {'node-name': 'not-found'}, "Cannot find node named 'not-found'")
         self.reopen(opts, {'node-name': ''}, "Cannot find node named ''")
-        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")
@@ -153,7 +158,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 18ba03467e..2b43853183 100755
--- a/tests/qemu-iotests/248
+++ b/tests/qemu-iotests/248
@@ -62,7 +62,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/298 b/tests/qemu-iotests/298
index d535946b5f..4efdb35b91 100644
--- 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: [RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen

[Vladimir Sementsov-Ogievskiy]

08.02.2021 21:44, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   qapi/block-core.json       |  2 +-
>   include/block/block.h      |  1 +
>   block.c                    | 16 +++++--
>   blockdev.c                 | 85 +++++++++++++++++++++-----------------
>   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/298     |  4 +-
>   10 files changed, 89 insertions(+), 63 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c0e7c23331..b9fcf20a81 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4177,7 +4177,7 @@
>   # Since: 4.0
>   ##
>   { 'command': 'x-blockdev-reopen',
> -  'data': 'BlockdevOptions', 'boxed': true }
> +  'data': { 'options': ['BlockdevOptions'] } }

Do we also want to drop x- prefix?

>   
>   ##
>   # @blockdev-del:
> diff --git a/include/block/block.h b/include/block/block.h
> index 6dd687a69e..fe4a220da9 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -372,6 +372,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 19b62da4af..b4fef2308f 100644
> --- a/block.c
> +++ b/block.c
> @@ -3933,6 +3933,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.
>    *
> @@ -4024,10 +4035,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);

this may be a separate patch

>   
>       return ret;
>   }
> diff --git a/blockdev.c b/blockdev.c
> index 098a05709d..6b688c0f73 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3528,38 +3528,16 @@ 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;
> -    QObject *obj;
> -    Visitor *v = qobject_output_visitor_new(&obj);
> -    BlockReopenQueue *queue;
> -    QDict *qdict;
> -
> -    /* 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, "Cannot find node named '%s'", options->node_name);
> -        goto fail;
> -    }
> -
> -    /* 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 */
> +    BlockReopenQueue *queue = NULL;
> +    GSList *aio_ctxs = NULL;
> +    GSList *visitors = NULL;
> +    GSList *drained = NULL;
>       BdrvNextIterator it;
> -    GSList *aio_ctxs = NULL, *ctx;
>       BlockDriverState *it_bs;
>   
> +    /* Acquire all AIO contexts */
>       for (it_bs = bdrv_first(&it); it_bs; it_bs = bdrv_next(&it)) {
>           AioContext *aio_context = bdrv_get_aio_context(it_bs);
>   
> @@ -3569,19 +3547,50 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
>           }
>       }
>   
> -    bdrv_subtree_drained_begin(bs);
> -    queue = bdrv_reopen_queue(NULL, bs, qdict, false);
> +    /* 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;
> +        QDict *qdict;
> +        Visitor *v;
> +        BlockDriverState *bs;
> +        QObject *obj;
> +
> +        /* 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, "Cannot find node named '%s'", options->node_name);
> +            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);
> +
> +        bdrv_subtree_drained_begin(bs);
> +        queue = bdrv_reopen_queue(queue, bs, qdict, false);
> +        drained = g_slist_prepend(drained, bs);
> +    }
> +
> +    /* Perform the reopen operation */
>       bdrv_reopen_multiple(queue, errp);
> -    bdrv_subtree_drained_end(bs);
> -
> -    for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) {
> -        AioContext *aio_context = ctx->data;
> -        aio_context_release(aio_context);
> -    }
> -    g_slist_free(aio_ctxs);
> +    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);

Probably you can use g_autoslist() for defining these lists to get automatic cleanup.

>   }
>   
>   void qmp_blockdev_del(const char *node_name, Error **errp)
> diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
> index 988f986144..5271f9541f 100755
> --- a/tests/qemu-iotests/155
> +++ b/tests/qemu-iotests/155
> @@ -260,9 +260,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 fb56a769b4..32db5086e1 100755
> --- a/tests/qemu-iotests/165
> +++ b/tests/qemu-iotests/165
> @@ -136,7 +136,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': {
> @@ -144,7 +144,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 bad6911f0c..850c9f070b 100755
> --- a/tests/qemu-iotests/245
> +++ b/tests/qemu-iotests/245
> @@ -84,8 +84,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)
>   
> @@ -100,12 +110,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
> @@ -141,10 +146,10 @@ class TestBlockdevReopen(iotests.QMPTestCase):
>           # We cannot change any of these
>           self.reopen(opts, {'node-name': 'not-found'}, "Cannot find node named 'not-found'")
>           self.reopen(opts, {'node-name': ''}, "Cannot find node named ''")
> -        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")
> @@ -153,7 +158,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 18ba03467e..2b43853183 100755
> --- a/tests/qemu-iotests/248
> +++ b/tests/qemu-iotests/248
> @@ -62,7 +62,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/298 b/tests/qemu-iotests/298
> index d535946b5f..4efdb35b91 100644
> --- 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')
> 


-- 
Best regards,
Vladimir

Re: [RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen

[Alberto Garcia]

On Tue 09 Feb 2021 09:03:02 AM CET, Vladimir Sementsov-Ogievskiy wrote:
>>   { 'command': 'x-blockdev-reopen',
>> -  'data': 'BlockdevOptions', 'boxed': true }
>> +  'data': { 'options': ['BlockdevOptions'] } }
>
> Do we also want to drop x- prefix?

I think we can drop it once it's clear the the API is fine. It can be on
a separate patch after this.

>> -    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);
>
> Probably you can use g_autoslist() for defining these lists to get
> automatic cleanup.

g_autoslist() requires that the type has a cleanup function, but that's
not the case here and I don't think we can add one ('drained' contains a
BlockDriverState, what's the cleanup function? bdrv_subtree_drained_end
or bdrv_unref?)

I think it's fine to call g_slist_free_full() explicitly in this case.

Berto

Re: [RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen

[Kevin Wolf]

Am 09.02.2021 um 09:03 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 08.02.2021 21:44, Alberto Garcia wrote:
> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> > ---
> >   qapi/block-core.json       |  2 +-
> >   include/block/block.h      |  1 +
> >   block.c                    | 16 +++++--
> >   blockdev.c                 | 85 +++++++++++++++++++++-----------------
> >   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/298     |  4 +-
> >   10 files changed, 89 insertions(+), 63 deletions(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index c0e7c23331..b9fcf20a81 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4177,7 +4177,7 @@
> >   # Since: 4.0
> >   ##
> >   { 'command': 'x-blockdev-reopen',
> > -  'data': 'BlockdevOptions', 'boxed': true }
> > +  'data': { 'options': ['BlockdevOptions'] } }
> 
> Do we also want to drop x- prefix?

libvirt really wants to have a stable blockdev-reopen interface in 6.0
because enabling the incremental backup code depends on this (they just
toggle the readonly flag if I understand correctly, so most of the work
we're currently doing isn't even relevant at this moment for libvirt).

Given that the soft freeze is coming closer (March 16), I wonder if we
should just make this API change and declare the interface stable. We
can then make Vladimir's fixes and the file reopening on top of it - if
it's in time for 6.0, that would be good, but if not we could move it to
6.1 without impacting libvirt.

I think we're reasonable confident that the QAPI interfaces are right,
even if maybe not that all aspects of the implementation are right yet.

What do you think?

Kevin

Re: [RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen

[Vladimir Sementsov-Ogievskiy]

24.02.2021 15:33, Kevin Wolf wrote:
> Am 09.02.2021 um 09:03 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 08.02.2021 21:44, Alberto Garcia wrote:
>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>> ---
>>>    qapi/block-core.json       |  2 +-
>>>    include/block/block.h      |  1 +
>>>    block.c                    | 16 +++++--
>>>    blockdev.c                 | 85 +++++++++++++++++++++-----------------
>>>    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/298     |  4 +-
>>>    10 files changed, 89 insertions(+), 63 deletions(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index c0e7c23331..b9fcf20a81 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -4177,7 +4177,7 @@
>>>    # Since: 4.0
>>>    ##
>>>    { 'command': 'x-blockdev-reopen',
>>> -  'data': 'BlockdevOptions', 'boxed': true }
>>> +  'data': { 'options': ['BlockdevOptions'] } }
>>
>> Do we also want to drop x- prefix?
> 
> libvirt really wants to have a stable blockdev-reopen interface in 6.0
> because enabling the incremental backup code depends on this (they just
> toggle the readonly flag if I understand correctly, so most of the work
> we're currently doing isn't even relevant at this moment for libvirt).

Do you know what is the case exactly? If they do it to remove dirty bitmap
from backing image after snapshot operation, probably we'd better improve
block-dirty-bitmap-remove command to be able to reopen r-o image?

(I just recently faced such a task)

> 
> Given that the soft freeze is coming closer (March 16), I wonder if we
> should just make this API change and declare the interface stable. We
> can then make Vladimir's fixes and the file reopening on top of it - if
> it's in time for 6.0, that would be good, but if not we could move it to
> 6.1 without impacting libvirt.
> 
> I think we're reasonable confident that the QAPI interfaces are right,
> even if maybe not that all aspects of the implementation are right yet.
> 
> What do you think?
> 

I think it's OK.. We have it since 4.0. What will we win keeping -x for years? Even latest change from updating one device to several could be easily done with help of 'alternate' if the command was already stable.


-- 
Best regards,
Vladimir

Re: [RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen

[Kevin Wolf]

Am 25.02.2021 um 18:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 24.02.2021 15:33, Kevin Wolf wrote:
> > Am 09.02.2021 um 09:03 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 08.02.2021 21:44, Alberto Garcia wrote:
> > > > Signed-off-by: Alberto Garcia <berto@igalia.com>
> > > > ---
> > > >    qapi/block-core.json       |  2 +-
> > > >    include/block/block.h      |  1 +
> > > >    block.c                    | 16 +++++--
> > > >    blockdev.c                 | 85 +++++++++++++++++++++-----------------
> > > >    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/298     |  4 +-
> > > >    10 files changed, 89 insertions(+), 63 deletions(-)
> > > > 
> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > index c0e7c23331..b9fcf20a81 100644
> > > > --- a/qapi/block-core.json
> > > > +++ b/qapi/block-core.json
> > > > @@ -4177,7 +4177,7 @@
> > > >    # Since: 4.0
> > > >    ##
> > > >    { 'command': 'x-blockdev-reopen',
> > > > -  'data': 'BlockdevOptions', 'boxed': true }
> > > > +  'data': { 'options': ['BlockdevOptions'] } }
> > > 
> > > Do we also want to drop x- prefix?
> > 
> > libvirt really wants to have a stable blockdev-reopen interface in 6.0
> > because enabling the incremental backup code depends on this (they just
> > toggle the readonly flag if I understand correctly, so most of the work
> > we're currently doing isn't even relevant at this moment for libvirt).
> 
> Do you know what is the case exactly? If they do it to remove dirty bitmap
> from backing image after snapshot operation, probably we'd better improve
> block-dirty-bitmap-remove command to be able to reopen r-o image?
> 
> (I just recently faced such a task)

I think it was to switch nodes between read-only and read-write, but I
don't remember the exact context.

Peter, can you add the details?

> > Given that the soft freeze is coming closer (March 16), I wonder if we
> > should just make this API change and declare the interface stable. We
> > can then make Vladimir's fixes and the file reopening on top of it - if
> > it's in time for 6.0, that would be good, but if not we could move it to
> > 6.1 without impacting libvirt.
> > 
> > I think we're reasonable confident that the QAPI interfaces are right,
> > even if maybe not that all aspects of the implementation are right yet.
> > 
> > What do you think?
> 
> I think it's OK.. We have it since 4.0. What will we win keeping -x
> for years? Even latest change from updating one device to several
> could be easily done with help of 'alternate' if the command was
> already stable.

I think your series is kind of important to really call the
implementation stable. We can always feature flags to indicate the fixes
if necessary, but it would still feel better to declare something stable
that doesn't have known bugs. :-)

Do you think your series will still take a while? Maybe my first
comments sounded a bit negative because it was really hard to review at
first without knowing the final state, but after all I think the
approach is sane and apart from some implementation details, we're not
that far away from getting it into a mergable state.

Kevin

Re: [RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen

[Vladimir Sementsov-Ogievskiy]

01.03.2021 14:07, Kevin Wolf wrote:
> Am 25.02.2021 um 18:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 24.02.2021 15:33, Kevin Wolf wrote:
>>> Am 09.02.2021 um 09:03 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 08.02.2021 21:44, Alberto Garcia wrote:
>>>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>>>> ---
>>>>>     qapi/block-core.json       |  2 +-
>>>>>     include/block/block.h      |  1 +
>>>>>     block.c                    | 16 +++++--
>>>>>     blockdev.c                 | 85 +++++++++++++++++++++-----------------
>>>>>     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/298     |  4 +-
>>>>>     10 files changed, 89 insertions(+), 63 deletions(-)
>>>>>
>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>>> index c0e7c23331..b9fcf20a81 100644
>>>>> --- a/qapi/block-core.json
>>>>> +++ b/qapi/block-core.json
>>>>> @@ -4177,7 +4177,7 @@
>>>>>     # Since: 4.0
>>>>>     ##
>>>>>     { 'command': 'x-blockdev-reopen',
>>>>> -  'data': 'BlockdevOptions', 'boxed': true }
>>>>> +  'data': { 'options': ['BlockdevOptions'] } }
>>>>
>>>> Do we also want to drop x- prefix?
>>>
>>> libvirt really wants to have a stable blockdev-reopen interface in 6.0
>>> because enabling the incremental backup code depends on this (they just
>>> toggle the readonly flag if I understand correctly, so most of the work
>>> we're currently doing isn't even relevant at this moment for libvirt).
>>
>> Do you know what is the case exactly? If they do it to remove dirty bitmap
>> from backing image after snapshot operation, probably we'd better improve
>> block-dirty-bitmap-remove command to be able to reopen r-o image?
>>
>> (I just recently faced such a task)
> 
> I think it was to switch nodes between read-only and read-write, but I
> don't remember the exact context.
> 

I already don't think that making implicit reopen-to-rw is a good idea. It's OK for blockdev-commit, but may be unexpected for bitmaps manipulation.

> 
>>> Given that the soft freeze is coming closer (March 16), I wonder if we
>>> should just make this API change and declare the interface stable. We
>>> can then make Vladimir's fixes and the file reopening on top of it - if
>>> it's in time for 6.0, that would be good, but if not we could move it to
>>> 6.1 without impacting libvirt.
>>>
>>> I think we're reasonable confident that the QAPI interfaces are right,
>>> even if maybe not that all aspects of the implementation are right yet.
>>>
>>> What do you think?
>>
>> I think it's OK.. We have it since 4.0. What will we win keeping -x
>> for years? Even latest change from updating one device to several
>> could be easily done with help of 'alternate' if the command was
>> already stable.
> 
> I think your series is kind of important to really call the
> implementation stable. We can always feature flags to indicate the fixes
> if necessary, but it would still feel better to declare something stable
> that doesn't have known bugs. :-)
> 
> Do you think your series will still take a while? Maybe my first
> comments sounded a bit negative because it was really hard to review at
> first without knowing the final state, but after all I think the
> approach is sane and apart from some implementation details, we're not
> that far away from getting it into a mergable state.
> 

Thanks :)

I'm now busy with our bugs for Virtuozzo release.. Still, I hope, I'll have a chance to reroll permission-update series this week.

-- 
Best regards,
Vladimir

Re: [RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen

[Peter Krempa]

On Mon, Mar 01, 2021 at 12:07:26 +0100, Kevin Wolf wrote:
> Am 25.02.2021 um 18:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > 24.02.2021 15:33, Kevin Wolf wrote:
> > > Am 09.02.2021 um 09:03 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > 08.02.2021 21:44, Alberto Garcia wrote:
> > > > > Signed-off-by: Alberto Garcia <berto@igalia.com>
> > > > > ---
> > > > >    qapi/block-core.json       |  2 +-
> > > > >    include/block/block.h      |  1 +
> > > > >    block.c                    | 16 +++++--
> > > > >    blockdev.c                 | 85 +++++++++++++++++++++-----------------
> > > > >    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/298     |  4 +-
> > > > >    10 files changed, 89 insertions(+), 63 deletions(-)
> > > > > 
> > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > > index c0e7c23331..b9fcf20a81 100644
> > > > > --- a/qapi/block-core.json
> > > > > +++ b/qapi/block-core.json
> > > > > @@ -4177,7 +4177,7 @@
> > > > >    # Since: 4.0
> > > > >    ##
> > > > >    { 'command': 'x-blockdev-reopen',
> > > > > -  'data': 'BlockdevOptions', 'boxed': true }
> > > > > +  'data': { 'options': ['BlockdevOptions'] } }
> > > > 
> > > > Do we also want to drop x- prefix?
> > > 
> > > libvirt really wants to have a stable blockdev-reopen interface in 6.0
> > > because enabling the incremental backup code depends on this (they just
> > > toggle the readonly flag if I understand correctly, so most of the work
> > > we're currently doing isn't even relevant at this moment for libvirt).
> > 
> > Do you know what is the case exactly? If they do it to remove dirty bitmap
> > from backing image after snapshot operation, probably we'd better improve
> > block-dirty-bitmap-remove command to be able to reopen r-o image?
> > 
> > (I just recently faced such a task)
> 
> I think it was to switch nodes between read-only and read-write, but I
> don't remember the exact context.
> 
> Peter, can you add the details?

Libvirt indeed has code to drive blockdev-reopen to use it to toggle the
read-write state for block dirty bitmap manipulation.

Few of the cases where we use it:

- after blockjobs to toggle the destination image writable (the
  blockjobs turns it RO when it finishes) so that we can merge the
  appropriate bitmaps

- for deletion of bitmaps in backing images in the checkpoint APIs

Both of the operations really require blockdev-reopen and are required
for the incremental backup feature as whole since bitmap manipulation is
crucial for it and it doesn't happen automatically.

The blockdev code has the facilities to also use blockdev-reopen to
change RO/RW state for blockjobs if it will be needed in the future or
it can be later used to even replace the auto-read-only feature of the
protocol node drivers which were actually added to make -blockdev work
in libvirt's s-virt environment properly.

Re: [RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen

[Alberto Garcia]

On Wed 24 Feb 2021 01:33:05 PM CET, Kevin Wolf <kwolf@redhat.com> wrote:
>> >   { 'command': 'x-blockdev-reopen',
>> > -  'data': 'BlockdevOptions', 'boxed': true }
>> > +  'data': { 'options': ['BlockdevOptions'] } }
>> 
>> Do we also want to drop x- prefix?
>
> libvirt really wants to have a stable blockdev-reopen interface in 6.0
> because enabling the incremental backup code depends on this (they
> just toggle the readonly flag if I understand correctly, so most of
> the work we're currently doing isn't even relevant at this moment for
> libvirt).
>
> Given that the soft freeze is coming closer (March 16), I wonder if we
> should just make this API change and declare the interface stable. We
> can then make Vladimir's fixes and the file reopening on top of it -
> if it's in time for 6.0, that would be good, but if not we could move
> it to 6.1 without impacting libvirt.

I expect to publish the new version of my patches next week, although
they still apply on top of Vladimir's code, which is not rebased.

We can of course simply update the API and implement the functionality
later, but apart from dropping the prefix we would also be changing the
parameters so qmp_x_blockdev_reopen() would also need to be modified.

Berto

[RFC PATCH v2 4/4] iotests: Test reopening multiple devices at the same time

[Alberto Garcia]

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

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 850c9f070b..d18dbbe638 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -574,6 +574,46 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         self.reopen(opts, {'file': 'hd1-file'})
         self.run_qemu_io("hd", "read  -P 0xa1 0 10k")
 
+    def test_swap_files(self):
+        opts0 = hd_opts(0)
+        opts2 = hd_opts(2)
+
+        # Add hd0 and hd2 (none of them with backing files)
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **opts0)
+        self.assert_qmp(result, 'return', {})
+        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")
+
     def test_insert_throttle_filter(self):
         hd0_opts = hd_opts(0)
         result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts)
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 537a2b5b63..1f9debbd61 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -10,8 +10,8 @@
 {"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"}}
-.......................
+........................
 ----------------------------------------------------------------------
-Ran 23 tests
+Ran 24 tests
 
 OK
-- 
2.20.1

Re: [RFC PATCH v2 0/4] Allow changing bs->file on reopen

[Vladimir Sementsov-Ogievskiy]

08.02.2021 21:44, Alberto Garcia wrote:
> Hi,
> 
> this series allows changing bs->file using x-blockdev-reopen. Read
> here for more details:
> 
>     https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00437.html
> 
> Version 2 of the series introduces a very significant change:
> x-blockdev-reopen now receives a list of BlockdevOptions instead of
> just one, so it is possible to reopen multiple block devices using a
> single transaction.
> 
> This is still an RFC, I haven't updated the documentation and the
> structure of the patches will probably change in the future, but I'd
> like to know your opinion about the approach.
> 
> These patches apply on top of Vladimir's branch:
> 
> git: https://src.openvz.org/scm/~vsementsov/qemu.git
> tag: up-block-topologic-perm-v2

Patchew understands "Based-on: MESSAGE_ID" tag, so, you can add:

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

> 
> Regards,
> 
> Berto
> 
> Alberto Garcia (4):
>    block: Allow changing bs->file on reopen
>    iotests: Update 245 to support replacing files with x-blockdev-reopen
>    block: Support multiple reopening with x-blockdev-reopen
>    iotests: Test reopening multiple devices at the same time
> 
>   qapi/block-core.json       |   2 +-
>   include/block/block.h      |   2 +
>   block.c                    |  81 +++++++++++++++++++++--
>   blockdev.c                 |  85 +++++++++++++-----------
>   tests/qemu-iotests/155     |   9 ++-
>   tests/qemu-iotests/165     |   4 +-
>   tests/qemu-iotests/245     | 128 ++++++++++++++++++++++++++++++++-----
>   tests/qemu-iotests/245.out |   4 +-
>   tests/qemu-iotests/248     |   2 +-
>   tests/qemu-iotests/248.out |   2 +-
>   tests/qemu-iotests/298     |   4 +-
>   11 files changed, 254 insertions(+), 69 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [RFC PATCH v2 0/4] Allow changing bs->file on reopen

[Kevin Wolf]

Am 08.02.2021 um 19:44 hat Alberto Garcia geschrieben:
> Hi,
> 
> this series allows changing bs->file using x-blockdev-reopen. Read
> here for more details:
> 
>    https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00437.html
> 
> Version 2 of the series introduces a very significant change:
> x-blockdev-reopen now receives a list of BlockdevOptions instead of
> just one, so it is possible to reopen multiple block devices using a
> single transaction.

Adding Peter to Cc for this one.

> This is still an RFC, I haven't updated the documentation and the
> structure of the patches will probably change in the future, but I'd
> like to know your opinion about the approach.

I like the direction where this is going.

You have a test case for adding a throttling filter. Can we also remove
it again or is there still a problem with that? I seem to remember that
that was a bit trickier, though I'm not sure what it was. Was it that we
can't have the throttle node without a file, so it would possibly still
have permission conflicts?

Kevin

Re: [RFC PATCH v2 0/4] Allow changing bs->file on reopen

[Peter Krempa]

On Wed, Feb 10, 2021 at 18:26:57 +0100, Kevin Wolf wrote:
> Am 08.02.2021 um 19:44 hat Alberto Garcia geschrieben:
> > Hi,
> > 
> > this series allows changing bs->file using x-blockdev-reopen. Read
> > here for more details:
> > 
> >    https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00437.html
> > 
> > Version 2 of the series introduces a very significant change:
> > x-blockdev-reopen now receives a list of BlockdevOptions instead of
> > just one, so it is possible to reopen multiple block devices using a
> > single transaction.
> 
> Adding Peter to Cc for this one.

For now I've only used blockdev-reopen for turning backing files
read-write and back for bitmap manipulation. In such case I presume it
doesn't really make sense to use the batching if I ever need to reopen
multiple images.

Other than that, the blockdev-reopen code is still dormant in libvirt so
it's easy to add the extra required wrapping without any problem.

Re: [RFC PATCH v2 0/4] Allow changing bs->file on reopen

[Alberto Garcia]

On Wed 10 Feb 2021 06:26:57 PM CET, Kevin Wolf wrote:

> You have a test case for adding a throttling filter. Can we also
> remove it again or is there still a problem with that? I seem to
> remember that that was a bit trickier, though I'm not sure what it
> was. Was it that we can't have the throttle node without a file, so it
> would possibly still have permission conflicts?

There is no problem with removing the filter anymore. See here for a
description of the original problem:

https://lists.gnu.org/archive/html/qemu-block/2020-12/msg00090.html

But this series is based on Vladimir's branch ("update graph permissions
update") which reworks how the permissions are calculated on reopen and
solves the issue.

Berto

Re: [RFC PATCH v2 0/4] Allow changing bs->file on reopen

[Kevin Wolf]

Am 16.02.2021 um 17:36 hat Alberto Garcia geschrieben:
> On Wed 10 Feb 2021 06:26:57 PM CET, Kevin Wolf wrote:
> 
> > You have a test case for adding a throttling filter. Can we also
> > remove it again or is there still a problem with that? I seem to
> > remember that that was a bit trickier, though I'm not sure what it
> > was. Was it that we can't have the throttle node without a file, so it
> > would possibly still have permission conflicts?
> 
> There is no problem with removing the filter anymore. See here for a
> description of the original problem:
> 
> https://lists.gnu.org/archive/html/qemu-block/2020-12/msg00090.html

Ah, nice. Can we just add removing the filter again to the test then?

> But this series is based on Vladimir's branch ("update graph permissions
> update") which reworks how the permissions are calculated on reopen and
> solves the issue.

Yes, I'm aware of this. I still hope we can get a stable blockdev-reopen
for 6.0.

Kevin

Re: [RFC PATCH v2 0/4] Allow changing bs->file on reopen

[Alberto Garcia]

On Tue 16 Feb 2021 05:48:07 PM CET, Kevin Wolf wrote:

>> There is no problem with removing the filter anymore. See here for a
>> description of the original problem:
>> 
>> https://lists.gnu.org/archive/html/qemu-block/2020-12/msg00090.html
>
> Ah, nice. Can we just add removing the filter again to the test then?

That is already in this series, see patch #2.

Berto

Re: [RFC PATCH v2 0/4] Allow changing bs->file on reopen

[Kevin Wolf]

Am 16.02.2021 um 18:25 hat Alberto Garcia geschrieben:
> On Tue 16 Feb 2021 05:48:07 PM CET, Kevin Wolf wrote:
> 
> >> There is no problem with removing the filter anymore. See here for a
> >> description of the original problem:
> >> 
> >> https://lists.gnu.org/archive/html/qemu-block/2020-12/msg00090.html
> >
> > Ah, nice. Can we just add removing the filter again to the test then?
> 
> That is already in this series, see patch #2.

Oh, indeed. I missed the second reopen, probably expected the code to be
longer than that. :-)

Kevin