Re: [PATCH v2 2/7] block: Relax restrictions for blockdev-snapshot

From: Kevin Wolf <kwolf@redhat.com>
To: Peter Krempa <pkrempa@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com
Subject: Re: [PATCH v2 2/7] block: Relax restrictions for blockdev-snapshot
Date: Tue, 10 Mar 2020 14:27:28 +0100	[thread overview]
Message-ID: <20200310132728.GE6926@linux.fritz.box> (raw)
In-Reply-To: <20200310131538.GZ1320660@angien.pipo.sk>

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

Thanks!

Kevin