[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/7] block: Relax restrictions for blockdev-snapshot
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v2 2/7] block: Relax restrictions for blockdev-snapshot |
Date: |
Tue, 10 Mar 2020 14:27:28 +0100 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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 <address@hidden>
> > ---
> > 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 <address@hidden>
> Tested-by: Peter Krempa <address@hidden>
Thanks!
Kevin
- [PATCH v2 0/7] block: Relax restrictions for blockdev-snapshot, Kevin Wolf, 2020/03/10
- [PATCH v2 1/7] block: Make bdrv_get_cumulative_perm() public, Kevin Wolf, 2020/03/10
- [PATCH v2 2/7] block: Relax restrictions for blockdev-snapshot, Kevin Wolf, 2020/03/10
- [PATCH v2 3/7] iotests: Fix run_job() with use_log=False, Kevin Wolf, 2020/03/10
- [PATCH v2 5/7] block: Fix cross-AioContext blockdev-snapshot, Kevin Wolf, 2020/03/10
- [PATCH v2 4/7] iotests: Test mirror with temporarily disabled target backing file, Kevin Wolf, 2020/03/10
- [PATCH v2 6/7] iotests: Add iothread cases to 155, Kevin Wolf, 2020/03/10
- [PATCH v2 7/7] qapi: Add '@allow-write-only-overlay' feature for 'blockdev-snapshot', Kevin Wolf, 2020/03/10
- Re: [PATCH v2 0/7] block: Relax restrictions for blockdev-snapshot, Kevin Wolf, 2020/03/10