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. Reviewed-by: Peter Krempa <pkre...@redhat.com> Tested-by: Peter Krempa <pkre...@redhat.com>