| Message ID | 145882bca942bb629bce2b1f5546fe0946ccdfcd.1612809837.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Allow changing bs->file on reopen | expand |
08.02.2021 21:44, Alberto Garcia wrote: > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > qapi/block-core.json | 2 +- > include/block/block.h | 1 + > block.c | 16 +++++-- > blockdev.c | 85 +++++++++++++++++++++----------------- > tests/qemu-iotests/155 | 9 ++-- > tests/qemu-iotests/165 | 4 +- > tests/qemu-iotests/245 | 27 +++++++----- > tests/qemu-iotests/248 | 2 +- > tests/qemu-iotests/248.out | 2 +- > tests/qemu-iotests/298 | 4 +- > 10 files changed, 89 insertions(+), 63 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index c0e7c23331..b9fcf20a81 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -4177,7 +4177,7 @@ > # Since: 4.0 > ## > { 'command': 'x-blockdev-reopen', > - 'data': 'BlockdevOptions', 'boxed': true } > + 'data': { 'options': ['BlockdevOptions'] } } Do we also want to drop x- prefix? > > ## > # @blockdev-del: > diff --git a/include/block/block.h b/include/block/block.h > index 6dd687a69e..fe4a220da9 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -372,6 +372,7 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name, > BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, > BlockDriverState *bs, QDict *options, > bool keep_old_opts); > +void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue); > int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp); > int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only, > Error **errp); > diff --git a/block.c b/block.c > index 19b62da4af..b4fef2308f 100644 > --- a/block.c > +++ b/block.c > @@ -3933,6 +3933,17 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, > NULL, 0, keep_old_opts); > } > > +void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue) > +{ > + if (bs_queue) { > + BlockReopenQueueEntry *bs_entry, *next; > + QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) { > + g_free(bs_entry); > + } > + g_free(bs_queue); > + } > +} > + > /* > * Reopen multiple BlockDriverStates atomically & transactionally. > * > @@ -4024,10 +4035,7 @@ abort: > } > > cleanup: > - QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) { > - g_free(bs_entry); > - } > - g_free(bs_queue); > + bdrv_reopen_queue_free(bs_queue); this may be a separate patch > > return ret; > } > diff --git a/blockdev.c b/blockdev.c > index 098a05709d..6b688c0f73 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3528,38 +3528,16 @@ fail: > visit_free(v); > } > > -void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp) > +void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp) > { > - BlockDriverState *bs; > - QObject *obj; > - Visitor *v = qobject_output_visitor_new(&obj); > - BlockReopenQueue *queue; > - QDict *qdict; > - > - /* Check for the selected node name */ > - if (!options->has_node_name) { > - error_setg(errp, "Node name not specified"); > - goto fail; > - } > - > - bs = bdrv_find_node(options->node_name); > - if (!bs) { > - error_setg(errp, "Cannot find node named '%s'", options->node_name); > - goto fail; > - } > - > - /* Put all options in a QDict and flatten it */ > - visit_type_BlockdevOptions(v, NULL, &options, &error_abort); > - visit_complete(v, &obj); > - qdict = qobject_to(QDict, obj); > - > - qdict_flatten(qdict); > - > - /* Perform the reopen operation */ > + BlockReopenQueue *queue = NULL; > + GSList *aio_ctxs = NULL; > + GSList *visitors = NULL; > + GSList *drained = NULL; > BdrvNextIterator it; > - GSList *aio_ctxs = NULL, *ctx; > BlockDriverState *it_bs; > > + /* Acquire all AIO contexts */ > for (it_bs = bdrv_first(&it); it_bs; it_bs = bdrv_next(&it)) { > AioContext *aio_context = bdrv_get_aio_context(it_bs); > > @@ -3569,19 +3547,50 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp) > } > } > > - bdrv_subtree_drained_begin(bs); > - queue = bdrv_reopen_queue(NULL, bs, qdict, false); > + /* Add each one of the BDS that we want to reopen to the queue */ > + for (; reopen_list != NULL; reopen_list = reopen_list->next) { > + BlockdevOptions *options = reopen_list->value; > + QDict *qdict; > + Visitor *v; > + BlockDriverState *bs; > + QObject *obj; > + > + /* Check for the selected node name */ > + if (!options->has_node_name) { > + error_setg(errp, "Node name not specified"); > + goto fail; > + } > + > + bs = bdrv_find_node(options->node_name); > + if (!bs) { > + error_setg(errp, "Cannot find node named '%s'", options->node_name); > + goto fail; > + } > + > + v = qobject_output_visitor_new(&obj); > + visitors = g_slist_prepend(visitors, v); > + > + /* Put all options in a QDict and flatten it */ > + visit_type_BlockdevOptions(v, NULL, &options, &error_abort); > + visit_complete(v, &obj); > + qdict = qobject_to(QDict, obj); > + > + qdict_flatten(qdict); > + > + bdrv_subtree_drained_begin(bs); > + queue = bdrv_reopen_queue(queue, bs, qdict, false); > + drained = g_slist_prepend(drained, bs); > + } > + > + /* Perform the reopen operation */ > bdrv_reopen_multiple(queue, errp); > - bdrv_subtree_drained_end(bs); > - > - for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) { > - AioContext *aio_context = ctx->data; > - aio_context_release(aio_context); > - } > - g_slist_free(aio_ctxs); > + queue = NULL; > > fail: > - visit_free(v); > + bdrv_reopen_queue_free(queue); > + g_slist_free_full(drained, (GDestroyNotify) bdrv_subtree_drained_end); > + g_slist_free_full(aio_ctxs, (GDestroyNotify) aio_context_release); > + g_slist_free_full(visitors, (GDestroyNotify) visit_free); Probably you can use g_autoslist() for defining these lists to get automatic cleanup. > } > > void qmp_blockdev_del(const char *node_name, Error **errp) > diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155 > index 988f986144..5271f9541f 100755 > --- a/tests/qemu-iotests/155 > +++ b/tests/qemu-iotests/155 > @@ -260,9 +260,12 @@ class TestBlockdevMirrorReopen(MirrorBaseClass): > result = self.vm.qmp('blockdev-add', node_name="backing", > driver="null-co") > self.assert_qmp(result, 'return', {}) > - result = self.vm.qmp('x-blockdev-reopen', node_name="target", > - driver=iotests.imgfmt, file="target-file", > - backing="backing") > + result = self.vm.qmp('x-blockdev-reopen', options = [{ > + 'node-name': "target", > + 'driver': iotests.imgfmt, > + 'file': "target-file", > + 'backing': "backing" > + }]) > self.assert_qmp(result, 'return', {}) > > class TestBlockdevMirrorReopenIothread(TestBlockdevMirrorReopen): > diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165 > index fb56a769b4..32db5086e1 100755 > --- a/tests/qemu-iotests/165 > +++ b/tests/qemu-iotests/165 > @@ -136,7 +136,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase): > assert sha256_1 == self.getSha256() > > # Reopen to RW > - result = self.vm.qmp('x-blockdev-reopen', **{ > + result = self.vm.qmp('x-blockdev-reopen', options = [{ > 'node-name': 'node0', > 'driver': iotests.imgfmt, > 'file': { > @@ -144,7 +144,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase): > 'filename': disk > }, > 'read-only': False > - }) > + }]) > self.assert_qmp(result, 'return', {}) > > # Check that bitmap is reopened to RW and we can write to it. > diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 > index bad6911f0c..850c9f070b 100755 > --- a/tests/qemu-iotests/245 > +++ b/tests/qemu-iotests/245 > @@ -84,8 +84,18 @@ class TestBlockdevReopen(iotests.QMPTestCase): > "Expected output of %d qemu-io commands, found %d" % > (found, self.total_io_cmds)) > > - # Run x-blockdev-reopen with 'opts' but applying 'newopts' > - # on top of it. The original 'opts' dict is unmodified > + # Run x-blockdev-reopen on a list of block devices > + def reopenMultiple(self, opts, errmsg = None): > + result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options = opts) > + if errmsg: > + self.assert_qmp(result, 'error/class', 'GenericError') > + self.assert_qmp(result, 'error/desc', errmsg) > + else: > + self.assert_qmp(result, 'return', {}) > + > + # Run x-blockdev-reopen on a single block device (specified by > + # 'opts') but applying 'newopts' on top of it. The original 'opts' > + # dict is unmodified > def reopen(self, opts, newopts = {}, errmsg = None): > opts = copy.deepcopy(opts) > > @@ -100,12 +110,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): > subdict = opts[prefix] > subdict[key] = value > > - result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, **opts) > - if errmsg: > - self.assert_qmp(result, 'error/class', 'GenericError') > - self.assert_qmp(result, 'error/desc', errmsg) > - else: > - self.assert_qmp(result, 'return', {}) > + self.reopenMultiple([ opts ], errmsg) > > > # Run query-named-block-nodes and return the specified entry > @@ -141,10 +146,10 @@ class TestBlockdevReopen(iotests.QMPTestCase): > # We cannot change any of these > self.reopen(opts, {'node-name': 'not-found'}, "Cannot find node named 'not-found'") > self.reopen(opts, {'node-name': ''}, "Cannot find node named ''") > - self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'node-name', expected: string") > + self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'options[0].node-name', expected: string") > self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'") > self.reopen(opts, {'driver': ''}, "Invalid parameter ''") > - self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string") > + self.reopen(opts, {'driver': None}, "Invalid parameter type for 'options[0].driver', expected: string") > self.reopen(opts, {'file': 'not-found'}, "Cannot find device= nor node_name=not-found") > self.reopen(opts, {'file': ''}, "Cannot find device= nor node_name=") > self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef") > @@ -153,7 +158,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): > self.reopen(opts, {'file.filename': hd_path[1]}, "Cannot change the option 'filename'") > self.reopen(opts, {'file.aio': 'native'}, "Cannot change the option 'aio'") > self.reopen(opts, {'file.locking': 'off'}, "Cannot change the option 'locking'") > - self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'file.filename', expected: string") > + self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'options[0].file.filename', expected: string") > > # node-name is optional in BlockdevOptions, but x-blockdev-reopen needs it > del opts['node-name'] > diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248 > index 18ba03467e..2b43853183 100755 > --- a/tests/qemu-iotests/248 > +++ b/tests/qemu-iotests/248 > @@ -62,7 +62,7 @@ vm.get_qmp_events() > > del blockdev_opts['file']['size'] > vm.qmp_log('x-blockdev-reopen', filters=[filter_qmp_testfiles], > - **blockdev_opts) > + options = [ blockdev_opts ]) > > vm.qmp_log('block-job-resume', device='drive0') > vm.event_wait('JOB_STATUS_CHANGE', timeout=1.0, > diff --git a/tests/qemu-iotests/248.out b/tests/qemu-iotests/248.out > index 369b25bf26..893f625347 100644 > --- a/tests/qemu-iotests/248.out > +++ b/tests/qemu-iotests/248.out > @@ -2,7 +2,7 @@ > {"return": {}} > {"execute": "blockdev-mirror", "arguments": {"device": "drive0", "on-target-error": "enospc", "sync": "full", "target": "target"}} > {"return": {}} > -{"execute": "x-blockdev-reopen", "arguments": {"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}} > +{"execute": "x-blockdev-reopen", "arguments": {"options": [{"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}]}} > {"return": {}} > {"execute": "block-job-resume", "arguments": {"device": "drive0"}} > {"return": {}} > diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298 > index d535946b5f..4efdb35b91 100644 > --- a/tests/qemu-iotests/298 > +++ b/tests/qemu-iotests/298 > @@ -98,7 +98,7 @@ class TestPreallocateFilter(TestPreallocateBase): > self.check_big() > > def test_reopen_opts(self): > - result = self.vm.qmp('x-blockdev-reopen', **{ > + result = self.vm.qmp('x-blockdev-reopen', options = [{ > 'node-name': 'disk', > 'driver': iotests.imgfmt, > 'file': { > @@ -112,7 +112,7 @@ class TestPreallocateFilter(TestPreallocateBase): > 'filename': disk > } > } > - }) > + }]) > self.assert_qmp(result, 'return', {}) > > self.vm.hmp_qemu_io('drive0', 'write 0 1M') >
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
Am 09.02.2021 um 09:03 hat Vladimir Sementsov-Ogievskiy geschrieben: > 08.02.2021 21:44, Alberto Garcia wrote: > > Signed-off-by: Alberto Garcia <berto@igalia.com> > > --- > > qapi/block-core.json | 2 +- > > include/block/block.h | 1 + > > block.c | 16 +++++-- > > blockdev.c | 85 +++++++++++++++++++++----------------- > > tests/qemu-iotests/155 | 9 ++-- > > tests/qemu-iotests/165 | 4 +- > > tests/qemu-iotests/245 | 27 +++++++----- > > tests/qemu-iotests/248 | 2 +- > > tests/qemu-iotests/248.out | 2 +- > > tests/qemu-iotests/298 | 4 +- > > 10 files changed, 89 insertions(+), 63 deletions(-) > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > index c0e7c23331..b9fcf20a81 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -4177,7 +4177,7 @@ > > # Since: 4.0 > > ## > > { 'command': 'x-blockdev-reopen', > > - 'data': 'BlockdevOptions', 'boxed': true } > > + 'data': { 'options': ['BlockdevOptions'] } } > > Do we also want to drop x- prefix? libvirt really wants to have a stable blockdev-reopen interface in 6.0 because enabling the incremental backup code depends on this (they just toggle the readonly flag if I understand correctly, so most of the work we're currently doing isn't even relevant at this moment for libvirt). Given that the soft freeze is coming closer (March 16), I wonder if we should just make this API change and declare the interface stable. We can then make Vladimir's fixes and the file reopening on top of it - if it's in time for 6.0, that would be good, but if not we could move it to 6.1 without impacting libvirt. I think we're reasonable confident that the QAPI interfaces are right, even if maybe not that all aspects of the implementation are right yet. What do you think? Kevin
24.02.2021 15:33, Kevin Wolf wrote: > Am 09.02.2021 um 09:03 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 08.02.2021 21:44, Alberto Garcia wrote: >>> Signed-off-by: Alberto Garcia <berto@igalia.com> >>> --- >>> qapi/block-core.json | 2 +- >>> include/block/block.h | 1 + >>> block.c | 16 +++++-- >>> blockdev.c | 85 +++++++++++++++++++++----------------- >>> tests/qemu-iotests/155 | 9 ++-- >>> tests/qemu-iotests/165 | 4 +- >>> tests/qemu-iotests/245 | 27 +++++++----- >>> tests/qemu-iotests/248 | 2 +- >>> tests/qemu-iotests/248.out | 2 +- >>> tests/qemu-iotests/298 | 4 +- >>> 10 files changed, 89 insertions(+), 63 deletions(-) >>> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index c0e7c23331..b9fcf20a81 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -4177,7 +4177,7 @@ >>> # Since: 4.0 >>> ## >>> { 'command': 'x-blockdev-reopen', >>> - 'data': 'BlockdevOptions', 'boxed': true } >>> + 'data': { 'options': ['BlockdevOptions'] } } >> >> Do we also want to drop x- prefix? > > libvirt really wants to have a stable blockdev-reopen interface in 6.0 > because enabling the incremental backup code depends on this (they just > toggle the readonly flag if I understand correctly, so most of the work > we're currently doing isn't even relevant at this moment for libvirt). Do you know what is the case exactly? If they do it to remove dirty bitmap from backing image after snapshot operation, probably we'd better improve block-dirty-bitmap-remove command to be able to reopen r-o image? (I just recently faced such a task) > > Given that the soft freeze is coming closer (March 16), I wonder if we > should just make this API change and declare the interface stable. We > can then make Vladimir's fixes and the file reopening on top of it - if > it's in time for 6.0, that would be good, but if not we could move it to > 6.1 without impacting libvirt. > > I think we're reasonable confident that the QAPI interfaces are right, > even if maybe not that all aspects of the implementation are right yet. > > What do you think? > I think it's OK.. We have it since 4.0. What will we win keeping -x for years? Even latest change from updating one device to several could be easily done with help of 'alternate' if the command was already stable.
On Wed 24 Feb 2021 01:33:05 PM CET, Kevin Wolf <kwolf@redhat.com> wrote: >> > { 'command': 'x-blockdev-reopen', >> > - 'data': 'BlockdevOptions', 'boxed': true } >> > + 'data': { 'options': ['BlockdevOptions'] } } >> >> Do we also want to drop x- prefix? > > libvirt really wants to have a stable blockdev-reopen interface in 6.0 > because enabling the incremental backup code depends on this (they > just toggle the readonly flag if I understand correctly, so most of > the work we're currently doing isn't even relevant at this moment for > libvirt). > > Given that the soft freeze is coming closer (March 16), I wonder if we > should just make this API change and declare the interface stable. We > can then make Vladimir's fixes and the file reopening on top of it - > if it's in time for 6.0, that would be good, but if not we could move > it to 6.1 without impacting libvirt. I expect to publish the new version of my patches next week, although they still apply on top of Vladimir's code, which is not rebased. We can of course simply update the API and implement the functionality later, but apart from dropping the prefix we would also be changing the parameters so qmp_x_blockdev_reopen() would also need to be modified. Berto
Am 25.02.2021 um 18:02 hat Vladimir Sementsov-Ogievskiy geschrieben: > 24.02.2021 15:33, Kevin Wolf wrote: > > Am 09.02.2021 um 09:03 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > 08.02.2021 21:44, Alberto Garcia wrote: > > > > Signed-off-by: Alberto Garcia <berto@igalia.com> > > > > --- > > > > qapi/block-core.json | 2 +- > > > > include/block/block.h | 1 + > > > > block.c | 16 +++++-- > > > > blockdev.c | 85 +++++++++++++++++++++----------------- > > > > tests/qemu-iotests/155 | 9 ++-- > > > > tests/qemu-iotests/165 | 4 +- > > > > tests/qemu-iotests/245 | 27 +++++++----- > > > > tests/qemu-iotests/248 | 2 +- > > > > tests/qemu-iotests/248.out | 2 +- > > > > tests/qemu-iotests/298 | 4 +- > > > > 10 files changed, 89 insertions(+), 63 deletions(-) > > > > > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > > > index c0e7c23331..b9fcf20a81 100644 > > > > --- a/qapi/block-core.json > > > > +++ b/qapi/block-core.json > > > > @@ -4177,7 +4177,7 @@ > > > > # Since: 4.0 > > > > ## > > > > { 'command': 'x-blockdev-reopen', > > > > - 'data': 'BlockdevOptions', 'boxed': true } > > > > + 'data': { 'options': ['BlockdevOptions'] } } > > > > > > Do we also want to drop x- prefix? > > > > libvirt really wants to have a stable blockdev-reopen interface in 6.0 > > because enabling the incremental backup code depends on this (they just > > toggle the readonly flag if I understand correctly, so most of the work > > we're currently doing isn't even relevant at this moment for libvirt). > > Do you know what is the case exactly? If they do it to remove dirty bitmap > from backing image after snapshot operation, probably we'd better improve > block-dirty-bitmap-remove command to be able to reopen r-o image? > > (I just recently faced such a task) I think it was to switch nodes between read-only and read-write, but I don't remember the exact context. Peter, can you add the details? > > Given that the soft freeze is coming closer (March 16), I wonder if we > > should just make this API change and declare the interface stable. We > > can then make Vladimir's fixes and the file reopening on top of it - if > > it's in time for 6.0, that would be good, but if not we could move it to > > 6.1 without impacting libvirt. > > > > I think we're reasonable confident that the QAPI interfaces are right, > > even if maybe not that all aspects of the implementation are right yet. > > > > What do you think? > > I think it's OK.. We have it since 4.0. What will we win keeping -x > for years? Even latest change from updating one device to several > could be easily done with help of 'alternate' if the command was > already stable. I think your series is kind of important to really call the implementation stable. We can always feature flags to indicate the fixes if necessary, but it would still feel better to declare something stable that doesn't have known bugs. :-) Do you think your series will still take a while? Maybe my first comments sounded a bit negative because it was really hard to review at first without knowing the final state, but after all I think the approach is sane and apart from some implementation details, we're not that far away from getting it into a mergable state. Kevin
01.03.2021 14:07, Kevin Wolf wrote: > Am 25.02.2021 um 18:02 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 24.02.2021 15:33, Kevin Wolf wrote: >>> Am 09.02.2021 um 09:03 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> 08.02.2021 21:44, Alberto Garcia wrote: >>>>> Signed-off-by: Alberto Garcia <berto@igalia.com> >>>>> --- >>>>> qapi/block-core.json | 2 +- >>>>> include/block/block.h | 1 + >>>>> block.c | 16 +++++-- >>>>> blockdev.c | 85 +++++++++++++++++++++----------------- >>>>> tests/qemu-iotests/155 | 9 ++-- >>>>> tests/qemu-iotests/165 | 4 +- >>>>> tests/qemu-iotests/245 | 27 +++++++----- >>>>> tests/qemu-iotests/248 | 2 +- >>>>> tests/qemu-iotests/248.out | 2 +- >>>>> tests/qemu-iotests/298 | 4 +- >>>>> 10 files changed, 89 insertions(+), 63 deletions(-) >>>>> >>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>>> index c0e7c23331..b9fcf20a81 100644 >>>>> --- a/qapi/block-core.json >>>>> +++ b/qapi/block-core.json >>>>> @@ -4177,7 +4177,7 @@ >>>>> # Since: 4.0 >>>>> ## >>>>> { 'command': 'x-blockdev-reopen', >>>>> - 'data': 'BlockdevOptions', 'boxed': true } >>>>> + 'data': { 'options': ['BlockdevOptions'] } } >>>> >>>> Do we also want to drop x- prefix? >>> >>> libvirt really wants to have a stable blockdev-reopen interface in 6.0 >>> because enabling the incremental backup code depends on this (they just >>> toggle the readonly flag if I understand correctly, so most of the work >>> we're currently doing isn't even relevant at this moment for libvirt). >> >> Do you know what is the case exactly? If they do it to remove dirty bitmap >> from backing image after snapshot operation, probably we'd better improve >> block-dirty-bitmap-remove command to be able to reopen r-o image? >> >> (I just recently faced such a task) > > I think it was to switch nodes between read-only and read-write, but I > don't remember the exact context. > I already don't think that making implicit reopen-to-rw is a good idea. It's OK for blockdev-commit, but may be unexpected for bitmaps manipulation. > >>> Given that the soft freeze is coming closer (March 16), I wonder if we >>> should just make this API change and declare the interface stable. We >>> can then make Vladimir's fixes and the file reopening on top of it - if >>> it's in time for 6.0, that would be good, but if not we could move it to >>> 6.1 without impacting libvirt. >>> >>> I think we're reasonable confident that the QAPI interfaces are right, >>> even if maybe not that all aspects of the implementation are right yet. >>> >>> What do you think? >> >> I think it's OK.. We have it since 4.0. What will we win keeping -x >> for years? Even latest change from updating one device to several >> could be easily done with help of 'alternate' if the command was >> already stable. > > I think your series is kind of important to really call the > implementation stable. We can always feature flags to indicate the fixes > if necessary, but it would still feel better to declare something stable > that doesn't have known bugs. :-) > > Do you think your series will still take a while? Maybe my first > comments sounded a bit negative because it was really hard to review at > first without knowing the final state, but after all I think the > approach is sane and apart from some implementation details, we're not > that far away from getting it into a mergable state. > Thanks :) I'm now busy with our bugs for Virtuozzo release.. Still, I hope, I'll have a chance to reroll permission-update series this week.
On Mon, Mar 01, 2021 at 12:07:26 +0100, Kevin Wolf wrote: > Am 25.02.2021 um 18:02 hat Vladimir Sementsov-Ogievskiy geschrieben: > > 24.02.2021 15:33, Kevin Wolf wrote: > > > Am 09.02.2021 um 09:03 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > > 08.02.2021 21:44, Alberto Garcia wrote: > > > > > Signed-off-by: Alberto Garcia <berto@igalia.com> > > > > > --- > > > > > qapi/block-core.json | 2 +- > > > > > include/block/block.h | 1 + > > > > > block.c | 16 +++++-- > > > > > blockdev.c | 85 +++++++++++++++++++++----------------- > > > > > tests/qemu-iotests/155 | 9 ++-- > > > > > tests/qemu-iotests/165 | 4 +- > > > > > tests/qemu-iotests/245 | 27 +++++++----- > > > > > tests/qemu-iotests/248 | 2 +- > > > > > tests/qemu-iotests/248.out | 2 +- > > > > > tests/qemu-iotests/298 | 4 +- > > > > > 10 files changed, 89 insertions(+), 63 deletions(-) > > > > > > > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > > > > index c0e7c23331..b9fcf20a81 100644 > > > > > --- a/qapi/block-core.json > > > > > +++ b/qapi/block-core.json > > > > > @@ -4177,7 +4177,7 @@ > > > > > # Since: 4.0 > > > > > ## > > > > > { 'command': 'x-blockdev-reopen', > > > > > - 'data': 'BlockdevOptions', 'boxed': true } > > > > > + 'data': { 'options': ['BlockdevOptions'] } } > > > > > > > > Do we also want to drop x- prefix? > > > > > > libvirt really wants to have a stable blockdev-reopen interface in 6.0 > > > because enabling the incremental backup code depends on this (they just > > > toggle the readonly flag if I understand correctly, so most of the work > > > we're currently doing isn't even relevant at this moment for libvirt). > > > > Do you know what is the case exactly? If they do it to remove dirty bitmap > > from backing image after snapshot operation, probably we'd better improve > > block-dirty-bitmap-remove command to be able to reopen r-o image? > > > > (I just recently faced such a task) > > I think it was to switch nodes between read-only and read-write, but I > don't remember the exact context. > > Peter, can you add the details? Libvirt indeed has code to drive blockdev-reopen to use it to toggle the read-write state for block dirty bitmap manipulation. Few of the cases where we use it: - after blockjobs to toggle the destination image writable (the blockjobs turns it RO when it finishes) so that we can merge the appropriate bitmaps - for deletion of bitmaps in backing images in the checkpoint APIs Both of the operations really require blockdev-reopen and are required for the incremental backup feature as whole since bitmap manipulation is crucial for it and it doesn't happen automatically. The blockdev code has the facilities to also use blockdev-reopen to change RO/RW state for blockjobs if it will be needed in the future or it can be later used to even replace the auto-read-only feature of the protocol node drivers which were actually added to make -blockdev work in libvirt's s-virt environment properly.
diff --git a/qapi/block-core.json b/qapi/block-core.json index c0e7c23331..b9fcf20a81 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -4177,7 +4177,7 @@ # Since: 4.0 ## { 'command': 'x-blockdev-reopen', - 'data': 'BlockdevOptions', 'boxed': true } + 'data': { 'options': ['BlockdevOptions'] } } ## # @blockdev-del: diff --git a/include/block/block.h b/include/block/block.h index 6dd687a69e..fe4a220da9 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -372,6 +372,7 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name, BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, BlockDriverState *bs, QDict *options, bool keep_old_opts); +void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue); int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp); int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only, Error **errp); diff --git a/block.c b/block.c index 19b62da4af..b4fef2308f 100644 --- a/block.c +++ b/block.c @@ -3933,6 +3933,17 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, NULL, 0, keep_old_opts); } +void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue) +{ + if (bs_queue) { + BlockReopenQueueEntry *bs_entry, *next; + QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) { + g_free(bs_entry); + } + g_free(bs_queue); + } +} + /* * Reopen multiple BlockDriverStates atomically & transactionally. * @@ -4024,10 +4035,7 @@ abort: } cleanup: - QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) { - g_free(bs_entry); - } - g_free(bs_queue); + bdrv_reopen_queue_free(bs_queue); return ret; } diff --git a/blockdev.c b/blockdev.c index 098a05709d..6b688c0f73 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3528,38 +3528,16 @@ fail: visit_free(v); } -void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp) +void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp) { - BlockDriverState *bs; - QObject *obj; - Visitor *v = qobject_output_visitor_new(&obj); - BlockReopenQueue *queue; - QDict *qdict; - - /* Check for the selected node name */ - if (!options->has_node_name) { - error_setg(errp, "Node name not specified"); - goto fail; - } - - bs = bdrv_find_node(options->node_name); - if (!bs) { - error_setg(errp, "Cannot find node named '%s'", options->node_name); - goto fail; - } - - /* Put all options in a QDict and flatten it */ - visit_type_BlockdevOptions(v, NULL, &options, &error_abort); - visit_complete(v, &obj); - qdict = qobject_to(QDict, obj); - - qdict_flatten(qdict); - - /* Perform the reopen operation */ + BlockReopenQueue *queue = NULL; + GSList *aio_ctxs = NULL; + GSList *visitors = NULL; + GSList *drained = NULL; BdrvNextIterator it; - GSList *aio_ctxs = NULL, *ctx; BlockDriverState *it_bs; + /* Acquire all AIO contexts */ for (it_bs = bdrv_first(&it); it_bs; it_bs = bdrv_next(&it)) { AioContext *aio_context = bdrv_get_aio_context(it_bs); @@ -3569,19 +3547,50 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp) } } - bdrv_subtree_drained_begin(bs); - queue = bdrv_reopen_queue(NULL, bs, qdict, false); + /* Add each one of the BDS that we want to reopen to the queue */ + for (; reopen_list != NULL; reopen_list = reopen_list->next) { + BlockdevOptions *options = reopen_list->value; + QDict *qdict; + Visitor *v; + BlockDriverState *bs; + QObject *obj; + + /* Check for the selected node name */ + if (!options->has_node_name) { + error_setg(errp, "Node name not specified"); + goto fail; + } + + bs = bdrv_find_node(options->node_name); + if (!bs) { + error_setg(errp, "Cannot find node named '%s'", options->node_name); + goto fail; + } + + v = qobject_output_visitor_new(&obj); + visitors = g_slist_prepend(visitors, v); + + /* Put all options in a QDict and flatten it */ + visit_type_BlockdevOptions(v, NULL, &options, &error_abort); + visit_complete(v, &obj); + qdict = qobject_to(QDict, obj); + + qdict_flatten(qdict); + + bdrv_subtree_drained_begin(bs); + queue = bdrv_reopen_queue(queue, bs, qdict, false); + drained = g_slist_prepend(drained, bs); + } + + /* Perform the reopen operation */ bdrv_reopen_multiple(queue, errp); - bdrv_subtree_drained_end(bs); - - for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) { - AioContext *aio_context = ctx->data; - aio_context_release(aio_context); - } - g_slist_free(aio_ctxs); + queue = NULL; fail: - visit_free(v); + bdrv_reopen_queue_free(queue); + g_slist_free_full(drained, (GDestroyNotify) bdrv_subtree_drained_end); + g_slist_free_full(aio_ctxs, (GDestroyNotify) aio_context_release); + g_slist_free_full(visitors, (GDestroyNotify) visit_free); } void qmp_blockdev_del(const char *node_name, Error **errp) diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155 index 988f986144..5271f9541f 100755 --- a/tests/qemu-iotests/155 +++ b/tests/qemu-iotests/155 @@ -260,9 +260,12 @@ class TestBlockdevMirrorReopen(MirrorBaseClass): result = self.vm.qmp('blockdev-add', node_name="backing", driver="null-co") self.assert_qmp(result, 'return', {}) - result = self.vm.qmp('x-blockdev-reopen', node_name="target", - driver=iotests.imgfmt, file="target-file", - backing="backing") + result = self.vm.qmp('x-blockdev-reopen', options = [{ + 'node-name': "target", + 'driver': iotests.imgfmt, + 'file': "target-file", + 'backing': "backing" + }]) self.assert_qmp(result, 'return', {}) class TestBlockdevMirrorReopenIothread(TestBlockdevMirrorReopen): diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165 index fb56a769b4..32db5086e1 100755 --- a/tests/qemu-iotests/165 +++ b/tests/qemu-iotests/165 @@ -136,7 +136,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase): assert sha256_1 == self.getSha256() # Reopen to RW - result = self.vm.qmp('x-blockdev-reopen', **{ + result = self.vm.qmp('x-blockdev-reopen', options = [{ 'node-name': 'node0', 'driver': iotests.imgfmt, 'file': { @@ -144,7 +144,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase): 'filename': disk }, 'read-only': False - }) + }]) self.assert_qmp(result, 'return', {}) # Check that bitmap is reopened to RW and we can write to it. diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index bad6911f0c..850c9f070b 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -84,8 +84,18 @@ class TestBlockdevReopen(iotests.QMPTestCase): "Expected output of %d qemu-io commands, found %d" % (found, self.total_io_cmds)) - # Run x-blockdev-reopen with 'opts' but applying 'newopts' - # on top of it. The original 'opts' dict is unmodified + # Run x-blockdev-reopen on a list of block devices + def reopenMultiple(self, opts, errmsg = None): + result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options = opts) + if errmsg: + self.assert_qmp(result, 'error/class', 'GenericError') + self.assert_qmp(result, 'error/desc', errmsg) + else: + self.assert_qmp(result, 'return', {}) + + # Run x-blockdev-reopen on a single block device (specified by + # 'opts') but applying 'newopts' on top of it. The original 'opts' + # dict is unmodified def reopen(self, opts, newopts = {}, errmsg = None): opts = copy.deepcopy(opts) @@ -100,12 +110,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): subdict = opts[prefix] subdict[key] = value - result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, **opts) - if errmsg: - self.assert_qmp(result, 'error/class', 'GenericError') - self.assert_qmp(result, 'error/desc', errmsg) - else: - self.assert_qmp(result, 'return', {}) + self.reopenMultiple([ opts ], errmsg) # Run query-named-block-nodes and return the specified entry @@ -141,10 +146,10 @@ class TestBlockdevReopen(iotests.QMPTestCase): # We cannot change any of these self.reopen(opts, {'node-name': 'not-found'}, "Cannot find node named 'not-found'") self.reopen(opts, {'node-name': ''}, "Cannot find node named ''") - self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'node-name', expected: string") + self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'options[0].node-name', expected: string") self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'") self.reopen(opts, {'driver': ''}, "Invalid parameter ''") - self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string") + self.reopen(opts, {'driver': None}, "Invalid parameter type for 'options[0].driver', expected: string") self.reopen(opts, {'file': 'not-found'}, "Cannot find device= nor node_name=not-found") self.reopen(opts, {'file': ''}, "Cannot find device= nor node_name=") self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef") @@ -153,7 +158,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): self.reopen(opts, {'file.filename': hd_path[1]}, "Cannot change the option 'filename'") self.reopen(opts, {'file.aio': 'native'}, "Cannot change the option 'aio'") self.reopen(opts, {'file.locking': 'off'}, "Cannot change the option 'locking'") - self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'file.filename', expected: string") + self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'options[0].file.filename', expected: string") # node-name is optional in BlockdevOptions, but x-blockdev-reopen needs it del opts['node-name'] diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248 index 18ba03467e..2b43853183 100755 --- a/tests/qemu-iotests/248 +++ b/tests/qemu-iotests/248 @@ -62,7 +62,7 @@ vm.get_qmp_events() del blockdev_opts['file']['size'] vm.qmp_log('x-blockdev-reopen', filters=[filter_qmp_testfiles], - **blockdev_opts) + options = [ blockdev_opts ]) vm.qmp_log('block-job-resume', device='drive0') vm.event_wait('JOB_STATUS_CHANGE', timeout=1.0, diff --git a/tests/qemu-iotests/248.out b/tests/qemu-iotests/248.out index 369b25bf26..893f625347 100644 --- a/tests/qemu-iotests/248.out +++ b/tests/qemu-iotests/248.out @@ -2,7 +2,7 @@ {"return": {}} {"execute": "blockdev-mirror", "arguments": {"device": "drive0", "on-target-error": "enospc", "sync": "full", "target": "target"}} {"return": {}} -{"execute": "x-blockdev-reopen", "arguments": {"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}} +{"execute": "x-blockdev-reopen", "arguments": {"options": [{"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}]}} {"return": {}} {"execute": "block-job-resume", "arguments": {"device": "drive0"}} {"return": {}} diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298 index d535946b5f..4efdb35b91 100644 --- a/tests/qemu-iotests/298 +++ b/tests/qemu-iotests/298 @@ -98,7 +98,7 @@ class TestPreallocateFilter(TestPreallocateBase): self.check_big() def test_reopen_opts(self): - result = self.vm.qmp('x-blockdev-reopen', **{ + result = self.vm.qmp('x-blockdev-reopen', options = [{ 'node-name': 'disk', 'driver': iotests.imgfmt, 'file': { @@ -112,7 +112,7 @@ class TestPreallocateFilter(TestPreallocateBase): 'filename': disk } } - }) + }]) self.assert_qmp(result, 'return', {}) self.vm.hmp_qemu_io('drive0', 'write 0 1M')
Signed-off-by: Alberto Garcia <berto@igalia.com> --- qapi/block-core.json | 2 +- include/block/block.h | 1 + block.c | 16 +++++-- blockdev.c | 85 +++++++++++++++++++++----------------- tests/qemu-iotests/155 | 9 ++-- tests/qemu-iotests/165 | 4 +- tests/qemu-iotests/245 | 27 +++++++----- tests/qemu-iotests/248 | 2 +- tests/qemu-iotests/248.out | 2 +- tests/qemu-iotests/298 | 4 +- 10 files changed, 89 insertions(+), 63 deletions(-)