Re: [PATCH v2 06/11] iotests/290: add test case to check 'discard-no-unref' option behavior
On Mon 13 May 2024 09:31:58 AM +03, Andrey Drobyshev wrote: > We basically fill 2 images with identical data and perform discard > operations with and without 'discard-no-unref' enabled. Then we check > that images still read identically, that their disk usage is the same > (i.e. fallocate(FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE) is called for > both) and that with the option enabled cluster is still marked as > allocated in "qemu-img map" output. We also check that the option > doesn't work with qcow2 v2 images. > > Signed-off-by: Andrey Drobyshev Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 05/11] iotests/common.rc: add disk_usage function
On Mon 13 May 2024 09:31:57 AM +03, Andrey Drobyshev wrote: > Move the definition from iotests/250 to common.rc. This is used to > detect real disk usage of sparse files. In particular, we want to use > it for checking subclusters-based discards. > > Signed-off-by: Andrey Drobyshev Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 04/11] block/file-posix: add trace event for fallocate() calls
On Mon 13 May 2024 09:31:56 AM +03, Andrey Drobyshev wrote: > This would ease debugging of write zeroes and discard operations. > > Signed-off-by: Andrey Drobyshev Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 02/11] qcow2: simplify L2 entries accounting for discard-no-unref
On Mon 13 May 2024 09:31:54 AM +03, Andrey Drobyshev wrote: > Commits 42a2890a and b2b10904 introduce handling of discard-no-unref > option in discard_in_l2_slice() and zero_in_l2_slice(). They add even > more if's when chosing the right l2 entry. What we really need for this > option is the new entry simply to contain the same host cluster offset, > no matter whether we unmap or zeroize the cluster. For that OR'ing with > the old entry is enough. > > This patch doesn't change the logic and is pure refactoring. > > Signed-off-by: Andrey Drobyshev Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 01/11] qcow2: make function update_refcount_discard() global
On Mon 13 May 2024 09:31:53 AM +03, Andrey Drobyshev wrote: > We are going to need it for discarding separate subclusters. The > function itself doesn't do anything with the refcount tables, it simply > adds a discard request to the queue, so rename it to qcow2_queue_discard(). > > Signed-off-by: Andrey Drobyshev > Reviewed-by: Hanna Czenczek Reviewed-by: Alberto Garcia Berto
Re: [PATCH for-9.0] qapi: Add 'recurse-children' option to qom-list
On Fri 22 Dec 2023 11:31:12 AM +01, Markus Armbruster wrote: >> This allows returning a tree of all object properties under a given >> path, in a way similar to scripts/qmp/qom-tree. > > Use case? We already have that script, and also HMP info qom-tree. The main use case is convenience. Management tools need to manually check that the type starts with "child<" and recursively make a new QMP call. That's what e.g libvirt does: https://gitlab.com/libvirt/libvirt/-/blob/v9.10.0/src/qemu/qemu_monitor_json.c?ref_type=tags#L7367 Parsing the output of HMP info qom-tree is not an option in that case. Berto
[PATCH for-9.0] qapi: Add 'recurse-children' option to qom-list
This allows returning a tree of all object properties under a given
path, in a way similar to scripts/qmp/qom-tree.
Signed-off-by: Alberto Garcia
---
qapi/qom.json | 10 +-
qom/qom-hmp-cmds.c | 4 ++--
qom/qom-qmp-cmds.c | 22 +-
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/qapi/qom.json b/qapi/qom.json
index c53ef978ff..dfe3a20725 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -33,6 +33,10 @@
#qdev device type name. Link properties form the device model
#graph.
#
+# @children: if specified, a list of @ObjectPropertyInfo describing
+# the child properties. This requires that this property's @type
+# is of the form 'child' (since 9.0)
+#
# @description: if specified, the description of the property.
#
# @default-value: the default value, if any (since 5.0)
@@ -42,6 +46,7 @@
{ 'struct': 'ObjectPropertyInfo',
'data': { 'name': 'str',
'type': 'str',
+'*children' : [ 'ObjectPropertyInfo' ],
'*description': 'str',
'*default-value': 'any' } }
@@ -54,6 +59,9 @@
# @path: the path within the object model. See @qom-get for a
# description of this parameter.
#
+# @recurse-children: if true, include the child properties recursively
+# in the return value (default: false) (since 9.0)
+#
# Returns: a list of @ObjectPropertyInfo that describe the properties
# of the object.
#
@@ -69,7 +77,7 @@
# { "name": "mon0", "type": "child" } ] }
##
{ 'command': 'qom-list',
- 'data': { 'path': 'str' },
+ 'data': { 'path': 'str', '*recurse-children': 'bool' },
'returns': [ 'ObjectPropertyInfo' ],
'allow-preconfig': true }
diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
index 6e3a2175a4..7592184fc3 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -28,7 +28,7 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict)
return;
}
-list = qmp_qom_list(path, );
+list = qmp_qom_list(path, false, false, );
if (err == NULL) {
ObjectPropertyInfoList *start = list;
while (list != NULL) {
@@ -206,7 +206,7 @@ void object_del_completion(ReadLineState *rs, int nb_args,
const char *str)
len = strlen(str);
readline_set_completion_index(rs, len);
-start = list = qmp_qom_list("/objects", NULL);
+start = list = qmp_qom_list("/objects", false, false, NULL);
while (list) {
ObjectPropertyInfo *info = list->value;
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 7c087299de..5c9cb8a09c 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -28,10 +28,15 @@
#include "qom/object_interfaces.h"
#include "qom/qom-qobject.h"
-ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
+ObjectPropertyInfoList *qmp_qom_list(const char *path,
+ bool has_recurse_children,
+ bool recurse_children,
+ Error **errp)
{
+ERRP_GUARD();
Object *obj;
bool ambiguous = false;
+bool recurse = has_recurse_children && recurse_children;
ObjectPropertyInfoList *props = NULL;
ObjectProperty *prop;
ObjectPropertyIterator iter;
@@ -55,8 +60,23 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error
**errp)
value->name = g_strdup(prop->name);
value->type = g_strdup(prop->type);
+
+if (recurse && g_str_has_prefix(prop->type, "child<")) {
+ObjectPropertyInfoList *children;
+g_autofree char *childpath = g_strdup_printf("%s/%s", path,
+ prop->name);
+children = qmp_qom_list(childpath, true, true, errp);
+if (*errp) {
+qapi_free_ObjectPropertyInfoList(props);
+props = NULL;
+goto out;
+}
+value->has_children = true;
+value->children = children;
+}
}
+out:
return props;
}
--
2.39.2
Re: Converting images to stdout
On Mon, Nov 20, 2023 at 05:23:27PM -0600, Eric Blake wrote: > > I'm interested in this use case, and I think that the method would be > > as simple as this: > > > > 1. Decide a cluster size for the output qcow2 file. > > 2. Read the input file once to determine which clusters need to be > >allocated in the output file and which ones don't. > > 3. That infomation is enough to determine the number and contents of > >the refcount table, refcount blocks, and L1/L2 tables. > > 4. Write the qcow2 header + metadata + allocated data to stdout. > > It may also be possible to write a qcow2 file that uses the external > data bit, so that you are only writing the qcow2 header + metadata, > and reusing the existing input file as the external data. Sure, although I'm not so certain about the use case here... also, the input file might not be raw. > > Because this would be an external tool it would only support > > a qcow2 file with the default options. Other features like > > compression would be out of scope. > > Why is compression not viable? Are you worried that the qcow2 > metadata (such as refcounts) becomes too complex? Yeah, mostly... also, since the output file would be streamable it's trivial to pipe it through gzip or whatever. > I've also wondered how easy or hard it would be to write a tool that > can take an existing qcow2 file and defragment and/or compress it > in-place (rather than having to copy it to a second qcow2 file). That sounds a bit more complex, but I guess it's doable. But not something that I need atm :) Berto
Converting images to stdout
Hi, I haven't written here in a while :) but I have something small that I would like to discuss. Using qemu-img to convert an image and writing the result directly to stdout is a question that has already been raised in the past (see [1] for an example) and it's clear that it's generally not possible because the images need to be seekable. While I think that there's almost certainly no generic way to do something like that for every combination of input and output formats, I do think that it should be relatively easy to produce a qcow2 file directly to stdout as long as the input file is on disk. I'm interested in this use case, and I think that the method would be as simple as this: 1. Decide a cluster size for the output qcow2 file. 2. Read the input file once to determine which clusters need to be allocated in the output file and which ones don't. 3. That infomation is enough to determine the number and contents of the refcount table, refcount blocks, and L1/L2 tables. 4. Write the qcow2 header + metadata + allocated data to stdout. Since this would be qcow2-specific I would probably implement this as a separate tool instead of adding it directly to qemu-img. This could go to the scripts/ directory because I can imagine that such a tool could be useful for other people. Because this would be an external tool it would only support a qcow2 file with the default options. Other features like compression would be out of scope. For the same reason the input would be a raw file for simplicity, other input files could be presented in raw format using qemu-storage-daemon. And that's the general idea, comments and questions are welcome. Thanks! Berto [1] https://lists.nongnu.org/archive/html/qemu-discuss/2020-01/msg00014.html
Re: [PATCH 02/10] tests/throttle: Clean up global variable shadowing
On Mon 09 Oct 2023 12:02:43 PM +02, Philippe Mathieu-Daudé wrote: > Follow all other tests pattern from this file, use the > global 'cfg' variable to fix: > > tests/unit/test-throttle.c:621:20: error: declaration shadows a variable in > the global scope [-Werror,-Wshadow] > ThrottleConfig cfg; > ^ > tests/unit/test-throttle.c:28:23: note: previous declaration is here > static ThrottleConfig cfg; > ^ > > Signed-off-by: Philippe Mathieu-Daudé Acked-by: Alberto Garcia Berto
[PATCH] test-throttle: don't shadow 'index' variable in do_test_accounting()
Fixes build with -Wshadow=local
Signed-off-by: Alberto Garcia
---
tests/unit/test-throttle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c
index cb587e33e7..ac35d65d19 100644
--- a/tests/unit/test-throttle.c
+++ b/tests/unit/test-throttle.c
@@ -625,7 +625,7 @@ static bool do_test_accounting(bool is_ops, /* are we
testing bps or ops */
throttle_config_init();
for (i = 0; i < 3; i++) {
-BucketType index = to_test[is_ops][i];
+index = to_test[is_ops][i];
cfg.buckets[index].avg = avg;
}
--
2.39.2
Re: [PATCH v2 4/5] test-throttle: test read only and write only
On Tue 27 Jun 2023 03:24:30 PM +08, zhenwei pi wrote: > Signed-off-by: zhenwei pi Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 5/5] cryptodev: use NULL throttle timer cb for read direction
On Tue 27 Jun 2023 03:24:31 PM +08, zhenwei pi wrote: > Operations on a crytpodev are considered as *write* only, the callback > of read direction is never invoked. Use NULL instead of an unreachable > path(cryptodev_backend_throttle_timer_cb on read direction). > > Signed-off-by: zhenwei pi Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 3/5] throttle: support read-only and write-only
On Tue 27 Jun 2023 03:24:29 PM +08, zhenwei pi wrote: > Only one direction is necessary in several scenarios: > - a read-only disk > - operations on a device are considered as *write* only. For example, > encrypt/decrypt/sign/verify operations on a cryptodev use a single > *write* timer(read timer callback is defined, but never invoked). > > Allow a single direction in throttle, this reduces memory, and uplayer > does not need a dummy callback any more. > > Signed-off-by: zhenwei pi Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 2/5] test-throttle: use enum ThrottleType
On Tue 27 Jun 2023 03:24:28 PM +08, zhenwei pi wrote: > Use enum ThrottleType instead in the throttle test codes. > > Signed-off-by: zhenwei pi Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 1/5] throttle: introduce enum ThrottleType
On Tue 27 Jun 2023 03:24:27 PM +08, zhenwei pi wrote: > Use enum ThrottleType instead of number index. > > Signed-off-by: zhenwei pi Reviewed-by: Alberto Garcia Berto
Re: [PATCH 3/5] throttle: support read-only and write-only
On Sun 25 Jun 2023 04:56:29 PM +08, zhenwei pi wrote:
> void throttle_timers_attach_aio_context(ThrottleTimers *tt,
> AioContext *new_context)
> {
> -tt->timers[THROTTLE_TIMER_READ] =
> -aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> - tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque);
> -tt->timers[THROTTLE_TIMER_WRITE] =
> -aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> - tt->timer_cb[THROTTLE_TIMER_WRITE], tt->timer_opaque);
> +if (tt->timer_cb[THROTTLE_TIMER_READ]) {
> +tt->timers[THROTTLE_TIMER_READ] =
> +aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> + tt->timer_cb[THROTTLE_TIMER_READ],
> tt->timer_opaque);
> +}
> +
> +if (tt->timer_cb[THROTTLE_TIMER_WRITE]) {
> +tt->timers[THROTTLE_TIMER_WRITE] =
> +aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> + tt->timer_cb[THROTTLE_TIMER_WRITE],
> tt->timer_opaque);
> +}
> }
If now any of the timers can be NULL, don't you want to add additional
checks / assertions to (at least) throttle_schedule_timer() ?
Berto
Re: [PATCH 4/5] test-throttle: test read only and write only
On Sun 25 Jun 2023 04:56:30 PM +08, zhenwei pi wrote: > Signed-off-by: zhenwei pi Reviewed-by: Alberto Garcia Berto
Re: [PATCH 5/5] cryptodev: use NULL throttle timer cb for read direction
On Sun 25 Jun 2023 04:56:31 PM +08, zhenwei pi wrote: > Operations on a crytpodev are considered as *write* only, the callback > of read direction is never invoked. Use NULL instead of an unreachable > path(cryptodev_backend_throttle_timer_cb on read direction). > > Signed-off-by: zhenwei pi Reviewed-by: Alberto Garcia Berto
Re: [PATCH 2/5] test-throttle: use enum ThrottleTimerType
On Sun 25 Jun 2023 04:56:28 PM +08, zhenwei pi wrote: > Use enum ThrottleTimerType instead in the throttle test codes. > > Signed-off-by: zhenwei pi Reviewed-by: Alberto Garcia Berto
Re: [PATCH 1/5] throttle: introduce enum ThrottleTimerType
On Sun 25 Jun 2023 04:56:27 PM +08, zhenwei pi wrote:
> Use enum ThrottleTimerType instead of number index.
> +typedef enum {
> +THROTTLE_TIMER_READ = 0,
> +THROTTLE_TIMER_WRITE,
> +THROTTLE_TIMER_MAX
> +} ThrottleTimerType;
If you're doing this I suppose you could also change 'bool is_write'
with something like 'ThrottleTimerType timer', i.e
static bool throttle_compute_timer(ThrottleState *ts,
ThrottleTimerType timer,
int64_t now,
int64_t *next_timestamp)
Berto
Re: [PATCH] tests/qemu-iotests/312: Mark "quorum" as required driver
On Wed 04 Jan 2023 12:46:01 PM +01, Thomas Huth wrote: > "quorum" is required by iotest 312 - if it is not compiled into the > QEMU binary, the test fails. Thus list "quorum" as required driver > so that the test gets skipped in case it is not available. > > Signed-off-by: Thomas Huth Reviewed-by: Alberto Garcia Berto
Re: [PATCH] ratelimit: restrict the delay time to a non-negative value
On Wed 21 Sep 2022 09:47:32 AM +08, Wang Liang wrote:
>> > -return limit->slice_end_time - now;
>> > +return MAX(limit->slice_end_time - now, 0);
>>
>> How can this be negative? slice_end_time is guaranteed to be larger
>> than
>> now:
>>
>> if (limit->slice_end_time < now) {
>> /* Previous, possibly extended, time slice finished; reset
>> the
>> * accounting. */
>> limit->slice_start_time = now;
>> limit->slice_end_time = now + limit->slice_ns;
>> limit->dispatched = 0;
>> }
>>
> This is just a guarantee.
>
> If slice_end_time is assigned later by
> limit->slice_end_time = limit->slice_start_time +
> (uint64_t)(delay_slices * limit->slice_ns);
Ok, on a closer look, if at the start of the function
limit->slice_start_time < now, and
limit->slice_end_time >= now
it seems that in principle what you say can happen.
If it's so, it would be good to know under what conditions this happens,
because this hasn't changed in years.
Berto
Re: [PATCH] ratelimit: restrict the delay time to a non-negative value
On Tue 20 Sep 2022 08:33:50 PM +08, wanglian...@126.com wrote:
> From: Wang Liang
>
> The delay time should never be a negative value.
>
> -return limit->slice_end_time - now;
> +return MAX(limit->slice_end_time - now, 0);
How can this be negative? slice_end_time is guaranteed to be larger than
now:
if (limit->slice_end_time < now) {
/* Previous, possibly extended, time slice finished; reset the
* accounting. */
limit->slice_start_time = now;
limit->slice_end_time = now + limit->slice_ns;
limit->dispatched = 0;
}
Berto
Re: [PATCH v2 02/10] qcow2: compressed read: simplify cluster descriptor passing
On Wed 05 May 2021 08:59:47 AM CEST, Vladimir Sementsov-Ogievskiy wrote: > Let's pass the whole L2 entry and not bother with > L2E_COMPRESSED_OFFSET_SIZE_MASK. > > It also helps further refactoring that adds generic > qcow2_parse_compressed_l2_entry() helper. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 4/5] block: simplify bdrv_child_user_desc()
On Tue 04 May 2021 11:45:09 AM CEST, Vladimir Sementsov-Ogievskiy
wrote:
> All existing parent types (block nodes, block devices, jobs) has the
> realization. So, drop unreachable code.
s/has/have/ , and I'm not sure what "have the realization" means
> static char *bdrv_child_user_desc(BdrvChild *c)
> {
> -if (c->klass->get_parent_desc) {
> -return c->klass->get_parent_desc(c);
> -}
> -
> -return g_strdup("another user");
> +return c->klass->get_parent_desc(c);
> }
Should we also assert(c->klass->get_parent_desc) ?
Berto
Re: [PATCH v2 3/5] block: improve bdrv_child_get_parent_desc()
On Tue 04 May 2021 11:45:08 AM CEST, Vladimir Sementsov-Ogievskiy wrote: > We have different types of parents: block nodes, block backends and > jobs. So, it makes sense to specify type together with name. > > Next, this handler us used to compose an error message about permission > conflict. And permission conflict occurs in a specific place of block > graph. We shouldn't report name of parent device (as it refers another > place in block graph), but exactly and only the name of the node. So, > use bdrv_get_node_name() directly. > > iotest 283 output is updated. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH 5/6] block: simplify bdrv_child_user_desc()
On Mon 03 May 2021 01:34:01 PM CEST, Vladimir Sementsov-Ogievskiy wrote: > All existing parent types (block nodes, block devices, jobs) has the > realization. So, drop unreachable code. > > Signed-off-by: Vladimir Sementsov-Ogievskiy With the updated description that you propose in your reply to the patch, Reviewed-by: Alberto Garcia Berto
Re: [PATCH 3/6] block-backend: improve blk_root_get_parent_desc()
On Mon 03 May 2021 01:33:59 PM CEST, Vladimir Sementsov-Ogievskiy wrote: > We have different types of parents: block nodes, block backends and > jobs. So, it makes sense to specify type together with name. > > While being here also use g_autofree. > > iotest 307 output is updated. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH 2/6] block: bdrv_reopen_multiple(): fix leak of tran object
On Mon 03 May 2021 01:33:58 PM CEST, Vladimir Sementsov-Ogievskiy wrote: > We have one path, where tran object is created, but we don't touch and > don't free it in any way: "goto cleanup" in first loop with calls to > bdrv_flush(). > > Fix it simply moving tran_new() call below that loop. > > Reported-by: Coverity (CID 1452772) > Reported-by: Peter Maydell > Suggested-by: Peter Maydell > Fixes: 72373e40fbc7e4218061a8211384db362d3e7348 > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child
On Mon 03 May 2021 01:33:57 PM CEST, Vladimir Sementsov-Ogievskiy
wrote:
> @@ -2918,12 +2918,18 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
> *child_bs,
> child_role, perm, shared_perm, opaque,
> , tran, errp);
> if (ret < 0) {
> -bdrv_unref(child_bs);
> -return NULL;
> +goto out;
> }
>
> ret = bdrv_refresh_perms(child_bs, errp);
> +if (ret < 0) {
> +goto out;
> +}
> +
> +out:
I see why you're doing this last error check, but it looks a bit
weird. My first reaction was to think that I was missing something.
I would remove it.
Berto
Re: [PATCH v3 09/36] block: bdrv_refresh_perms: check for parents permissions conflict
On Wed 17 Mar 2021 03:35:02 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Add additional check that node parents do not interfere with each > other. This should not hurt existing callers and allows in further > patch use bdrv_refresh_perms() to update a subtree of changed > BdrvChild (check that change is correct). > > New check will substitute bdrv_check_update_perm() in following > permissions refactoring, so keep error messages the same to avoid > unit test result changes. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH v3 05/36] block: BdrvChildClass: add .get_parent_aio_context handler
On Wed 17 Mar 2021 03:34:58 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Add new handler to get aio context and implement it in all child > classes. Add corresponding public interface to be used soon. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH v3 06/36] block: drop ctx argument from bdrv_root_attach_child
On Wed 17 Mar 2021 03:34:59 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Passing parent aio context is redundant, as child_class and parent > opaque pointer are enough to retrieve it. Drop the argument and use new > bdrv_child_get_parent_aio_context() interface. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH v3 04/36] block: bdrv_append(): don't consume reference
On Wed 17 Mar 2021 03:34:57 PM CET, Vladimir Sementsov-Ogievskiy wrote: > We have too much comments for this feature. It seems better just don't > do it. Most of real users (tests don't count) have to create additional > reference. > > Drop also comment in external_snapshot_prepare: > - bdrv_append doesn't "remove" old bs in common sense, it sounds >strange > - the fact that bdrv_append can fail is obvious from the context > - the fact that we must rollback all changes in transaction abort is >known (it's the direct role of abort) > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia > @@ -4645,36 +4640,22 @@ int bdrv_replace_node(BlockDriverState *from, > BlockDriverState *to, > * bs_new must not be attached to a BlockBackend. > * > * This function does not create any image files. > - * > - * bdrv_append() takes ownership of a bs_new reference and unrefs it because > - * that's what the callers commonly need. bs_new will be referenced by the > old > - * parents of bs_top after bdrv_append() returns. If the caller needs to > keep a > - * reference of its own, it must call bdrv_ref(). > */ > int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, > Error **errp) You could still mention explicitly that the old parents of @bs_top will add a new reference to @bs_new. Berto
Re: [PATCH v5 2/6] qcow2: fix cache discarding in update_refcount()
On Fri 26 Mar 2021 09:00:41 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Here refcount of cluster at @cluster_offset reached 0, so we "free" > that cluster. Not a cluster at @offset. The thing that save us from the > bug is that L2 tables and refblocks are discarded one by one. Still, > let's be precise. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen
On Thu 18 Mar 2021 03:25:07 PM CET, Vladimir Sementsov-Ogievskiy
wrote:
>> static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>> BlockReopenQueue *queue,
>> - Transaction *set_backings_tran, Error
>> **errp);
>> + Transaction *tran, Error **errp);
>
> I'd not call it just "tran" to not interfere with transaction
> actions. Of course, reopen should be finally refactored to work
> cleanly on Transaction API, but that is not done yet. And here we pass
> a transaction pointer only to keep children modification.. So, let's
> make it change_child_tran, or something like this.
The name change looks good to me.
>> +} else if (bdrv_recurse_has_child(new_child_bs, bs)) {
>> +error_setg(errp, "Making '%s' a %s of '%s' would create a
>> cycle",
>> + str, parse_file ? "file" : "backing file",
>
> maybe s/"file"/"file child"/
Ok.
>> default:
>> -/* 'backing' does not allow any other data type */
>> +/* The options QDict has been flattened, so 'backing' and 'file'
>> + * do not allow any other data type here. */
>
> checkpatch should complain that you didn't fix style of the comment...
I actually don't like to use the proposed style for 2-line comments in
many cases. I think it makes sense for big comment blocks but adds noise
for shorter comments.
>> +} else {
>> +/*
>> + * Ensure that @bs can really handle backing files, because we are
>> + * about to give it one (or swap the existing one)
>> + */
>> +if (bs->drv->is_filter) {
>> +/* Filters always have a file or a backing child */
>
> Probably we can assert bs->backing, as otherwise backing option should
> be unsupported [preexisting, not about this patch]
Yes, I see that this was added in commit 1d42f48c3a, maybe Max has good
reasons to keep it this way?
>> if (bdrv_is_backing_chain_frozen(overlay_bs,
>> - child_bs(overlay_bs->backing),
>> errp))
>> + bdrv_filter_or_cow_bs(overlay_bs),
>> + errp))
>> {
>> return -EPERM;
>> }
I just realized that this part is probably not ok if you want to change
bs->file on a node that is not a filter, because this would check
bs->backing->frozen and not bs->file->frozen.
>> +if (parse_file) {
>> +/* Store the old file bs, we'll need to refresh its permissions
>> */
>> +reopen_state->old_file_bs = bs->file->bs;
>> +
>> +/* And finally replace the child */
>> +bdrv_replace_child(bs->file, new_child_bs, tran);
>
> I think that actually, we need also to update inherits_from and do
> refresh_limits like in bdrv_set_backing_noperm().
Yes, I think you're right.
> Probably, bdrv_replace_child should do it. Probably not (there are
> still a lot of things to refactor in block.c :)..
>
> Hm. Also, using blockdev-reopen probably means that we are in a
>blockdev word, so we should not care about inherits_from here.
But with blockdev-reopen we do update inherits_from for backing files,
don't we?
> Also, you don't create reopen_state->replace_file_bs, like for
> backing.. On bdrv_reopen_comnmit replace_backing_bs is used to remove
> corresponding options.. Shouldn't we do the same with file options?
I think you're right.
>> -self.reopen(opts, {'file': 'not-found'}, "Cannot change the option
>> 'file'")
>> -self.reopen(opts, {'file': ''}, "Cannot change the option 'file'")
>> +self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor
>> node-name='not-found'")
>
> Interesting that error-message say about device='', not 'not-found'...
That's because 'file' refers to a node name.
Thanks for reviewing,
Berto
Re: [PATCH] tests/unit/test-block-iothread: fix maybe-uninitialized error on GCC 11
On Fri 19 Mar 2021 12:22:18 PM CET, Emanuele Giuseppe Esposito wrote: > When building qemu with GCC 11, test-block-iothread produces the following > warning: > > ../tests/unit/test-block-iothread.c:148:11: error: ‘buf’ may be used > uninitialized [-Werror=maybe-uninitialized] > > This is caused by buf[512] left uninitialized and passed to > bdrv_save_vmstate() that expects a const uint8_t *, so the compiler > assumes it will be read and expects the parameter to be initialized. > > Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Alberto Garcia Berto
[PATCH v4 4/6] block: Support multiple reopening with x-blockdev-reopen
Signed-off-by: Alberto Garcia
---
qapi/block-core.json | 18 +
blockdev.c | 78 +++---
tests/qemu-iotests/155 | 9 +++--
tests/qemu-iotests/165 | 4 +-
tests/qemu-iotests/245 | 27 +++--
tests/qemu-iotests/248 | 2 +-
tests/qemu-iotests/248.out | 2 +-
tests/qemu-iotests/296 | 9 +++--
tests/qemu-iotests/298 | 4 +-
9 files changed, 92 insertions(+), 61 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9f555d5c1d..9150f765da 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4181,13 +4181,15 @@
##
# @x-blockdev-reopen:
#
-# Reopens a block device using the given set of options. Any option
-# not specified will be reset to its default value regardless of its
-# previous status. If an option cannot be changed or a particular
+# Reopens one or more block devices using the given set of options.
+# Any option not specified will be reset to its default value regardless
+# of its previous status. If an option cannot be changed or a particular
# driver does not support reopening then the command will return an
-# error.
+# error. All devices in the list are reopened in one transaction, so
+# if one of them fails then the whole transaction is cancelled.
#
-# The top-level @node-name option (from BlockdevOptions) must be
+# The command receives a list of block devices to reopen. For each one
+# of them, the top-level @node-name option (from BlockdevOptions) must be
# specified and is used to select the block device to be reopened.
# Other @node-name options must be either omitted or set to the
# current name of the appropriate node. This command won't change any
@@ -4207,8 +4209,8 @@
#
# 4) NULL: the current child (if any) is detached.
#
-# Options (1) and (2) are supported in all cases, but at the moment
-# only @backing allows replacing or detaching an existing child.
+# Options (1) and (2) are supported in all cases. Option (3) is
+# supported for @file and @backing, and option (4) for @backing only.
#
# Unlike with blockdev-add, the @backing option must always be present
# unless the node being reopened does not have a backing file and its
@@ -4218,7 +4220,7 @@
# Since: 4.0
##
{ 'command': 'x-blockdev-reopen',
- 'data': 'BlockdevOptions', 'boxed': true }
+ 'data': { 'options': ['BlockdevOptions'] } }
##
# @blockdev-del:
diff --git a/blockdev.c b/blockdev.c
index 825d40aa11..7019397b05 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3580,46 +3580,64 @@ fail:
visit_free(v);
}
-void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
+void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
{
-BlockDriverState *bs;
-AioContext *ctx;
-QObject *obj;
-Visitor *v = qobject_output_visitor_new();
-BlockReopenQueue *queue;
-QDict *qdict;
+BlockReopenQueue *queue = NULL;
+GSList *aio_ctxs = NULL;
+GSList *visitors = NULL;
+GSList *drained = NULL;
-/* Check for the selected node name */
-if (!options->has_node_name) {
-error_setg(errp, "node-name not specified");
-goto fail;
-}
+/* Add each one of the BDS that we want to reopen to the queue */
+for (; reopen_list != NULL; reopen_list = reopen_list->next) {
+BlockdevOptions *options = reopen_list->value;
+BlockDriverState *bs;
+AioContext *ctx;
+QObject *obj;
+Visitor *v;
+QDict *qdict;
-bs = bdrv_find_node(options->node_name);
-if (!bs) {
-error_setg(errp, "Failed to find node with node-name='%s'",
+/* Check for the selected node name */
+if (!options->has_node_name) {
+error_setg(errp, "node-name not specified");
+goto fail;
+}
+
+bs = bdrv_find_node(options->node_name);
+if (!bs) {
+error_setg(errp, "Failed to find node with node-name='%s'",
options->node_name);
-goto fail;
+goto fail;
+}
+
+v = qobject_output_visitor_new();
+visitors = g_slist_prepend(visitors, v);
+
+/* Put all options in a QDict and flatten it */
+visit_type_BlockdevOptions(v, NULL, , _abort);
+visit_complete(v, );
+qdict = qobject_to(QDict, obj);
+
+qdict_flatten(qdict);
+
+ctx = bdrv_get_aio_context(bs);
+if (!g_slist_find(aio_ctxs, ctx)) {
+aio_ctxs = g_slist_prepend(aio_ctxs, ctx);
+aio_context_acquire(ctx);
+}
+bdrv_subtree_drained_begin(bs);
+queue = bdrv_reopen_queue(queue, bs, qdict, false);
+drained = g_slist_prepend(drained, bs);
}
-/* Put all options in a QDict and flatten it */
-visit_type_BlockdevOptions(v, NULL, , _abort);
-visit_complete(v, );
-qdict = qobject_to(QDict, obj);
-
-qdict_flatten(qdict);
-
/* Pe
[PATCH v4 2/6] block: Allow changing bs->file on reopen
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
---
include/block/block.h | 1 +
block.c| 119 ++---
tests/qemu-iotests/245 | 9 ++--
3 files changed, 81 insertions(+), 48 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 5eb1e4cab9..e2732a0187 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -209,6 +209,7 @@ typedef struct BDRVReopenState {
bool backing_missing;
bool replace_backing_bs; /* new_backing_bs is ignored if this is false */
BlockDriverState *old_backing_bs; /* keep pointer for permissions update */
+BlockDriverState *old_file_bs;/* keep pointer for permissions update */
QDict *options;
QDict *explicit_options;
void *opaque;
diff --git a/block.c b/block.c
index 764cdbec7d..8ff0afd77b 100644
--- a/block.c
+++ b/block.c
@@ -98,7 +98,7 @@ static void bdrv_remove_filter_or_cow_child(BlockDriverState
*bs,
static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
- Transaction *set_backings_tran, Error **errp);
+ Transaction *tran, Error **errp);
static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
@@ -4049,6 +4049,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
Error **errp)
refresh_list = bdrv_topological_dfs(refresh_list, found,
state->old_backing_bs);
}
+if (state->old_file_bs) {
+refresh_list = bdrv_topological_dfs(refresh_list, found,
+state->old_file_bs);
+}
}
/*
@@ -4161,65 +4165,77 @@ static bool bdrv_reopen_can_attach(BlockDriverState
*parent,
*
* Return 0 on success, otherwise return < 0 and set @errp.
*/
-static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
- Transaction *set_backings_tran,
- Error **errp)
+static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
+ bool parse_file, Transaction
*tran,
+ Error **errp)
{
BlockDriverState *bs = reopen_state->bs;
-BlockDriverState *overlay_bs, *below_bs, *new_backing_bs;
+BlockDriverState *overlay_bs, *below_bs, *new_child_bs;
+BdrvChild *child = parse_file ? bs->file : bs->backing;
QObject *value;
const char *str;
-value = qdict_get(reopen_state->options, "backing");
+value = qdict_get(reopen_state->options, parse_file ? "file" : "backing");
if (value == NULL) {
return 0;
}
switch (qobject_type(value)) {
case QTYPE_QNULL:
-new_backing_bs = NULL;
+assert(!parse_file); /* The 'file' option does not allow a null value
*/
+new_child_bs = NULL;
break;
case QTYPE_QSTRING:
str = qstring_get_str(qobject_to(QString, value));
-new_backing_bs = bdrv_lookup_bs(NULL, str, errp);
-if (new_backing_bs == NULL) {
+new_child_bs = bdrv_lookup_bs(NULL, str, errp);
+if (new_child_bs == NULL) {
return -EINVAL;
-} else if (bdrv_recurse_has_child(new_backing_bs, bs)) {
-error_setg(errp, "Making '%s' a backing file of '%s' "
- "would create a cycle", str, bs->node_name);
+} else if (bdrv_recurse_has_child(new_child_bs, bs)) {
+error_setg(errp, "Making '%s' a %s of '%s' would create a cycle",
+ str, parse_file ? "file" : "backing file",
+ bs->node_name);
return -EINVAL;
}
break;
default:
-/* 'backing' does not allow any other data type */
+/* The options QDict has been flattened, so 'backing' and 'file'
+ * do not allow any other data type here. */
g_assert_not_reached();
}
-/*
- * Check AioContext compatibility so that the bdrv_set_backing_hd() call in
- * bdrv_reopen_commit() won't fail.
- */
-if (new_backing_bs) {
-if (!bdrv_reopen_can_attach(bs, bs->backing, new_backing_bs, errp)) {
+/* If 'file' poi
[PATCH v4 6/6] block: Make blockdev-reopen stable API
This patch drops the 'x-' prefix from x-blockdev-reopen.
Signed-off-by: Alberto Garcia
---
qapi/block-core.json | 6 +++---
blockdev.c | 2 +-
tests/qemu-iotests/155 | 2 +-
tests/qemu-iotests/165 | 2 +-
tests/qemu-iotests/245 | 10 +-
tests/qemu-iotests/248 | 2 +-
tests/qemu-iotests/248.out | 2 +-
tests/qemu-iotests/296 | 2 +-
tests/qemu-iotests/298 | 2 +-
9 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9150f765da..93fb0606e9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4179,7 +4179,7 @@
{ 'command': 'blockdev-add', 'data': 'BlockdevOptions', 'boxed': true }
##
-# @x-blockdev-reopen:
+# @blockdev-reopen:
#
# Reopens one or more block devices using the given set of options.
# Any option not specified will be reset to its default value regardless
@@ -4217,9 +4217,9 @@
# image does not have a default backing file name as part of its
# metadata.
#
-# Since: 4.0
+# Since: 6.0
##
-{ 'command': 'x-blockdev-reopen',
+{ 'command': 'blockdev-reopen',
'data': { 'options': ['BlockdevOptions'] } }
##
diff --git a/blockdev.c b/blockdev.c
index 7019397b05..d697db8417 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3580,7 +3580,7 @@ fail:
visit_free(v);
}
-void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
+void qmp_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
{
BlockReopenQueue *queue = NULL;
GSList *aio_ctxs = NULL;
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index 3400b0312a..fec43d662d 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -261,7 +261,7 @@ class TestBlockdevMirrorReopen(MirrorBaseClass):
result = self.vm.qmp('blockdev-add', node_name="backing",
driver="null-co")
self.assert_qmp(result, 'return', {})
-result = self.vm.qmp('x-blockdev-reopen', options = [{
+result = self.vm.qmp('blockdev-reopen', options = [{
'node-name': "target",
'driver': iotests.imgfmt,
'file': "target-file",
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index 57aa88ecae..92a431315b 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -137,7 +137,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
assert sha256_1 == self.getSha256()
# Reopen to RW
-result = self.vm.qmp('x-blockdev-reopen', options = [{
+result = self.vm.qmp('blockdev-reopen', options = [{
'node-name': 'node0',
'driver': iotests.imgfmt,
'file': {
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 1a4413e6ab..e7498b5b3e 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1,7 +1,7 @@
#!/usr/bin/env python3
# group: rw
#
-# Test cases for the QMP 'x-blockdev-reopen' command
+# Test cases for the QMP 'blockdev-reopen' command
#
# Copyright (C) 2018-2019 Igalia, S.L.
# Author: Alberto Garcia
@@ -85,16 +85,16 @@ class TestBlockdevReopen(iotests.QMPTestCase):
"Expected output of %d qemu-io commands, found %d" %
(found, self.total_io_cmds))
-# Run x-blockdev-reopen on a list of block devices
+# Run blockdev-reopen on a list of block devices
def reopenMultiple(self, opts, errmsg = None):
-result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options =
opts)
+result = self.vm.qmp('blockdev-reopen', conv_keys = False, options =
opts)
if errmsg:
self.assert_qmp(result, 'error/class', 'GenericError')
self.assert_qmp(result, 'error/desc', errmsg)
else:
self.assert_qmp(result, 'return', {})
-# Run x-blockdev-reopen on a single block device (specified by
+# Run blockdev-reopen on a single block device (specified by
# 'opts') but applying 'newopts' on top of it. The original 'opts'
# dict is unmodified
def reopen(self, opts, newopts = {}, errmsg = None):
@@ -161,7 +161,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.reopen(opts, {'file.locking': 'off'}, "Cannot change the option
'locking'")
self.reopen(opts, {'file.filename': None}, "Invalid parameter type for
'options[0].file.filename', expected: string")
-# node-name is optional in BlockdevOptions, but x-blockdev-reopen
needs it
+# node-name is optional in BlockdevOptions, but blockdev-reopen needs
it
del opts['node-name']
self.reopen(opts, {}, "node-name not specified")
diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248
index 03911333c4..2ec2416e8a 100755
--- a/tests/qemu-iotests/248
+++ b/tests/q
[PATCH v4 0/6] Allow changing bs->file on reopen
Based-on: <20210317143529.615584-1-vsement...@virtuozzo.com> Hello, this is the same as v3, but rebased on top of Vladimir's "block: update graph permissions update v3", which you can get here: git: https://src.openvz.org/scm/~vsementsov/qemu.git tag: up-block-topologic-perm-v3 Tip: you may find it easier to review patch 4 if you use 'git diff -w' since a big part of the changes that you see in qmp_x_blockdev_reopen() are just indentation changes. Berto v4: - Rebase on top of version 3 of Vladimir's branch v3: https://lists.gnu.org/archive/html/qemu-block/2021-03/msg00553.html v2: https://lists.gnu.org/archive/html/qemu-block/2021-02/msg00623.html v1: https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00437.html Output of git backport-diff against v3: Key: [] : patches are identical [] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/6:[] [--] 'block: Add bdrv_reopen_queue_free()' 002/6:[0018] [FC] 'block: Allow changing bs->file on reopen' 003/6:[] [--] 'iotests: Test replacing files with x-blockdev-reopen' 004/6:[0071] [FC] 'block: Support multiple reopening with x-blockdev-reopen' 005/6:[] [--] 'iotests: Test reopening multiple devices at the same time' 006/6:[] [-C] 'block: Make blockdev-reopen stable API' Alberto Garcia (6): block: Add bdrv_reopen_queue_free() block: Allow changing bs->file on reopen iotests: Test replacing files with x-blockdev-reopen block: Support multiple reopening with x-blockdev-reopen iotests: Test reopening multiple devices at the same time block: Make blockdev-reopen stable API qapi/block-core.json | 24 ++--- include/block/block.h | 2 + block.c| 135 -- blockdev.c | 78 +-- tests/qemu-iotests/155 | 9 +- tests/qemu-iotests/165 | 4 +- tests/qemu-iotests/245 | 190 + tests/qemu-iotests/245.out | 11 ++- tests/qemu-iotests/248 | 4 +- tests/qemu-iotests/248.out | 2 +- tests/qemu-iotests/296 | 11 ++- tests/qemu-iotests/298 | 4 +- 12 files changed, 351 insertions(+), 123 deletions(-) -- 2.20.1
[PATCH v4 1/6] block: Add bdrv_reopen_queue_free()
Move the code to free a BlockReopenQueue to a separate function.
It will be used in a subsequent patch.
Signed-off-by: Alberto Garcia
---
include/block/block.h | 1 +
block.c | 16
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 8d5b3ecebd..5eb1e4cab9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -385,6 +385,7 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv,
const char *node_name,
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
BlockDriverState *bs, QDict *options,
bool keep_old_opts);
+void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue);
int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp);
diff --git a/block.c b/block.c
index 95d8246d92..764cdbec7d 100644
--- a/block.c
+++ b/block.c
@@ -3985,6 +3985,17 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue
*bs_queue,
NULL, 0, keep_old_opts);
}
+void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue)
+{
+if (bs_queue) {
+BlockReopenQueueEntry *bs_entry, *next;
+QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+g_free(bs_entry);
+}
+g_free(bs_queue);
+}
+}
+
/*
* Reopen multiple BlockDriverStates atomically & transactionally.
*
@@ -4088,10 +4099,7 @@ abort:
}
cleanup:
-QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
-g_free(bs_entry);
-}
-g_free(bs_queue);
+bdrv_reopen_queue_free(bs_queue);
return ret;
}
--
2.20.1
[PATCH v4 3/6] iotests: Test replacing files with x-blockdev-reopen
This patch adds new tests in which we use x-blockdev-reopen to change
bs->file
Signed-off-by: Alberto Garcia
---
tests/qemu-iotests/245 | 109 -
tests/qemu-iotests/245.out | 11 +++-
2 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index a4d0b10e9d..caebef4ac8 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -79,7 +79,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
for line in log.split("\n"):
if line.startswith("Pattern verification failed"):
raise Exception("%s (command #%d)" % (line, found))
-if re.match("read .*/.* bytes at offset", line):
+if re.match("(read|wrote) .*/.* bytes at offset", line):
found += 1
self.assertEqual(found, self.total_io_cmds,
"Expected output of %d qemu-io commands, found %d" %
@@ -537,6 +537,113 @@ class TestBlockdevReopen(iotests.QMPTestCase):
result = self.vm.qmp('blockdev-del', conv_keys = True, node_name =
'bv')
self.assert_qmp(result, 'return', {})
+# Replace the protocol layer ('file' parameter) of a disk image
+def test_replace_file(self):
+# Create two small raw images and add them to a running VM
+qemu_img('create', '-f', 'raw', hd_path[0], '10k')
+qemu_img('create', '-f', 'raw', hd_path[1], '10k')
+
+hd0_opts = {'driver': 'file', 'node-name': 'hd0-file', 'filename':
hd_path[0] }
+hd1_opts = {'driver': 'file', 'node-name': 'hd1-file', 'filename':
hd_path[1] }
+
+result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts)
+self.assert_qmp(result, 'return', {})
+result = self.vm.qmp('blockdev-add', conv_keys = False, **hd1_opts)
+self.assert_qmp(result, 'return', {})
+
+# Add a raw format layer that uses hd0-file as its protocol layer
+opts = {'driver': 'raw', 'node-name': 'hd', 'file': 'hd0-file'}
+
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+# Fill the image with data
+self.run_qemu_io("hd", "read -P 0 0 10k")
+self.run_qemu_io("hd", "write -P 0xa0 0 10k")
+
+# Replace hd0-file with hd1-file and fill it with (different) data
+self.reopen(opts, {'file': 'hd1-file'})
+self.run_qemu_io("hd", "read -P 0 0 10k")
+self.run_qemu_io("hd", "write -P 0xa1 0 10k")
+
+# Use hd0-file again and check that it contains the expected data
+self.reopen(opts, {'file': 'hd0-file'})
+self.run_qemu_io("hd", "read -P 0xa0 0 10k")
+
+# And finally do the same with hd1-file
+self.reopen(opts, {'file': 'hd1-file'})
+self.run_qemu_io("hd", "read -P 0xa1 0 10k")
+
+# Insert (and remove) a throttle filter
+def test_insert_throttle_filter(self):
+# Add an image to the VM
+hd0_opts = hd_opts(0)
+result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts)
+self.assert_qmp(result, 'return', {})
+
+# Create a throttle-group object
+opts = { 'qom-type': 'throttle-group', 'id': 'group0',
+ 'limits': { 'iops-total': 1000 } }
+result = self.vm.qmp('object-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+# Add a throttle filter with the group that we just created.
+# The filter is not used by anyone yet
+opts = { 'driver': 'throttle', 'node-name': 'throttle0',
+ 'throttle-group': 'group0', 'file': 'hd0-file' }
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+# Insert the throttle filter between hd0 and hd0-file
+self.reopen(hd0_opts, {'file': 'throttle0'})
+
+# Remove the throttle filter from hd0
+self.reopen(hd0_opts, {'file': 'hd0-file'})
+
+# Insert (and remove) a compress filter
+def test_insert_compress_filter(self):
+# Add an image to the VM: hd (raw) -> hd0 (qcow2) -> hd0-file (file)
+opts = {'driver': 'raw', 'node-name': 'hd', 'file': hd_opts(0)}
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+# Add a 'compress' filter
+filter_opts = {'driver': 'compress',
+ 'node-name': 'compress0',
+ 'file': 'hd0'}
+result = self.vm.qmp('blockdev-add', conv_keys = False, **filter_opts)
+self.assert_qmp(result, 'return', {})
+
+# Unmap the beginning of the image (we cannot write compressed
+# data to an allocated clus
[PATCH v4 5/6] iotests: Test reopening multiple devices at the same time
This test swaps the images used by two active block devices.
This is now possible thanks to the new ability to run
x-blockdev-reopen on multiple devices at the same time.
Signed-off-by: Alberto Garcia
---
tests/qemu-iotests/245 | 41 ++
tests/qemu-iotests/245.out | 4 ++--
2 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index c27557fa6b..1a4413e6ab 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -649,6 +649,47 @@ class TestBlockdevReopen(iotests.QMPTestCase):
'-c', 'read -P 0x40 0x40008 1',
'-c', 'read -P 0x80 0x40010 1',
hd_path[0])
+# Swap the disk images of two active block devices
+def test_swap_files(self):
+# Add hd0 and hd2 (none of them with backing files)
+opts0 = hd_opts(0)
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts0)
+self.assert_qmp(result, 'return', {})
+
+opts2 = hd_opts(2)
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts2)
+self.assert_qmp(result, 'return', {})
+
+# Write different data to both block devices
+self.run_qemu_io("hd0", "write -P 0xa0 0 1k")
+self.run_qemu_io("hd2", "write -P 0xa2 0 1k")
+
+# Check that the data reads correctly
+self.run_qemu_io("hd0", "read -P 0xa0 0 1k")
+self.run_qemu_io("hd2", "read -P 0xa2 0 1k")
+
+# It's not possible to make a block device use an image that
+# is already being used by the other device.
+self.reopen(opts0, {'file': 'hd2-file'},
+"Conflicts with use by hd0 as 'file', which does not allow
'write, resize' on hd2-file")
+self.reopen(opts2, {'file': 'hd0-file'},
+"Conflicts with use by hd2 as 'file', which does not allow
'write, resize' on hd0-file")
+
+# But we can swap the images if we reopen both devices at the
+# same time
+opts0['file'] = 'hd2-file'
+opts2['file'] = 'hd0-file'
+self.reopenMultiple([opts0, opts2])
+self.run_qemu_io("hd0", "read -P 0xa2 0 1k")
+self.run_qemu_io("hd2", "read -P 0xa0 0 1k")
+
+# And we can of course come back to the original state
+opts0['file'] = 'hd0-file'
+opts2['file'] = 'hd2-file'
+self.reopenMultiple([opts0, opts2])
+self.run_qemu_io("hd0", "read -P 0xa0 0 1k")
+self.run_qemu_io("hd2", "read -P 0xa2 0 1k")
+
# Misc reopen tests with different block drivers
@iotests.skip_if_unsupported(['quorum', 'throttle'])
def test_misc_drivers(self):
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 6ea1b2798f..c4230b51d1 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -17,8 +17,8 @@ read 1/1 bytes at offset 262152
read 1/1 bytes at offset 262160
1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
+.
--
-Ran 24 tests
+Ran 25 tests
OK
--
2.20.1
Re: [PATCH v3 13/36] block: use topological sort for permission update
On Wed 17 Mar 2021 03:35:06 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Rewrite bdrv_check_perm(), bdrv_abort_perm_update() and bdrv_set_perm() > to update nodes in topological sort order instead of simple DFS. With > topologically sorted nodes, we update a node only when all its parents > already updated. With DFS it's not so. This patch does not apply cleanly on the master branch, the branch from your repository at https://src.openvz.org/scm/~vsementsov/qemu.git does work fine, however. Berto
Re: [PATCH] stream: Don't crash when node permission is denied
On Tue 09 Mar 2021 06:34:51 PM CET, Kevin Wolf wrote: > The image streaming block job restricts shared permissions of the nodes > it accesses. This can obviously fail when other users already got these > permissions. _abort is therefore wrong and can crash. Handle these > errors gracefully and just fail starting the block job. > > Reported-by: Nini Gu > Signed-off-by: Kevin Wolf Reviewed-by: Alberto Garcia Berto
[PATCH v3 0/6] Allow changing bs->file on reopen
Based-on: <20201127144522.29991-1-vsement...@virtuozzo.com> Hello, here's the third version of the patches that allow replacing bs->file using (x-)blockdev-reopen. You can read more details here: https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00437.html In summary, the series does three things: - Allows replacing bs->file - Allows reopening multiple block devices with one single command. - Drops the x- prefix from the command name. This is still depending on Vladimir's "update graph permissions update" branch. Regards, Berto v3: - Patch 1: Move bdrv_reopen_queue_free() to a new patch - Patch 2: Merge bdrv_reopen_parse_backing() and bdrv_reopen_parse_file() - Patch 3: Add more tests - Patch 4: Update documentation and fix iotest 296 - Patch 5: Minor updates to iotest 245 - Patch 6: New patch, drop the 'x-' prefix from x-blockdev-reopen v2: https://lists.gnu.org/archive/html/qemu-block/2021-02/msg00623.html v1: https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00437.html Output of git backport-diff against v2: Key: [] : patches are identical [] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/6:[down] 'block: Add bdrv_reopen_queue_free()' 002/6:[0160] [FC] 'block: Allow changing bs->file on reopen' 003/6:[down] 'iotests: Test replacing files with x-blockdev-reopen' 004/6:[0042] [FC] 'block: Support multiple reopening with x-blockdev-reopen' 005/6:[0015] [FC] 'iotests: Test reopening multiple devices at the same time' 006/6:[down] 'block: Make blockdev-reopen stable API' Alberto Garcia (6): block: Add bdrv_reopen_queue_free() block: Allow changing bs->file on reopen iotests: Test replacing files with x-blockdev-reopen block: Support multiple reopening with x-blockdev-reopen iotests: Test reopening multiple devices at the same time block: Make blockdev-reopen stable API qapi/block-core.json | 24 ++--- include/block/block.h | 2 + block.c| 137 -- blockdev.c | 85 + tests/qemu-iotests/155 | 9 +- tests/qemu-iotests/165 | 4 +- tests/qemu-iotests/245 | 190 + tests/qemu-iotests/245.out | 11 ++- tests/qemu-iotests/248 | 4 +- tests/qemu-iotests/248.out | 2 +- tests/qemu-iotests/296 | 11 ++- tests/qemu-iotests/298 | 4 +- 12 files changed, 351 insertions(+), 132 deletions(-) -- 2.20.1
[PATCH v3 4/6] block: Support multiple reopening with x-blockdev-reopen
Signed-off-by: Alberto Garcia
---
qapi/block-core.json | 18
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/296 | 9 ++--
tests/qemu-iotests/298 | 4 +-
9 files changed, 91 insertions(+), 69 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c0e7c23331..0363c901b7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4140,13 +4140,15 @@
##
# @x-blockdev-reopen:
#
-# Reopens a block device using the given set of options. Any option
-# not specified will be reset to its default value regardless of its
-# previous status. If an option cannot be changed or a particular
+# Reopens one or more block devices using the given set of options.
+# Any option not specified will be reset to its default value regardless
+# of its previous status. If an option cannot be changed or a particular
# driver does not support reopening then the command will return an
-# error.
+# error. All devices in the list are reopened in one transaction, so
+# if one of them fails then the whole transaction is cancelled.
#
-# The top-level @node-name option (from BlockdevOptions) must be
+# The command receives a list of block devices to reopen. For each one
+# of them, the top-level @node-name option (from BlockdevOptions) must be
# specified and is used to select the block device to be reopened.
# Other @node-name options must be either omitted or set to the
# current name of the appropriate node. This command won't change any
@@ -4166,8 +4168,8 @@
#
# 4) NULL: the current child (if any) is detached.
#
-# Options (1) and (2) are supported in all cases, but at the moment
-# only @backing allows replacing or detaching an existing child.
+# Options (1) and (2) are supported in all cases. Option (3) is
+# supported for @file and @backing, and option (4) for @backing only.
#
# Unlike with blockdev-add, the @backing option must always be present
# unless the node being reopened does not have a backing file and its
@@ -4177,7 +4179,7 @@
# Since: 4.0
##
{ 'command': 'x-blockdev-reopen',
- 'data': 'BlockdevOptions', 'boxed': true }
+ 'data': { 'options': ['BlockdevOptions'] } }
##
# @blockdev-del:
diff --git a/blockdev.c b/blockdev.c
index 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();
-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, , _abort);
-visit_complete(v, );
-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_bs; it_bs = bdrv_next()) {
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();
+visitors = g_slist_prepend(visitors, v);
+
+/* Put all options in a QDict and flatten it */
+visit_type_BlockdevOptions(v, NULL, , _abort);
+
[PATCH v3 6/6] block: Make blockdev-reopen stable API
This patch drops the 'x-' prefix from x-blockdev-reopen.
Signed-off-by: Alberto Garcia
---
qapi/block-core.json | 6 +++---
blockdev.c | 2 +-
tests/qemu-iotests/155 | 2 +-
tests/qemu-iotests/165 | 2 +-
tests/qemu-iotests/245 | 10 +-
tests/qemu-iotests/248 | 2 +-
tests/qemu-iotests/248.out | 2 +-
tests/qemu-iotests/296 | 2 +-
tests/qemu-iotests/298 | 2 +-
9 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0363c901b7..c9ef9fdebb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4138,7 +4138,7 @@
{ 'command': 'blockdev-add', 'data': 'BlockdevOptions', 'boxed': true }
##
-# @x-blockdev-reopen:
+# @blockdev-reopen:
#
# Reopens one or more block devices using the given set of options.
# Any option not specified will be reset to its default value regardless
@@ -4176,9 +4176,9 @@
# image does not have a default backing file name as part of its
# metadata.
#
-# Since: 4.0
+# Since: 6.0
##
-{ 'command': 'x-blockdev-reopen',
+{ 'command': 'blockdev-reopen',
'data': { 'options': ['BlockdevOptions'] } }
##
diff --git a/blockdev.c b/blockdev.c
index 6b688c0f73..ccc7978665 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3528,7 +3528,7 @@ fail:
visit_free(v);
}
-void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
+void qmp_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
{
BlockReopenQueue *queue = NULL;
GSList *aio_ctxs = NULL;
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index 5271f9541f..eb9b3c68ac 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -260,7 +260,7 @@ class TestBlockdevMirrorReopen(MirrorBaseClass):
result = self.vm.qmp('blockdev-add', node_name="backing",
driver="null-co")
self.assert_qmp(result, 'return', {})
-result = self.vm.qmp('x-blockdev-reopen', options = [{
+result = self.vm.qmp('blockdev-reopen', options = [{
'node-name': "target",
'driver': iotests.imgfmt,
'file': "target-file",
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index 32db5086e1..d76d5036f1 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', options = [{
+result = self.vm.qmp('blockdev-reopen', options = [{
'node-name': 'node0',
'driver': iotests.imgfmt,
'file': {
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index fb11eb146d..57ff4d846f 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1,6 +1,6 @@
#!/usr/bin/env python3
#
-# Test cases for the QMP 'x-blockdev-reopen' command
+# Test cases for the QMP 'blockdev-reopen' command
#
# Copyright (C) 2018-2019 Igalia, S.L.
# Author: Alberto Garcia
@@ -84,16 +84,16 @@ class TestBlockdevReopen(iotests.QMPTestCase):
"Expected output of %d qemu-io commands, found %d" %
(found, self.total_io_cmds))
-# Run x-blockdev-reopen on a list of block devices
+# Run blockdev-reopen on a list of block devices
def reopenMultiple(self, opts, errmsg = None):
-result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options =
opts)
+result = self.vm.qmp('blockdev-reopen', conv_keys = False, options =
opts)
if errmsg:
self.assert_qmp(result, 'error/class', 'GenericError')
self.assert_qmp(result, 'error/desc', errmsg)
else:
self.assert_qmp(result, 'return', {})
-# Run x-blockdev-reopen on a single block device (specified by
+# Run blockdev-reopen on a single block device (specified by
# 'opts') but applying 'newopts' on top of it. The original 'opts'
# dict is unmodified
def reopen(self, opts, newopts = {}, errmsg = None):
@@ -160,7 +160,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.reopen(opts, {'file.locking': 'off'}, "Cannot change the option
'locking'")
self.reopen(opts, {'file.filename': None}, "Invalid parameter type for
'options[0].file.filename', expected: string")
-# node-name is optional in BlockdevOptions, but x-blockdev-reopen
needs it
+# node-name is optional in BlockdevOptions, but blockdev-reopen needs
it
del opts['node-name']
self.reopen(opts, {}, "Node name not specified")
diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248
index 2b43853183..0da519d79f 100755
--- a/tests/qemu-iotests/248
+++ b/tests/q
[PATCH v3 1/6] block: Add bdrv_reopen_queue_free()
Move the code to free a BlockReopenQueue to a separate function.
It will be used in a subsequent patch.
Signed-off-by: Alberto Garcia
---
include/block/block.h | 1 +
block.c | 16
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 82271d9ccd..aa3d568fec 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -371,6 +371,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 576b145cbf..03b36cd5df 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.
*
@@ -4020,10 +4031,7 @@ abort:
}
cleanup:
-QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
-g_free(bs_entry);
-}
-g_free(bs_queue);
+bdrv_reopen_queue_free(bs_queue);
return ret;
}
--
2.20.1
[PATCH v3 5/6] iotests: Test reopening multiple devices at the same time
This test swaps the images used by two active block devices.
This is now possible thanks to the new ability to run
x-blockdev-reopen on multiple devices at the same time.
Signed-off-by: Alberto Garcia
---
tests/qemu-iotests/245 | 41 ++
tests/qemu-iotests/245.out | 4 ++--
2 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 53281228bc..fb11eb146d 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -648,6 +648,47 @@ class TestBlockdevReopen(iotests.QMPTestCase):
'-c', 'read -P 0x40 0x40008 1',
'-c', 'read -P 0x80 0x40010 1',
hd_path[0])
+# Swap the disk images of two active block devices
+def test_swap_files(self):
+# Add hd0 and hd2 (none of them with backing files)
+opts0 = hd_opts(0)
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts0)
+self.assert_qmp(result, 'return', {})
+
+opts2 = hd_opts(2)
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts2)
+self.assert_qmp(result, 'return', {})
+
+# Write different data to both block devices
+self.run_qemu_io("hd0", "write -P 0xa0 0 1k")
+self.run_qemu_io("hd2", "write -P 0xa2 0 1k")
+
+# Check that the data reads correctly
+self.run_qemu_io("hd0", "read -P 0xa0 0 1k")
+self.run_qemu_io("hd2", "read -P 0xa2 0 1k")
+
+# It's not possible to make a block device use an image that
+# is already being used by the other device.
+self.reopen(opts0, {'file': 'hd2-file'},
+"Conflicts with use by hd0 as 'file', which does not allow
'write, resize' on hd2-file")
+self.reopen(opts2, {'file': 'hd0-file'},
+"Conflicts with use by hd2 as 'file', which does not allow
'write, resize' on hd0-file")
+
+# But we can swap the images if we reopen both devices at the
+# same time
+opts0['file'] = 'hd2-file'
+opts2['file'] = 'hd0-file'
+self.reopenMultiple([opts0, opts2])
+self.run_qemu_io("hd0", "read -P 0xa2 0 1k")
+self.run_qemu_io("hd2", "read -P 0xa0 0 1k")
+
+# And we can of course come back to the original state
+opts0['file'] = 'hd0-file'
+opts2['file'] = 'hd2-file'
+self.reopenMultiple([opts0, opts2])
+self.run_qemu_io("hd0", "read -P 0xa0 0 1k")
+self.run_qemu_io("hd2", "read -P 0xa2 0 1k")
+
# Misc reopen tests with different block drivers
@iotests.skip_if_unsupported(['quorum', 'throttle'])
def test_misc_drivers(self):
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 6ea1b2798f..c4230b51d1 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -17,8 +17,8 @@ read 1/1 bytes at offset 262152
read 1/1 bytes at offset 262160
1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
+.
--
-Ran 24 tests
+Ran 25 tests
OK
--
2.20.1
[PATCH v3 2/6] block: Allow changing bs->file on reopen
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
---
include/block/block.h | 1 +
block.c| 121 ++---
tests/qemu-iotests/245 | 9 +--
3 files changed, 82 insertions(+), 49 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index aa3d568fec..fe4a220da9 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 03b36cd5df..bce5d8a69c 100644
--- a/block.c
+++ b/block.c
@@ -106,7 +106,7 @@ static void bdrv_remove_backing(BlockDriverState *bs,
GSList **tran);
static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
- GSList **set_backings_tran, Error **errp);
+ GSList **tran, Error **errp);
static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
@@ -3989,6 +3989,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, , errp);
@@ -4093,65 +4097,77 @@ static bool bdrv_reopen_can_attach(BlockDriverState
*parent,
*
* Return 0 on success, otherwise return < 0 and set @errp.
*/
-static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
- GSList **set_backings_tran,
- Error **errp)
+static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
+ bool parse_file, GSList **tran,
+ Error **errp)
{
BlockDriverState *bs = reopen_state->bs;
-BlockDriverState *overlay_bs, *below_bs, *new_backing_bs;
+BlockDriverState *overlay_bs, *below_bs, *new_child_bs;
+BdrvChild *child = parse_file ? bs->file : bs->backing;
QObject *value;
const char *str;
-value = qdict_get(reopen_state->options, "backing");
+value = qdict_get(reopen_state->options, parse_file ? "file" : "backing");
if (value == NULL) {
return 0;
}
switch (qobject_type(value)) {
case QTYPE_QNULL:
-new_backing_bs = NULL;
+assert(!parse_file); /* The 'file' option does not allow a null value
*/
+new_child_bs = NULL;
break;
case QTYPE_QSTRING:
-str = qobject_get_try_str(value);
-new_backing_bs = bdrv_lookup_bs(NULL, str, errp);
-if (new_backing_bs == NULL) {
+str = qstring_get_str(qobject_to(QString, value));
+new_child_bs = bdrv_lookup_bs(NULL, str, errp);
+if (new_child_bs == NULL) {
return -EINVAL;
-} else if (bdrv_recurse_has_child(new_backing_bs, bs)) {
-error_setg(errp, "Making '%s' a backing file of '%s' "
- "would create a cycle", str, bs->node_name);
+} else if (bdrv_recurse_has_child(new_child_bs, bs)) {
+error_setg(errp, "Making '%s' a %s of '%s' would create a cycle",
+ str, parse_file ? "file" : "backing file",
+ bs->node_name);
return -EINVAL;
}
break;
default:
-/* 'backing' does not allow any other data type */
+/* The options QDict has been flattened, so 'backing' and 'file'
+ * do not allow any other data type here. */
g_assert_not_reached();
}
-/*
- * Check AioContext compatibility so that the bdrv_set_backing_hd() call in
- * bdrv_reopen_commit() won't fail.
- */
-if (new_backing_bs) {
-if
[PATCH v3 3/6] iotests: Test replacing files with x-blockdev-reopen
This patch adds new tests in which we use x-blockdev-reopen to change
bs->file
Signed-off-by: Alberto Garcia
---
tests/qemu-iotests/245 | 109 -
tests/qemu-iotests/245.out | 11 +++-
2 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 52c2ed7c2d..1549a42e2b 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,113 @@ class TestBlockdevReopen(iotests.QMPTestCase):
result = self.vm.qmp('blockdev-del', conv_keys = True, node_name =
'bv')
self.assert_qmp(result, 'return', {})
+# Replace the protocol layer ('file' parameter) of a disk image
+def test_replace_file(self):
+# Create two small raw images and add them to a running VM
+qemu_img('create', '-f', 'raw', hd_path[0], '10k')
+qemu_img('create', '-f', 'raw', hd_path[1], '10k')
+
+hd0_opts = {'driver': 'file', 'node-name': 'hd0-file', 'filename':
hd_path[0] }
+hd1_opts = {'driver': 'file', 'node-name': 'hd1-file', 'filename':
hd_path[1] }
+
+result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts)
+self.assert_qmp(result, 'return', {})
+result = self.vm.qmp('blockdev-add', conv_keys = False, **hd1_opts)
+self.assert_qmp(result, 'return', {})
+
+# Add a raw format layer that uses hd0-file as its protocol layer
+opts = {'driver': 'raw', 'node-name': 'hd', 'file': 'hd0-file'}
+
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+# Fill the image with data
+self.run_qemu_io("hd", "read -P 0 0 10k")
+self.run_qemu_io("hd", "write -P 0xa0 0 10k")
+
+# Replace hd0-file with hd1-file and fill it with (different) data
+self.reopen(opts, {'file': 'hd1-file'})
+self.run_qemu_io("hd", "read -P 0 0 10k")
+self.run_qemu_io("hd", "write -P 0xa1 0 10k")
+
+# Use hd0-file again and check that it contains the expected data
+self.reopen(opts, {'file': 'hd0-file'})
+self.run_qemu_io("hd", "read -P 0xa0 0 10k")
+
+# And finally do the same with hd1-file
+self.reopen(opts, {'file': 'hd1-file'})
+self.run_qemu_io("hd", "read -P 0xa1 0 10k")
+
+# Insert (and remove) a throttle filter
+def test_insert_throttle_filter(self):
+# Add an image to the VM
+hd0_opts = hd_opts(0)
+result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts)
+self.assert_qmp(result, 'return', {})
+
+# Create a throttle-group object
+opts = { 'qom-type': 'throttle-group', 'id': 'group0',
+ 'limits': { 'iops-total': 1000 } }
+result = self.vm.qmp('object-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+# Add a throttle filter with the group that we just created.
+# The filter is not used by anyone yet
+opts = { 'driver': 'throttle', 'node-name': 'throttle0',
+ 'throttle-group': 'group0', 'file': 'hd0-file' }
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+# Insert the throttle filter between hd0 and hd0-file
+self.reopen(hd0_opts, {'file': 'throttle0'})
+
+# Remove the throttle filter from hd0
+self.reopen(hd0_opts, {'file': 'hd0-file'})
+
+# Insert (and remove) a compress filter
+def test_insert_compress_filter(self):
+# Add an image to the VM: hd (raw) -> hd0 (qcow2) -> hd0-file (file)
+opts = {'driver': 'raw', 'node-name': 'hd', 'file': hd_opts(0)}
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+# Add a 'compress' filter
+filter_opts = {'driver': 'compress',
+ 'node-name': 'compress0',
+ 'file': 'hd0'}
+result = self.vm.qmp('blockdev-add', conv_keys = False, **filter_opts)
+self.assert_qmp(result, 'return', {})
+
+# Unmap the beginning of the image (we cannot write compressed
+# data to an allocated clus
Re: block/throttle and burst bucket
On Mon 01 Mar 2021 01:11:55 PM CET, Peter Lieven wrote:
> Why we talk about throttling I still do not understand the following part in
> util/throttle.c function throttle_compute_wait
>
>
> if (!bkt->max) {
> /* If bkt->max is 0 we still want to allow short bursts of I/O
> * from the guest, otherwise every other request will be throttled
> * and performance will suffer considerably. */
> bucket_size = (double) bkt->avg / 10;
> burst_bucket_size = 0;
> } else {
> /* If we have a burst limit then we have to wait until all I/O
> * at burst rate has finished before throttling to bkt->avg */
> bucket_size = bkt->max * bkt->burst_length;
> burst_bucket_size = (double) bkt->max / 10;
> }
>
>
> Why burst_bucket_size = bkt->max / 10?
>
> From what I understand it should be bkt->max. Otherwise we compare the
> "extra" against a tenth of the bucket capacity
1) bkt->max is the burst rate in bytes/second [*]
2) burst_bucket_size is used to decide when to start throttling (you can
see the code at the end of throttle_compute_wait()).
The important thing is that burst_bucket_size does not actually have an
influence on the actual burst rate. Increasing that value is not going
to make the I/O faster, it just means that I/O will be throttled later.
Once the I/O is throttled, the actual burst rate is define by how quick
the burst bucket leaks (see throttle_leak_bucket()).
The higher burst_bucket_size is, the longer we allow the guest to exceed
the maximum rate. So we divide blk->max by 10 in order to allow the
guest to perform 100ms' worth of I/O without being throttled.
See the commit message of 0770a7a6466cc2dbf4ac91841173ad4488e1fbc7 for
more details.
Berto
Re: [RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen
On Wed 24 Feb 2021 01:33:05 PM CET, Kevin Wolf 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
Re: block/throttle and burst bucket
On Thu 25 Feb 2021 06:34:48 PM CET, Peter Lieven wrote: > I was wondering if there is a way to check from outside (qmp etc.) if > a throttled block device has exceeded the iops_max_length seconds of > time bursting up to iops_max and is now hard limited to the iops limit > that is supplied? > > Would it be also a good idea to exetend the accounting to account for > requests that must have waited before being sent out to the backend > device? No, there's no such interface as far as I'm aware. I think one problem is that throttling is now done using a filter, that can be inserted anywhere in the node graph, and accounting is done at the BlockBackend level. We don't even have a query-block-throttle function. I actually started to write one six years ago but it was never finished. Berto
[PATCH v2] iotests: Drop deprecated 'props' from object-add
Signed-off-by: Alberto Garcia --- v2: Don't use the x-* interface to specify limits [Kevin] tests/qemu-iotests/087 | 8 ++-- tests/qemu-iotests/184 | 18 ++ tests/qemu-iotests/218 | 2 +- tests/qemu-iotests/235 | 2 +- tests/qemu-iotests/245 | 4 ++-- tests/qemu-iotests/258 | 6 +++--- tests/qemu-iotests/258.out | 4 ++-- tests/qemu-iotests/295 | 2 +- tests/qemu-iotests/296 | 2 +- 9 files changed, 19 insertions(+), 29 deletions(-) diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 index edd43f1a28..d8e0e384cd 100755 --- a/tests/qemu-iotests/087 +++ b/tests/qemu-iotests/087 @@ -143,9 +143,7 @@ run_qemu <
Re: [PATCH] iotests: Drop deprecated 'props' from object-add
On Fri 19 Feb 2021 01:21:49 PM CET, Kevin Wolf wrote:
>> log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
>> - props={ 'x-bps-total': size }))
>> + x_bps_total=size))
>
> x-bps-total isn't a stable interface, I'd prefer to use limits.
>
> My patch from November [1] had this:
Do you want me to resend mine, or wait for yours, or what then? :)
Berto
Re: [PATCH] iotests: Drop deprecated 'props' from object-add
On Fri 19 Feb 2021 01:04:00 PM CET, Max Reitz wrote:
> Two Python syntax nit picks below.
>> ret = vm.qmp('object-add', qom_type='throttle-group', id='tg',
>> - props={'x-bps-read': 4096})
>> + x_bps_read = 4096)
>
> To stay consistent, I think there shouldn’t be spaces around '=' here.
Right, I didn't notice that.
>> @@ -103,10 +103,9 @@ def test_concurrent_finish(write_to_stream_node):
>> vm.qmp_log('object-add',
>> qom_type='throttle-group',
>> id='tg',
>> - props={
>> - 'x-iops-write': 1,
>> - 'x-iops-write-max': 1
>> - })
>> + x_iops_write=1,
>> + x_iops_write_max=1
>> + )
>
> This indentation looks weird to me now. Unfortunately, flake8 finds
> this is the only correct indentation, so I have no reason to complain.
>
> Perhaps putting it on the preceding line would be better?
I'm fine either way, I can resend the patch with Kevin's suggestions.
Berto
Re: [RFC PATCH v2 0/4] Allow changing bs->file on reopen
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
[PATCH] iotests: Drop deprecated 'props' from object-add
Signed-off-by: Alberto Garcia --- tests/qemu-iotests/087 | 8 ++-- tests/qemu-iotests/184 | 18 ++ tests/qemu-iotests/218 | 2 +- tests/qemu-iotests/235 | 2 +- tests/qemu-iotests/245 | 4 ++-- tests/qemu-iotests/258 | 7 +++ tests/qemu-iotests/258.out | 4 ++-- tests/qemu-iotests/295 | 2 +- tests/qemu-iotests/296 | 2 +- 9 files changed, 19 insertions(+), 30 deletions(-) diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 index edd43f1a28..d8e0e384cd 100755 --- a/tests/qemu-iotests/087 +++ b/tests/qemu-iotests/087 @@ -143,9 +143,7 @@ run_qemu <
Re: [RFC PATCH v2 0/4] Allow changing bs->file on reopen
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 3/4] block: Support multiple reopening with x-blockdev-reopen
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 1/4] block: Allow changing bs->file on reopen
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 4/4] iotests: Test reopening multiple devices at the same time
Signed-off-by: Alberto Garcia
---
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
[RFC PATCH v2 3/4] block: Support multiple reopening with x-blockdev-reopen
Signed-off-by: Alberto Garcia
---
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();
-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, , _abort);
-visit_complete(v, );
-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_bs; it_bs = bdrv_next()) {
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();
+visitors = g_slist_prepend(visitors, v);
+
+/* Put all options in a QDict and flatten it */
+visit_type_BlockdevOptions(v, NULL, , _abort);
+visit_complete(v, );
+qdict = qobject_to(QDict, obj);
+
+
[RFC PATCH v2 0/4] Allow changing bs->file on reopen
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 2/4] iotests: Update 245 to support replacing files with x-blockdev-reopen
Signed-off-by: Alberto Garcia
---
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
[RFC PATCH v2 1/4] block: Allow changing bs->file on reopen
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
---
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, , 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
Re: [RFC PATCH 0/2] Allow changing bs->file on reopen
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: [PATCH v7 08/14] block/qcow2: qcow2_get_specific_info(): drop error propagation
On Fri 05 Feb 2021 12:52:03 PM CET, Vladimir Sementsov-Ogievskiy wrote: >> However the new code only uses and updates 'info_list' and it does not >> keep the head anywhere, so what the caller gets is a pointer to the >> tail. >> > > No. *info_list is modified only on the first loop iteration. And than > info_list is switched to &(*(info_list))->next, so on second iteration > we will modify @next field of first element, not original *info_list. Right, I see! I find it a bit confusing, 'info_list' is at the same time a return value and a local variable that you use to iterate over the list. Anyway, Reviewed-by: Alberto Garcia Berto
Re: [PATCH v7 08/14] block/qcow2: qcow2_get_specific_info(): drop error propagation
On Tue 02 Feb 2021 01:49:50 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
> -Error **errp)
> +bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
> +Qcow2BitmapInfoList **info_list, Error
> **errp)
> {
> BDRVQcow2State *s = bs->opaque;
> Qcow2BitmapList *bm_list;
> Qcow2Bitmap *bm;
> -Qcow2BitmapInfoList *list = NULL;
> -Qcow2BitmapInfoList **tail =
>
> if (s->nb_bitmaps == 0) {
> -return NULL;
> +*info_list = NULL;
> +return true;
> }
>
> bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> s->bitmap_directory_size, errp);
> -if (bm_list == NULL) {
> -return NULL;
> +if (!bm_list) {
> +return false;
> }
>
> +*info_list = NULL;
> +
> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
> info->granularity = 1U << bm->granularity_bits;
> info->name = g_strdup(bm->name);
> info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS);
> -QAPI_LIST_APPEND(tail, info);
> +QAPI_LIST_APPEND(info_list, info);
> }
>
> bitmap_list_free(bm_list);
>
> -return list;
> +return true;
> }
Maybe I'm reading this wrong but...
In the original code you had the head and tail of the list ('list' and
'tail') then you would append items to the tail and finally return the
head.
However the new code only uses and updates 'info_list' and it does not
keep the head anywhere, so what the caller gets is a pointer to the
tail.
Berto
Re: [PATCH v7 01/14] block: return status from bdrv_append and friends
On Tue 02 Feb 2021 01:49:43 PM CET, Vladimir Sementsov-Ogievskiy wrote: > The recommended use of qemu error api assumes returning status together > with setting errp and avoid void functions with errp parameter. Let's > improve bdrv_append and some friends to reduce error-propagation > overhead in further patches. > > Choose int return status, because bdrv_replace_node_common() has call > to bdrv_check_update_perm(), which reports int status, which seems > correct to propagate. > > Signed-off-by: Vladimir Sementsov-Ogievskiy I had already reviewed this one, hadn't I? Reviewed-by: Alberto Garcia Berto
Re: [PATCH v6 01/14] block: return status from bdrv_append and friends
On Sat 16 Jan 2021 10:51:56 PM CET, Vladimir Sementsov-Ogievskiy wrote: > The recommended use of qemu error api assumes returning status together > with setting errp and avoid void functions with errp parameter. Let's > improve bdrv_append and some friends to reduce error-propagation > overhead in further patches. > > Choose int return status, because bdrv_replace_node_common() has call > to bdrv_check_update_perm(), which reports int status, which seems > correct to propagate. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [RFC PATCH 0/2] Allow changing bs->file on reopen
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 1/2] block: Allow changing bs->file on reopen
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
Signed-off-by: Alberto Garcia
---
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
[RFC PATCH 0/2] Allow changing bs->file on reopen
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
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
---
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, , 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 '
Re: [PATCH v5 02/14] block: use return status of bdrv_append()
On Sat 09 Jan 2021 01:57:59 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Now bdrv_append returns status and we can drop all the local_err things > around it. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH v5 01/14] block: return status from bdrv_append and friends
On Sat 09 Jan 2021 01:57:58 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> -void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> +int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> Error **errp)
The indentation of the second line should be adjusted, shouldn't it?
> {
> +int ret;
> bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
> bdrv_inherits_from_recursive(backing_hd, bs);
>
> if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
> -return;
> +return -EPERM;
> }
>
> if (backing_hd) {
> @@ -2853,15 +2854,24 @@ void bdrv_set_backing_hd(BlockDriverState *bs,
> BlockDriverState *backing_hd,
>
> bs->backing = bdrv_attach_child(bs, backing_hd, "backing", _of_bds,
> bdrv_backing_role(bs), errp);
> +if (!bs->backing) {
> +ret = -EPERM;
> +goto out;
> +}
This is not visible in the patch, but before the bdrv_attach_child()
call there's this:
if (!backing_hd) {
goto out;
}
But in this case 'ret' is still uninitialized.
> out:
> bdrv_refresh_limits(bs, NULL);
> +
> +return ret;
> }
> -static void bdrv_replace_node_common(BlockDriverState *from,
> - BlockDriverState *to,
> - bool auto_skip, Error **errp)
> +static int bdrv_replace_node_common(BlockDriverState *from,
> +BlockDriverState *to,
> +bool auto_skip, Error **errp)
> {
> BdrvChild *c, *next;
> GSList *list = NULL, *p;
> @@ -4562,6 +4572,7 @@ static void bdrv_replace_node_common(BlockDriverState
> *from,
> goto out;
> }
> if (c->frozen) {
> +ret = -EPERM;
> error_setg(errp, "Cannot change '%s' link to '%s'",
> c->name, from->node_name);
> goto out;
Same here, you set 'ret' in the second 'goto out' but not in the first.
Berto
[PATCH v2] iotests: Add test for the regression fixed in c8bf9a9169
Signed-off-by: Alberto Garcia
Suggested-by: Maxim Levitsky
Reviewed-by: Maxim Levitsky
---
v2: Rebase on top of the latest master
tests/qemu-iotests/313 | 103 +
tests/qemu-iotests/313.out | 29 +++
tests/qemu-iotests/group | 1 +
3 files changed, 133 insertions(+)
create mode 100755 tests/qemu-iotests/313
create mode 100644 tests/qemu-iotests/313.out
diff --git a/tests/qemu-iotests/313 b/tests/qemu-iotests/313
new file mode 100755
index 00..0a5202ad49
--- /dev/null
+++ b/tests/qemu-iotests/313
@@ -0,0 +1,103 @@
+#!/usr/bin/env bash
+#
+# Test for the regression fixed in commit c8bf9a9169
+#
+# Copyright (C) 2020 Igalia, S.L.
+# Author: Alberto Garcia
+# Based on a test case by Maxim Levitsky
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=be...@igalia.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1# failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+_unsupported_imgopts cluster_size refcount_bits extended_l2 compat=0.10
data_file
+
+# The cluster size must be at least the granularity of the mirror job (4KB)
+# Note that larger cluster sizes will produce very large images (several GBs)
+cluster_size=4096
+refcount_bits=64 # Make it equal to the L2 entry size for convenience
+options="cluster_size=${cluster_size},refcount_bits=${refcount_bits}"
+
+# Number of refcount entries per refcount blocks
+ref_entries=$(( ${cluster_size} * 8 / ${refcount_bits} ))
+
+# Number of data clusters needed to fill a refcount block
+# Equals ${ref_entries} minus two (one L2 table and one refcount block)
+data_clusters_per_refblock=$(( ${ref_entries} - 2 ))
+
+# Number of entries in the refcount cache
+ref_blocks=4
+
+# Write enough data clusters to fill the refcount cache and allocate
+# one more refcount block.
+# Subtract 3 clusters from the total: qcow2 header, refcount table, L1 table
+total_data_clusters=$(( ${data_clusters_per_refblock} * ${ref_blocks} + 1 - 3
))
+
+# Total size to write in bytes
+total_size=$(( ${total_data_clusters} * ${cluster_size} ))
+
+echo
+echo '### Create the image'
+echo
+TEST_IMG_FILE=$TEST_IMG.base _make_test_img -o $options $total_size |
_filter_img_create_size
+
+echo
+echo '### Write data to allocate more refcount blocks than the cache can hold'
+echo
+$QEMU_IO -c "write -P 1 0 $total_size" $TEST_IMG.base | _filter_qemu_io
+
+echo
+echo '### Create an overlay'
+echo
+_make_test_img -F $IMGFMT -b $TEST_IMG.base -o $options |
_filter_img_create_size
+
+echo
+echo '### Fill the overlay with zeroes'
+echo
+$QEMU_IO -c "write -z 0 $total_size" $TEST_IMG | _filter_qemu_io
+
+echo
+echo '### Commit changes to the base image'
+echo
+$QEMU_IMG commit $TEST_IMG
+
+echo
+echo '### Check the base image'
+echo
+$QEMU_IMG check $TEST_IMG.base
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/313.out b/tests/qemu-iotests/313.out
new file mode 100644
index 00..adb9f7bd95
--- /dev/null
+++ b/tests/qemu-iotests/313.out
@@ -0,0 +1,29 @@
+QA output created by 313
+
+### Create the image
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=SIZE
+
+### Write data to allocate more refcount blocks than the cache can hold
+
+wrote 8347648/8347648 bytes at offset 0
+7.961 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+### Create an overlay
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+
+### Fill the overlay with zeroes
+
+wrote 8347648/8347648 bytes at offset 0
+7.961 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+### Commit changes to the base image
+
+Image committed.
+
+### Check the base image
+
+No errors were found on the image.
+Image end offset: 8396800
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index bc5bc324fe..26c9c67c45 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -319,3 +319,4 @@
308 rw
309 rw auto quick
312 rw quick
+313 rw auto quick
--
2.20.1
Re: [PATCH v5 14/14] block/qcow2: refactor qcow2_update_options_prepare error paths
On Sat 09 Jan 2021 01:58:11 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Keep setting ret close to setting errp and don't merge different error
> paths into one. This way it's more obvious that we don't return
> error without setting errp.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
I get the idea, but I feel the code is getting innecessarily verbose.
One alternative would be to get rid of all -EINVAL inside the switch
block, take advantage of the existing local_err and follow the block
with:
if (local_err) {
error_propagate(errp, local_err);
ret = -EINVAL;
goto fail;
}
But otherwise your solution is correct, so you can keep it if you
prefer:
Reviewed-by: Alberto Garcia
Berto
Re: [PATCH v2 14/36] block: inline bdrv_child_*() permission functions calls
On Fri 27 Nov 2020 03:45:00 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Each of them has only one caller. Open-coding simplifies further > pemission-update system changes. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 09/36] block: return value from bdrv_replace_node()
On Fri 27 Nov 2020 03:44:55 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Functions with errp argument are not recommened to be void-functions. > Improve bdrv_replace_node(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 08/36] block: make bdrv_reopen_{prepare, commit, abort} private
On Fri 27 Nov 2020 03:44:54 PM CET, Vladimir Sementsov-Ogievskiy via wrote: > These functions are called only from bdrv_reopen_multiple() in block.c. > No reason to publish them. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 04/36] block: bdrv_append(): return status
On Fri 27 Nov 2020 03:44:50 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Return int status to avoid extra error propagation schemes. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH v4 4/4] block: qcow2: remove the created file on initialization error
On Wed 09 Dec 2020 05:44:41 PM CET, Maxim Levitsky wrote:
> @@ -3847,12 +3847,13 @@ static int coroutine_fn
> qcow2_co_create_opts(BlockDriver *drv,
>
> /* Create the qcow2 image (format layer) */
> ret = qcow2_co_create(create_options, errp);
> +
> +finish:
> if (ret < 0) {
> -goto finish;
> +bdrv_co_delete_file_noerr(bs);
> +bdrv_co_delete_file_noerr(data_bs);
> }
>
> -ret = 0;
Many/most functions in qcow2.c force ret to be 0 on success, we could
also keep that here (although in practice I don't think that ret can be
greater than 0 in this case, or that the caller would care).
Either way,
Reviewed-by: Alberto Garcia
Berto
Re: [PATCH v4 2/4] block: add bdrv_co_delete_file_noerr
On Wed 09 Dec 2020 05:44:39 PM CET, Maxim Levitsky wrote:
> +void coroutine_fn bdrv_co_delete_file_noerr(BlockDriverState *bs)
> +{
> +Error *local_err = NULL;
> +
> +if (!bs) {
> +return;
> +}
> +
> +int ret = bdrv_co_delete_file(bs, _err);
^^^
According to the QEMU coding style we should not have declarations in
the middle of a block.
The patch looks otherwise fine.
Reviewed-by: Alberto Garcia
Berto
Re: [PATCH v4 3/4] crypto: luks: use bdrv_co_delete_file_noerr
On Wed 09 Dec 2020 05:44:40 PM CET, Maxim Levitsky wrote: > This refactoring is now possible thanks to this function. > > Signed-off-by: Maxim Levitsky Reviewed-by: Alberto Garcia Berto
Re: [PATCH v3 2/2] block: qcow2: remove the created file on initialization error
On Tue 08 Dec 2020 03:21:59 PM CET, Maxim Levitsky wrote:
> If the qcow initialization fails, we should remove the file if it was
> already created, to avoid leaving stale files around.
>
> We already do this for luks raw images.
>
> Signed-off-by: Maxim Levitsky
Reviewed-by: Alberto Garcia
> ret = qcow2_co_create(create_options, errp);
> if (ret < 0) {
> +
> +Error *local_delete_err = NULL;
Why that empty line though?
Berto
Re: [PATCH v3 1/2] crypto: luks: Fix tiny memory leak
On Tue 08 Dec 2020 03:21:58 PM CET, Maxim Levitsky wrote: > When the underlying block device doesn't support the > bdrv_co_delete_file interface, an 'Error' object was leaked. > > Signed-off-by: Maxim Levitsky Reviewed-by: Alberto Garcia Berto
Re: [PATCH 3/4] block/io: bdrv_check_byte_request(): drop bdrv_is_inserted()
On Thu 03 Dec 2020 11:27:12 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Move bdrv_is_inserted() calls into callers. > > We are going to make bdrv_check_byte_request() a clean thing. > bdrv_is_inserted() is not about checking the request, it's about > checking the bs. So, it should be separate. > > With this patch we probably change error path for some failure > scenarios. But depending on the fact that querying too big request on > empty cdrom (or corrupted qcow2 node with no drv) will result in EIO > and not ENOMEDIUM would be very strange. More over, we are going to > move to 64bit requests, so larger requests will be allowed anyway. > > More over, keeping in mind that cdrom is the only driver that has > .bdrv_is_inserted() handler it's strange that we should care so much > about it in generic block layer, intuitively we should just do read and > write, and cdrom driver should return correct errors if it is not > inserted. But it's a work for another series. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH 2/4] block/io: bdrv_refresh_limits(): use ERRP_GUARD
On Thu 03 Dec 2020 11:27:11 PM CET, Vladimir Sementsov-Ogievskiy wrote: > This simplifies following commit. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: Plans to bring QMP 'x-blockdev-reopen' out of experimental?
On Wed 02 Dec 2020 06:51:21 PM CET, Kevin Wolf wrote: >> I had tried this already and it does work when inserting the filter (we >> know that 'hd0-file' is about to be detached from the parent so we can >> put it in the list) but I don't think it's so easy if we want to remove >> the filter, i.e. >> >>hd0 -> throttle -> hd0-file ==> hd0 -> hd0-file >> >> In this case we get a similar error, we want to make hd0-file a child of >> hd0 but it is being used by the throttle filter. >> >> Telling bdrv_check_update_perm() to ignore hd0's current child >> (throttle) won't solve the problem. > > Isn't this the very same case as removing e.g. a mirror filter from the > chain? I'm sure we have already solved this somewhere. > > Hm, no, it might actually be different in that the throttle node > survives this, so we do have to check that the resulting graph is > valid. Do we need a combined operation to remove the throttle node > from the graph and immediately delete it? What kind of API are you thinking about? One basic problem with inserting filter nodes (as opposed to replacing a node with a different one) is that we have a protocol BDS used twice at the same time, e.g. hd0 -> hd0-file throttle -> hd0-file And then we would reopen hd0 and do the change, but ideally one would prefer to avoid having hd0-file twice. There's also the x-blockdev-change function (currently for Quorum only), I wonder if it could be a better fit for this use case. Berto
Re: Plans to bring QMP 'x-blockdev-reopen' out of experimental?
On Wed 02 Dec 2020 05:28:08 PM CET, Kevin Wolf wrote:
>> So x-blockdev-reopen sees that we want to replace the current
>> bs->file ("hd0-file") with a new one ("throttle0"). The problem here
>> is that throttle0 has hd0-file as its child, so when we check the
>> permissions on throttle0 (and its children) we get that hd0-file
>> refuses because it's already being used (although in in the process
>> of being replaced) by hd0:
>>
>> "Conflicts with use by hd0 as 'file', which does not allow 'write, resize'
>> on hd0-file"
>>
> This kind of situation isn't new, I believe some of the existing graph
> changes (iirc in the context of block jobs) can cause the same problem.
>
> This is essentially why some functions in the permission system take a
> GSList *ignore_children. So I believe the right thing to do here is
> telling the permission system that it needs to check the situation
> without the BdrvChild that links hd0 with hd0-file.
I had tried this already and it does work when inserting the filter (we
know that 'hd0-file' is about to be detached from the parent so we can
put it in the list) but I don't think it's so easy if we want to remove
the filter, i.e.
hd0 -> throttle -> hd0-file ==> hd0 -> hd0-file
In this case we get a similar error, we want to make hd0-file a child of
hd0 but it is being used by the throttle filter.
Telling bdrv_check_update_perm() to ignore hd0's current child
(throttle) won't solve the problem.
> I don't know the exact stack trace of your failure, so maybe this
> parameter isn't available yet in the place where you need it, but in
> the core functions it exists.
This is in bdrv_reopen_multiple(), in the same place where we are
currently checking the permissions of the new backing file.
Berto
Re: Plans to bring QMP 'x-blockdev-reopen' out of experimental?
On Tue 20 Oct 2020 10:23:33 AM CEST, Kevin Wolf wrote:
>> I forgot to add, we still don't support changing bs->file with this
>> command, so I guess that would be one blocker?
>>
>> There's no other way of inserting filter nodes, or is there?
>
> Not that I'm aware of.
>
> So yes, changing bs->file is the one thing I had in mind for
> implementing before we mark it stable.
>
> I'm not entirely sure if we should make some restrictions or allow
> arbitrary changes. If it's only about filters, we could check that the
> node returned by bdrv_skip_filters() stays the same. This would be
> almost certainly safe (if the chain is not frozen, of course).
>
> If people want to dynamically insert non-filters (maybe quorum?), it
> might be more restrictive than necessary, though.
>
> Other cases like inserting a qcow2 file in the chain where the old
> child becomes the backing file of the newly inserted node should in
> theory already be covered by blockdev-snapshot.
Hi,
I have been working a bit on this and I have doubts about the
following situation: let's say we have a normal qcow2 image with two
BDS for format (node-name "hd0") and protocol ("hd0-file"):
hd0 -> hd0-file
{ "execute": "blockdev-add", "arguments":
{'driver': 'file', 'node-name': 'hd0-file', 'filename': 'hd0.qcow2 }}
{ "execute": "blockdev-add", "arguments":
{'driver': 'qcow2', 'node-name': 'hd0', 'file': 'hd0-file'}}
Now we want to use x-blockdev-reopen to insert a throttle filter
between them, so the result would be:
hd0 -> throttle -> hd0-file
First we add the filter:
{ "execute": "object-add", "arguments":
{ 'qom-type': 'throttle-group', 'id': 'group0',
'props': { 'limits': { 'iops-total': 1000 } } } }
{ "execute": "blockdev-add", "arguments":
{ 'driver': 'throttle', 'node-name': 'throttle0',
'throttle-group': 'group0', 'file': "hd0-file" } }
And then we insert it:
{ "execute": "x-blockdev-reopen", "arguments":
{'driver': 'qcow2', 'node-name': 'hd0', 'file': 'throttle0'}}
So x-blockdev-reopen sees that we want to replace the current bs->file
("hd0-file") with a new one ("throttle0"). The problem here is that
throttle0 has hd0-file as its child, so when we check the permissions on
throttle0 (and its children) we get that hd0-file refuses because it's
already being used (although in in the process of being replaced) by
hd0:
"Conflicts with use by hd0 as 'file', which does not allow 'write, resize' on
hd0-file"
And we would get a similar problem with the reverse operation (removing
the filter).
Berto
Re: [PATCH] qemu-nbd: Fix a memleak in qemu_nbd_client_list()
On Mon 30 Nov 2020 01:36:51 PM CET, Alex Chen wrote: > When the qio_channel_socket_connect_sync() fails > we should goto 'out' label to free the 'sioc' instead of return. > > Reported-by: Euler Robot > Signed-off-by: Alex Chen Reviewed-by: Alberto Garcia Berto
[PATCH] iotests: Add test for the regression fixed in c8bf9a9169
Signed-off-by: Alberto Garcia
Suggested-by: Maxim Levitsky
---
tests/qemu-iotests/313 | 103 +
tests/qemu-iotests/313.out | 29 +++
tests/qemu-iotests/group | 1 +
3 files changed, 133 insertions(+)
create mode 100755 tests/qemu-iotests/313
create mode 100644 tests/qemu-iotests/313.out
diff --git a/tests/qemu-iotests/313 b/tests/qemu-iotests/313
new file mode 100755
index 00..0a5202ad49
--- /dev/null
+++ b/tests/qemu-iotests/313
@@ -0,0 +1,103 @@
+#!/usr/bin/env bash
+#
+# Test for the regression fixed in commit c8bf9a9169
+#
+# Copyright (C) 2020 Igalia, S.L.
+# Author: Alberto Garcia
+# Based on a test case by Maxim Levitsky
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=be...@igalia.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1# failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+_unsupported_imgopts cluster_size refcount_bits extended_l2 compat=0.10
data_file
+
+# The cluster size must be at least the granularity of the mirror job (4KB)
+# Note that larger cluster sizes will produce very large images (several GBs)
+cluster_size=4096
+refcount_bits=64 # Make it equal to the L2 entry size for convenience
+options="cluster_size=${cluster_size},refcount_bits=${refcount_bits}"
+
+# Number of refcount entries per refcount blocks
+ref_entries=$(( ${cluster_size} * 8 / ${refcount_bits} ))
+
+# Number of data clusters needed to fill a refcount block
+# Equals ${ref_entries} minus two (one L2 table and one refcount block)
+data_clusters_per_refblock=$(( ${ref_entries} - 2 ))
+
+# Number of entries in the refcount cache
+ref_blocks=4
+
+# Write enough data clusters to fill the refcount cache and allocate
+# one more refcount block.
+# Subtract 3 clusters from the total: qcow2 header, refcount table, L1 table
+total_data_clusters=$(( ${data_clusters_per_refblock} * ${ref_blocks} + 1 - 3
))
+
+# Total size to write in bytes
+total_size=$(( ${total_data_clusters} * ${cluster_size} ))
+
+echo
+echo '### Create the image'
+echo
+TEST_IMG_FILE=$TEST_IMG.base _make_test_img -o $options $total_size |
_filter_img_create_size
+
+echo
+echo '### Write data to allocate more refcount blocks than the cache can hold'
+echo
+$QEMU_IO -c "write -P 1 0 $total_size" $TEST_IMG.base | _filter_qemu_io
+
+echo
+echo '### Create an overlay'
+echo
+_make_test_img -F $IMGFMT -b $TEST_IMG.base -o $options |
_filter_img_create_size
+
+echo
+echo '### Fill the overlay with zeroes'
+echo
+$QEMU_IO -c "write -z 0 $total_size" $TEST_IMG | _filter_qemu_io
+
+echo
+echo '### Commit changes to the base image'
+echo
+$QEMU_IMG commit $TEST_IMG
+
+echo
+echo '### Check the base image'
+echo
+$QEMU_IMG check $TEST_IMG.base
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/313.out b/tests/qemu-iotests/313.out
new file mode 100644
index 00..adb9f7bd95
--- /dev/null
+++ b/tests/qemu-iotests/313.out
@@ -0,0 +1,29 @@
+QA output created by 313
+
+### Create the image
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=SIZE
+
+### Write data to allocate more refcount blocks than the cache can hold
+
+wrote 8347648/8347648 bytes at offset 0
+7.961 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+### Create an overlay
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+
+### Fill the overlay with zeroes
+
+wrote 8347648/8347648 bytes at offset 0
+7.961 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+### Commit changes to the base image
+
+Image committed.
+
+### Check the base image
+
+No errors were found on the image.
+Image end offset: 8396800
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 2960dff728..df339f1720 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -316,3 +316,4 @@
305 rw quick
307 rw quick export
309 rw auto quick
+313 rw auto quick
--
2.20.1
