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

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

[Alberto Garcia]

Hi,

during the past months we talked about making x-blockdev-reopen stable
API, and one of the missing things was having support for changing
bs->file. See here for the discusssion (I can't find the message from
Kashyap that started the thread in the web archives):

   https://lists.gnu.org/archive/html/qemu-block/2020-10/msg00922.html

I was testing this and one of the problems that I found was that
removing a filter node using this command is tricky because of the
permission system, see here for details:

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

The good news is that Vladimir posted a set of patches that changes
the way that permissions are updated on reopen:

   https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00745.html

I was testing if this would be useful to solve the problem that I
mentioned earlier and it seems to be the case so I wrote a patch to
add support for changing bs->file, along with a couple of test cases.

This is still an RFC but you can see the idea.

These patches apply on top of Vladimir's branch:

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

Opinions are very welcome!

Berto

Alberto Garcia (2):
  block: Allow changing bs->file on reopen
  iotests: Update 245 to support replacing files with x-blockdev-reopen

 include/block/block.h      |  1 +
 block.c                    | 61 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/245     | 61 +++++++++++++++++++++++++++++++++++---
 tests/qemu-iotests/245.out |  4 +--
 4 files changed, 121 insertions(+), 6 deletions(-)

-- 
2.20.1

[RFC PATCH 1/2] 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                | 61 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/245 |  7 ++---
 3 files changed, 66 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..114788e58e 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,57 @@ 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;
+    }
+
+    /* Check AioContext compatibility */
+    if (!bdrv_reopen_can_attach(bs, bs->file, new_file_bs, errp)) {
+        return -EINVAL;
+    }
+
+    /* At the moment only backing links are frozen */
+    assert(!bs->file->frozen);
+
+    /* 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 +4402,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 1/2] block: Allow changing bs->file on reopen

[Vladimir Sementsov-Ogievskiy]

15.01.2021 16:02, 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                | 61 ++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/245 |  7 ++---
>   3 files changed, 66 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..114788e58e 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,57 @@ 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);

why are we sure at this point? Probably, we should just return an error..

> +
> +    /* If 'file' points to the current child then there's nothing to do */
> +    if (bs->file->bs == new_file_bs) {
> +        return 0;
> +    }
> +
> +    /* Check AioContext compatibility */
> +    if (!bdrv_reopen_can_attach(bs, bs->file, new_file_bs, errp)) {
> +        return -EINVAL;
> +    }
> +
> +    /* At the moment only backing links are frozen */
> +    assert(!bs->file->frozen);

I think it can: file-child based filters can be a part of frozen backing chain currently.

> +
> +    /* 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 +4402,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 1/2] block: Allow changing bs->file on reopen

[Alberto Garcia]

On Mon 18 Jan 2021 11:15:17 AM CET, Vladimir Sementsov-Ogievskiy wrote:
>> +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);
>
> why are we sure at this point? Probably, we should just return an
> error..

Unlike 'backing', 'file' is a BlockdevRef and it is not optional, so
block devices that accept that parameter must have it set.

>> +    /* At the moment only backing links are frozen */
>> +    assert(!bs->file->frozen);
>
> I think it can: file-child based filters can be a part of frozen
> backing chain currently.

You're right, since 7b99a26600e bdrv_freeze_backing_chain() uses
bdrv_filter_or_cow_child().

Berto

[RFC PATCH 2/2] 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 0/2] Allow changing bs->file on reopen

[Kashyap Chamarthy]

[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]

On Fri, Jan 15, 2021 at 02:02:36PM +0100, Alberto Garcia wrote:
> Hi,

Hi, 

> during the past months we talked about making x-blockdev-reopen stable
> API, and one of the missing things was having support for changing
> bs->file. See here for the discusssion (I can't find the message from
> Kashyap that started the thread in the web archives):
> 
>    https://lists.gnu.org/archive/html/qemu-block/2020-10/msg00922.html

Yeah, I noticed that too -- seems like it got "lost" somehow :-(.  For
the record, I've attached here the original e-mail I sent on 06-OCT-2020
that started the above thread.

Thanks for working on this!

> I was testing this and one of the problems that I found was that
> removing a filter node using this command is tricky because of the
> permission system, see here for details:
> 
>    https://lists.gnu.org/archive/html/qemu-block/2020-12/msg00092.html
> 
> The good news is that Vladimir posted a set of patches that changes
> the way that permissions are updated on reopen:
> 
>    https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00745.html
> 
> I was testing if this would be useful to solve the problem that I
> mentioned earlier and it seems to be the case so I wrote a patch to
> add support for changing bs->file, along with a couple of test cases.
> 
> This is still an RFC but you can see the idea.
> 
> These patches apply on top of Vladimir's branch:
> 
> git: https://src.openvz.org/scm/~vsementsov/qemu.git
> tag: up-block-topologic-perm-v2
> 
> Opinions are very welcome!
> 
> Berto
> 
> Alberto Garcia (2):
>   block: Allow changing bs->file on reopen
>   iotests: Update 245 to support replacing files with x-blockdev-reopen
> 
>  include/block/block.h      |  1 +
>  block.c                    | 61 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/245     | 61 +++++++++++++++++++++++++++++++++++---
>  tests/qemu-iotests/245.out |  4 +--
>  4 files changed, 121 insertions(+), 6 deletions(-)
> 
> -- 
> 2.20.1
> 

-- 
/kashyap

[-- Attachment #2: x-blockdev-reopen-thread_06OCT2020.txt --]
[-- Type: text/plain, Size: 2097 bytes --]

Date: Tue, 6 Oct 2020 11:10:01 +0200
From: Kashyap Chamarthy <kchamart@redhat.com>
To: qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: berto@igalia.com, eblake@redhat.com
Subject: Plans to bring QMP 'x-blockdev-reopen' out of experimental?
Message-ID: <20201006091001.GA64583@paraplu>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hi, folks

If this was already discussed on the list, please point me to the
thread.  I took a quick look at my local archives, I didn't find any,
besides patches to tests.

I learn that `x-blockdev-reopen` enables a couple of interesting use
cases:

(#) Allowing one to live-change the backing file to point to a different
    location, with the target having content identical to original.
    This one I was already familiar with.

(#) Yesterday I learnt another use case from Peter Krempa and Eric
    Blake.  Allow me to quote (paraphrasing) Eric's example from IRC.
    E.g.  we have (where 'overlay1' has a bitmap):

        base <- overlay1

    Then create a temporary snapshot (which results a bitmap being
    created in 'overlay2', because 'overlay1' had one):
    
        base <- overlay1 <- overlay2
    
    If you want to do a `block-commit` to merge 'overlay2' back into
    'overlay1', currently upstream QEMU does not merge the bitmap states
    from 'overlay2' back into 'overlay1' properly.  This current
    limitation is because QEMU can't merge the bitmaps unless it can
    reopen 'overlay1' [for read-write] _prior_ to doing the commit — but
    the only way to do that is with `x-blockdev-reopen`.

            - - -

From an old chat with Berto on #qemu, he was looking for some more
robust testing, before lifting it out of experimental mode, as it was a
rather complicated command to implement.

Currently, I see there are some 'qemu-iotests' that exercise
'x-blockdev-reopen': 155, 165, 245, and 248.  What else kind of tests
can give more confidence?

(I personally don't have an urgent need for this, so I'm not trying to
rush anything. :-))

-- 
/kashyap

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

[Vladimir Sementsov-Ogievskiy]

15.01.2021 16:02, Alberto Garcia wrote:
> Hi,
> 
> during the past months we talked about making x-blockdev-reopen stable
> API, and one of the missing things was having support for changing
> bs->file. See here for the discusssion (I can't find the message from
> Kashyap that started the thread in the web archives):
> 
>     https://lists.gnu.org/archive/html/qemu-block/2020-10/msg00922.html
> 
> I was testing this and one of the problems that I found was that
> removing a filter node using this command is tricky because of the
> permission system, see here for details:
> 
>     https://lists.gnu.org/archive/html/qemu-block/2020-12/msg00092.html
> 
> The good news is that Vladimir posted a set of patches that changes
> the way that permissions are updated on reopen:
> 
>     https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00745.html
> 
> I was testing if this would be useful to solve the problem that I
> mentioned earlier and it seems to be the case so I wrote a patch to
> add support for changing bs->file, along with a couple of test cases.
> 
> This is still an RFC but you can see the idea.

Good idea and I glad to see that my patches help:)

Hmm, still, removing a filter which want to unshare WRITE even when doesn't have any parents will be a problem anyway, so we'll need a new command to drop filter with a logic like in bdrv_drop_filter in my series.

Or, we can introduce multiple reopen.. So that x-blockdev-reopen will take a list of BlockdevOptions, and do all modifications in one transaction. Than we'll be able to drop filter by transactional update of top node child and removing filter child link.

> 
> These patches apply on top of Vladimir's branch:
> 
> git: https://src.openvz.org/scm/~vsementsov/qemu.git
> tag: up-block-topologic-perm-v2
> 
> Opinions are very welcome!
> 
> Berto
> 
> Alberto Garcia (2):
>    block: Allow changing bs->file on reopen
>    iotests: Update 245 to support replacing files with x-blockdev-reopen
> 
>   include/block/block.h      |  1 +
>   block.c                    | 61 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/245     | 61 +++++++++++++++++++++++++++++++++++---
>   tests/qemu-iotests/245.out |  4 +--
>   4 files changed, 121 insertions(+), 6 deletions(-)
> 


-- 
Best regards,
Vladimir

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

[Alberto Garcia]

On Mon 18 Jan 2021 11:22:49 AM CET, Vladimir Sementsov-Ogievskiy wrote:
>> This is still an RFC but you can see the idea.
>
> Good idea and I glad to see that my patches help:)
>
> Hmm, still, removing a filter which want to unshare WRITE even when
> doesn't have any parents will be a problem anyway, so we'll need a new
> command to drop filter with a logic like in bdrv_drop_filter in my
> series.

Do you have an example?

Berto

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

[Vladimir Sementsov-Ogievskiy]

20.01.2021 16:51, Alberto Garcia wrote:
> On Mon 18 Jan 2021 11:22:49 AM CET, Vladimir Sementsov-Ogievskiy wrote:
>>> This is still an RFC but you can see the idea.
>>
>> Good idea and I glad to see that my patches help:)
>>
>> Hmm, still, removing a filter which want to unshare WRITE even when
>> doesn't have any parents will be a problem anyway, so we'll need a new
>> command to drop filter with a logic like in bdrv_drop_filter in my
>> series.
> 
> Do you have an example?
> 

backup_top filter



-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 18.01.2021 um 11:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 15.01.2021 16:02, Alberto Garcia wrote:
> > Hi,
> > 
> > during the past months we talked about making x-blockdev-reopen stable
> > API, and one of the missing things was having support for changing
> > bs->file. See here for the discusssion (I can't find the message from
> > Kashyap that started the thread in the web archives):
> > 
> >     https://lists.gnu.org/archive/html/qemu-block/2020-10/msg00922.html
> > 
> > I was testing this and one of the problems that I found was that
> > removing a filter node using this command is tricky because of the
> > permission system, see here for details:
> > 
> >     https://lists.gnu.org/archive/html/qemu-block/2020-12/msg00092.html
> > 
> > The good news is that Vladimir posted a set of patches that changes
> > the way that permissions are updated on reopen:
> > 
> >     https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00745.html
> > 
> > I was testing if this would be useful to solve the problem that I
> > mentioned earlier and it seems to be the case so I wrote a patch to
> > add support for changing bs->file, along with a couple of test cases.
> > 
> > This is still an RFC but you can see the idea.
> 
> Good idea and I glad to see that my patches help:)
> 
> Hmm, still, removing a filter which want to unshare WRITE even when
> doesn't have any parents will be a problem anyway, so we'll need a new
> command to drop filter with a logic like in bdrv_drop_filter in my
> series.
> 
> Or, we can introduce multiple reopen.. So that x-blockdev-reopen will
> take a list of BlockdevOptions, and do all modifications in one
> transaction. Than we'll be able to drop filter by transactional update
> of top node child and removing filter child link.

Internally, we already have reopen queues anyway, so it would make sense
to me to expose them externally and take a list of BlockdevOptions.
This way we should be able to do even complex changes of the graph where
adding some edges requires the removal of other edges in a single atomic
operation.

Kevin

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

[Alberto Garcia]

On Thu 21 Jan 2021 11:52:17 AM CET, Kevin Wolf wrote:
>> Hmm, still, removing a filter which want to unshare WRITE even when
>> doesn't have any parents will be a problem anyway, so we'll need a
>> new command to drop filter with a logic like in bdrv_drop_filter in
>> my series.
>> 
>> Or, we can introduce multiple reopen.. So that x-blockdev-reopen will
>> take a list of BlockdevOptions, and do all modifications in one
>> transaction. Than we'll be able to drop filter by transactional
>> update of top node child and removing filter child link.
>
> Internally, we already have reopen queues anyway, so it would make
> sense to me to expose them externally and take a list of
> BlockdevOptions.  This way we should be able to do even complex
> changes of the graph where adding some edges requires the removal of
> other edges in a single atomic operation.

So you mean changing the signature to something like this?

{ 'command': 'x-blockdev-reopen',
  'data': { 'options': ['BlockdevOptions'] } }

It should be easy to make that change, I can have a look.

Berto

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

[Kevin Wolf]

Am 05.02.2021 um 13:47 hat Alberto Garcia geschrieben:
> On Thu 21 Jan 2021 11:52:17 AM CET, Kevin Wolf wrote:
> >> Hmm, still, removing a filter which want to unshare WRITE even when
> >> doesn't have any parents will be a problem anyway, so we'll need a
> >> new command to drop filter with a logic like in bdrv_drop_filter in
> >> my series.
> >> 
> >> Or, we can introduce multiple reopen.. So that x-blockdev-reopen will
> >> take a list of BlockdevOptions, and do all modifications in one
> >> transaction. Than we'll be able to drop filter by transactional
> >> update of top node child and removing filter child link.
> >
> > Internally, we already have reopen queues anyway, so it would make
> > sense to me to expose them externally and take a list of
> > BlockdevOptions.  This way we should be able to do even complex
> > changes of the graph where adding some edges requires the removal of
> > other edges in a single atomic operation.
> 
> So you mean changing the signature to something like this?
> 
> { 'command': 'x-blockdev-reopen',
>   'data': { 'options': ['BlockdevOptions'] } }
> 
> It should be easy to make that change, I can have a look.

Yes, this is what I had in mind.

Kevin