Am 10.03.2020 um 14:15 hat Peter Krempa geschrieben: > On Tue, Mar 10, 2020 at 12:38:26 +0100, Kevin Wolf wrote: > > blockdev-snapshot returned an error if the overlay was already in use, > > which it defined as having any BlockBackend parent. This is in fact both > > too strict (some parents can tolerate the change of visible data caused > > by attaching a backing file) and too loose (some non-BlockBackend > > parents may not be happy with it). > > > > One important use case that is prevented by the too strict check is live > > storage migration with blockdev-mirror. Here, the target node is > > usually opened without a backing file so that the active layer is > > mirrored while its backing chain can be copied in the background. > > > > The backing chain should be attached to the mirror target node when > > finalising the job, just before switching the users of the source node > > to the new copy (at which point the mirror job still has a reference to > > the node). drive-mirror did this automatically, but with blockdev-mirror > > this is the job of the QMP client, so it needs a way to do this. > > > > blockdev-snapshot is the obvious way, so this patch makes it work in > > this scenario. The new condition is that no parent uses CONSISTENT_READ > > permissions. This will ensure that the operation will still be blocked > > when the node is attached to the guest device, so blockdev-snapshot > > remains safe. > > > > (For the sake of completeness, x-blockdev-reopen can be used to achieve > > the same, however it is a big hammer, performs the graph change > > completely unchecked and is still experimental. So even with the option > > of using x-blockdev-reopen, there are reasons why blockdev-snapshot > > should be able to perform this operation.) > > > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > > --- > > blockdev.c | 14 ++++++++------ > > tests/qemu-iotests/085.out | 4 ++-- > > 2 files changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/blockdev.c b/blockdev.c > > index 3e44fa766b..bba0e9775b 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -1501,6 +1501,7 @@ static void external_snapshot_prepare(BlkActionState > > *common, > > TransactionAction *action = common->action; > > AioContext *aio_context; > > AioContext *old_context; > > + uint64_t perm, shared; > > int ret; > > > > /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar > > @@ -1616,16 +1617,17 @@ static void > > external_snapshot_prepare(BlkActionState *common, > > goto out; > > } > > > > - if (bdrv_has_blk(state->new_bs)) { > > + /* > > + * Allow attaching a backing file to an overlay that's already in use > > only > > + * if the parents don't assume that they are already seeing a valid > > image. > > + * (Specifically, allow it as a mirror target, which is write-only > > access.) > > + */ > > + bdrv_get_cumulative_perm(state->new_bs, &perm, &shared); > > + if (perm & BLK_PERM_CONSISTENT_READ) { > > error_setg(errp, "The overlay is already in use"); > > goto out; > > } > > Moving this code a bit further down ... > > > > > - if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, > > - errp)) { > > - goto out; > > - } > > - > > if (state->new_bs->backing != NULL) { > > error_setg(errp, "The overlay already has a backing image"); > > goto out; > > diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out > > index d94ad22f70..fd11aae678 100644 > > --- a/tests/qemu-iotests/085.out > > +++ b/tests/qemu-iotests/085.out > > @@ -82,7 +82,7 @@ Formatting 'TEST_DIR/12-snapshot-v0.IMGFMT', fmt=IMGFMT > > size=134217728 backing_f > > === Invalid command - cannot create a snapshot using a file BDS === > > > > { 'execute': 'blockdev-snapshot', 'arguments': { 'node':'virtio0', > > 'overlay':'file_12' } } > > -{"error": {"class": "GenericError", "desc": "The overlay does not support > > backing images"}} > > +{"error": {"class": "GenericError", "desc": "The overlay is already in > > use"}} > > > > === Invalid command - snapshot node used as active layer === > > > > @@ -96,7 +96,7 @@ Formatting 'TEST_DIR/12-snapshot-v0.IMGFMT', fmt=IMGFMT > > size=134217728 backing_f > > === Invalid command - snapshot node used as backing hd === > > > > { 'execute': 'blockdev-snapshot', 'arguments': { 'node': 'virtio0', > > 'overlay':'snap_11' } } > > -{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node > > is used as backing hd of 'snap_12'"}} > > +{"error": {"class": "GenericError", "desc": "The overlay is already in > > use"}} > > Would probably prevent these test changes.
It would prevent the first one, though I don't think it's important which of the two checks make it error out. The second message came from the bdrv_op_is_blocked() that this patch removes, so this change would be there anyway. > Reviewed-by: Peter Krempa <pkre...@redhat.com> > Tested-by: Peter Krempa <pkre...@redhat.com> Thanks! Kevin