[Qemu-devel] [PATCH v4 0/7] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD

[Qemu-devel] [PATCH v4 0/7] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD

[Fam Zheng]

This series adds for point-in-time snapshot NBD exporting based on
blockdev-backup (variant of drive-backup with existing device as target).

We get a thin point-in-time snapshot by COW mechanism of drive-backup, and
export it through built in NBD server. The steps are as below:

 1. (SHELL) qemu-img create -f qcow2 BACKUP.qcow2 <source size here>

    (Alternatively we can use -o backing_file=RUNNING-VM.img to omit explicitly
    providing the size by ourselves, but it's risky because RUNNING-VM.qcow2 is
    used r/w by guest. Whether or not setting backing file in the image file
    doesn't matter, as we are going to override the backing hd in the next
    step)

 2. (QMP) blockdev-add backing=source-drive file.driver=file file.filename=BACKUP.qcow2 id=target0 if=none driver=qcow2

    (where ide0-hd0 is the running BlockDriverState name for
    RUNNING-VM.img. This patch implements "backing=" option to override
    backing_hd for added drive)

 3. (QMP) blockdev-backup device=source-drive sync=none target=target0

    (this is the QMP command introduced by this series, which use a named
    device as target of drive-backup)

 4. (QMP) nbd-server-add device=target0

When image fleecing done:

 1. (QMP) block-job-complete device=ide0-hd0

 2. (HMP) drive_del target0

 3. (SHELL) rm BACKUP.qcow2

v4: Dropping RFC, this series tries to address the crashing cases with an added
    safety mechanics.
    
    In the first half of series, replace the in_use flag with an operation
    blocker list, and block all operations on named backing hd:

    [01/07] qapi: Add BlockOperationType enum
    [02/07] block: Introduce op_blockers to BlockDriverState
    [03/07] block: Replace in_use with operation blocker
    [04/07] block: Add checks of blocker in block operations

    The second half enables point in time snapshot over NBD, with fixes from
    last revision:

    [05/07] block: Parse "backing" option to reference existing BDS
            Fix NULL dereference if device not found.
    [06/07] qmp: add command 'blockdev-backup'
            Moved some checks into backup_run. (Paolo)
    [07/07] block: Allow backup on referenced named BlockDriverState
            New. Removes one specific blocker on backing referenced BDS, so we
            can start a backup job on it.

v3: Base on blockdev-add.
    The syntax blockdev-add backing=<id> is new: This will make referenced BDS
    in the middle of a backing chain, which has significant effects over all
    existing block operations in QMP. It needs to be reviewed and tested carefully.

    I'm listing the commands here that can take a device id as parameter (but
    may not be complete).

    These commands do not mutate the backing chain (not directly, at least) and
    should be safe:
        block_passwd
        block_set_io_throttle
        block-job-set-speed
        block-job-cancel
        block-job-pause
        block-job-resume
        block-job-complete
        drive-backup
        blockdev-snapshot-sync
        blockdev-snapshot-internal-sync
        blockdev-snapshot-delete-internal-sync: These should be safe.
        nbd-server-add

    These can mutates the chain (removing, closing or swapping BDS), need more
    work to convert them to safe operations with a BDS in the middle.
        device_del
        eject: it does bdrv_close the device.
        change: internally calls eject.
        block-commit: it deletes intermediate BDSes, which may include other named BDS.
        block-stream: 
        drive-mirror: it swaps active with target when complete.

    Resizing a middle BDS need to be reviewed too:
        block_resize: TBD.

    Adding and backing HD referencing will put other BDS in middle, but should
    not immediately break things:
        blockdev-add


Fam Zheng (7):
  qapi: Add BlockOperationType enum
  block: Introduce op_blockers to BlockDriverState
  block: Replace in_use with operation blocker
  block: Add checks of blocker in block operations
  block: Parse "backing" option to reference existing BDS
  qmp: add command 'blockdev-backup'
  block: Allow backup on referenced named BlockDriverState

 block-migration.c               |   8 ++-
 block.c                         | 124 ++++++++++++++++++++++++++++++++++------
 block/backup.c                  |  22 +++++++
 blockdev-nbd.c                  |   4 ++
 blockdev.c                      | 100 ++++++++++++++++++++++++++++----
 blockjob.c                      |  12 ++--
 hw/block/dataplane/virtio-blk.c |  16 ++++--
 include/block/block.h           |   8 ++-
 include/block/block_int.h       |   9 ++-
 include/block/blockjob.h        |   3 +
 qapi-schema.json                |  74 ++++++++++++++++++++++++
 qmp-commands.hx                 |  46 +++++++++++++++
 12 files changed, 384 insertions(+), 42 deletions(-)

-- 
1.8.4.2

[Qemu-devel] [PATCH v4 1/7] qapi: Add BlockOperationType enum

[Fam Zheng]

This adds the enum of all the operations that can be taken on a block
device.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qapi-schema.json | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index 83fa485..4656e8c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1440,6 +1440,31 @@
   'data': ['commit', 'stream', 'mirror', 'backup'] }
 
 ##
+# @BlockOperationType
+# Type of a block operation.
+#
+# Since: 1.8
+##
+{ 'enum': 'BlockOpType',
+  'data': [
+    'backup',
+    'change',
+    'commit',
+    'dataplane',
+    'drive-del',
+    'eject',
+    'external-snapshot',
+    'internal-snapshot',
+    'internal-snapshot-delete',
+    'mirror',
+    'nbd-server-add',
+    'passwd',
+    'resize',
+    'set-io-throttle',
+    'stream'
+] }
+
+##
 # @BlockJobInfo:
 #
 # Information about a long-running block device operation.
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v4 1/7] qapi: Add BlockOperationType enum

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On 11/21/2013 10:24 PM, Fam Zheng wrote:
> This adds the enum of all the operations that can be taken on a block
> device.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qapi-schema.json | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)


>  
>  ##
> +# @BlockOperationType
> +# Type of a block operation.
> +#
> +# Since: 1.8

It might be worth adding a bit more docs, as in one line per enum value
with a cross-reference to the command(s) that can trigger that
operation.  Something like:

@backup: See the 'drive-backup' command.
@change: See the 'change' command.

This is especially true if we later add new block ops later than 1.8, so
such additions can have the useful '(since 1.9)' notation.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v4 1/7] qapi: Add BlockOperationType enum

[Kevin Wolf]

Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben:
> This adds the enum of all the operations that can be taken on a block
> device.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Okay, so now I've read the whole series and I still couldn't solve this
mystery: Why is this a QAPI type and not some internal enum?

Kevin

Re: [Qemu-devel] [PATCH v4 1/7] qapi: Add BlockOperationType enum

[Fam Zheng]

On 2013年11月25日 19:26, Kevin Wolf wrote:
> Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben:
>> This adds the enum of all the operations that can be taken on a block
>> device.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>
> Okay, so now I've read the whole series and I still couldn't solve this
> mystery: Why is this a QAPI type and not some internal enum?
>
The logic is becoming complicated, I think even more rules will come 
with Quorum and BlockBackend, etc. By then we probably will have to add 
an interface to query the blockers, so user knows what operation is 
possible and what is not, without trial and error.

Eric, any ideas?

Fam

[Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Fam Zheng]

BlockDriverState.op_blockers is an array of list with BLOCK_OP_TYPE_MAX
elements. Each list is a list of blockers of an operation type
(BlockOpType), that marks this BDS is currently blocked for certain type
of operation with reason errors stored in the list. The rule of usage
is:

 * BDS user who wants to take an operation should check if there's any
   blocker of the type with bdrv_op_is_blocked().

 * BDS user who wants to block certain types of operation, should call
   bdrv_op_block (or bdrv_op_block_all to block all types of operations,
   which is similar to bdrv_set_in_use of now).

 * A blocker is only referenced by op_blockers, so the lifecycle is
   managed by caller, and shouldn't be lost until unblock, so typically
   a caller does these:

   - Allocate a blocker with error_setg or similar, call bdrv_op_block()
     to block some operations.
   - Hold the blocker, do his job.
   - Unblock operations that it blocked, with the same reason pointer
     passed to bdrv_op_unblock().
   - Release the blocker with error_free().

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                   | 55 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  6 ++++++
 include/block/block_int.h |  5 +++++
 3 files changed, 66 insertions(+)

diff --git a/block.c b/block.c
index 382ea71..2b18a43 100644
--- a/block.c
+++ b/block.c
@@ -4425,6 +4425,61 @@ void bdrv_unref(BlockDriverState *bs)
     }
 }
 
+struct BdrvOpBlocker {
+    Error *reason;
+    QLIST_ENTRY(BdrvOpBlocker) list;
+};
+
+bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
+{
+    BdrvOpBlocker *blocker;
+    assert(op >=0 && op < BLOCK_OP_TYPE_MAX);
+    if (!QLIST_EMPTY(&bs->op_blockers[op])) {
+        blocker = QLIST_FIRST(&bs->op_blockers[op]);
+        *errp = error_copy(blocker->reason);
+        return true;
+    }
+    return false;
+}
+
+void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+    BdrvOpBlocker *blocker;
+    assert(op >=0 && op < BLOCK_OP_TYPE_MAX);
+
+    blocker = g_malloc0(sizeof(BdrvOpBlocker));
+    blocker->reason = reason;
+    QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
+}
+
+void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+    BdrvOpBlocker *blocker, *next;
+    assert(op >=0 && op < BLOCK_OP_TYPE_MAX);
+    QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {
+        if (blocker->reason == reason) {
+            QLIST_REMOVE(blocker, list);
+            g_free(blocker);
+        }
+    }
+}
+
+void bdrv_op_block_all(BlockDriverState *bs, Error *reason)
+{
+    int i;
+    for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+        bdrv_op_block(bs, i, reason);
+    }
+}
+
+void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason)
+{
+    int i;
+    for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+        bdrv_op_unblock(bs, i, reason);
+    }
+}
+
 void bdrv_set_in_use(BlockDriverState *bs, int in_use)
 {
     assert(bs->in_use != in_use);
diff --git a/include/block/block.h b/include/block/block.h
index 3560deb..693d305 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -403,6 +403,12 @@ void bdrv_unref(BlockDriverState *bs);
 void bdrv_set_in_use(BlockDriverState *bs, int in_use);
 int bdrv_in_use(BlockDriverState *bs);
 
+bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
+void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
+void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason);
+void bdrv_op_block_all(BlockDriverState *bs, Error *reason);
+void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason);
+
 #ifdef CONFIG_LINUX_AIO
 int raw_get_aio_fd(BlockDriverState *bs);
 #else
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1666066..d8cef85 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -230,6 +230,8 @@ struct BlockDriver {
     QLIST_ENTRY(BlockDriver) list;
 };
 
+typedef struct BdrvOpBlocker BdrvOpBlocker;
+
 /*
  * Note: the function bdrv_append() copies and swaps contents of
  * BlockDriverStates, so if you add new fields to this struct, please
@@ -308,6 +310,9 @@ struct BlockDriverState {
 
     QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
 
+    /* operation blockers */
+    QLIST_HEAD(, BdrvOpBlocker) op_blockers[BLOCK_OP_TYPE_MAX];
+
     /* long-running background operation */
     BlockJob *job;
 
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Stefan Hajnoczi]

On Fri, Nov 22, 2013 at 01:24:49PM +0800, Fam Zheng wrote:
> +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
> +{
> +    BdrvOpBlocker *blocker;
> +    assert(op >=0 && op < BLOCK_OP_TYPE_MAX);
> +    if (!QLIST_EMPTY(&bs->op_blockers[op])) {
> +        blocker = QLIST_FIRST(&bs->op_blockers[op]);
> +        *errp = error_copy(blocker->reason);
> +        return true;
> +    }
> +    return false;
> +}

It's worth following the convention that Error **errp may be NULL:

if (errp) {
    *errp = error_copy(blocker->reason);
}

The bool return value might be enough for some callers who don't need
the full Error.

Re: [Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Kevin Wolf]

Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben:
> BlockDriverState.op_blockers is an array of list with BLOCK_OP_TYPE_MAX

s/list/lists/

> elements. Each list is a list of blockers of an operation type
> (BlockOpType), that marks this BDS is currently blocked for certain type

s/is/as/
s/for/for a/

> of operation with reason errors stored in the list. The rule of usage
> is:
> 
>  * BDS user who wants to take an operation should check if there's any
>    blocker of the type with bdrv_op_is_blocked().
> 
>  * BDS user who wants to block certain types of operation, should call
>    bdrv_op_block (or bdrv_op_block_all to block all types of operations,
>    which is similar to bdrv_set_in_use of now).
> 
>  * A blocker is only referenced by op_blockers, so the lifecycle is
>    managed by caller, and shouldn't be lost until unblock, so typically
>    a caller does these:
> 
>    - Allocate a blocker with error_setg or similar, call bdrv_op_block()
>      to block some operations.
>    - Hold the blocker, do his job.
>    - Unblock operations that it blocked, with the same reason pointer
>      passed to bdrv_op_unblock().
>    - Release the blocker with error_free().
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c                   | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h     |  6 ++++++
>  include/block/block_int.h |  5 +++++
>  3 files changed, 66 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 382ea71..2b18a43 100644
> --- a/block.c
> +++ b/block.c
> @@ -4425,6 +4425,61 @@ void bdrv_unref(BlockDriverState *bs)
>      }
>  }
>  
> +struct BdrvOpBlocker {
> +    Error *reason;
> +    QLIST_ENTRY(BdrvOpBlocker) list;
> +};
> +
> +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
> +{
> +    BdrvOpBlocker *blocker;
> +    assert(op >=0 && op < BLOCK_OP_TYPE_MAX);

Missing space after >= (same in all other functions)

The logic looks fine.

Kevin

Re: [Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Kevin Wolf]

Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben:
> BlockDriverState.op_blockers is an array of list with BLOCK_OP_TYPE_MAX
> elements. Each list is a list of blockers of an operation type
> (BlockOpType), that marks this BDS is currently blocked for certain type
> of operation with reason errors stored in the list. The rule of usage
> is:
> 
>  * BDS user who wants to take an operation should check if there's any
>    blocker of the type with bdrv_op_is_blocked().
> 
>  * BDS user who wants to block certain types of operation, should call
>    bdrv_op_block (or bdrv_op_block_all to block all types of operations,
>    which is similar to bdrv_set_in_use of now).
> 
>  * A blocker is only referenced by op_blockers, so the lifecycle is
>    managed by caller, and shouldn't be lost until unblock, so typically
>    a caller does these:
> 
>    - Allocate a blocker with error_setg or similar, call bdrv_op_block()
>      to block some operations.
>    - Hold the blocker, do his job.
>    - Unblock operations that it blocked, with the same reason pointer
>      passed to bdrv_op_unblock().
>    - Release the blocker with error_free().
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c                   | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h     |  6 ++++++
>  include/block/block_int.h |  5 +++++
>  3 files changed, 66 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 382ea71..2b18a43 100644
> --- a/block.c
> +++ b/block.c
> @@ -4425,6 +4425,61 @@ void bdrv_unref(BlockDriverState *bs)
>      }
>  }
>  
> +struct BdrvOpBlocker {
> +    Error *reason;
> +    QLIST_ENTRY(BdrvOpBlocker) list;
> +};
> +
> +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
> +{
> +    BdrvOpBlocker *blocker;
> +    assert(op >=0 && op < BLOCK_OP_TYPE_MAX);

Oh, and my compiler doesn't like that check in the first place:

block.c: In function 'bdrv_op_is_blocked':
block.c:4473:5: error: comparison of unsigned expression >= 0 is always
true [-Werror=type-limits]
block.c: In function 'bdrv_op_block':
block.c:4485:5: error: comparison of unsigned expression >= 0 is always
true [-Werror=type-limits]
block.c: In function 'bdrv_op_unblock':
block.c:4495:5: error: comparison of unsigned expression >= 0 is always
true [-Werror=type-limits]

Perhaps cast op to int for the first condition:

    assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);

Kevin

Re: [Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Paolo Bonzini]

Il 22/11/2013 06:24, Fam Zheng ha scritto:
> +void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
> +{
> +    BdrvOpBlocker *blocker;
> +    assert(op >=0 && op < BLOCK_OP_TYPE_MAX);
> +
> +    blocker = g_malloc0(sizeof(BdrvOpBlocker));
> +    blocker->reason = reason;
> +    QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
> +}
> +
> +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)

What about making BlockOpType a bitmask, and having bdrv_op_{,un}block
take multiple ORed BlockOpTypes?

bdrv_op_{,un}block_all then are not necessary, you only need a
BLOCK_OPERATION_ALL value.

Paolo

Re: [Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Fam Zheng]

On 2013年11月26日 01:13, Paolo Bonzini wrote:
> Il 22/11/2013 06:24, Fam Zheng ha scritto:
>> +void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
>> +{
>> +    BdrvOpBlocker *blocker;
>> +    assert(op >=0 && op < BLOCK_OP_TYPE_MAX);
>> +
>> +    blocker = g_malloc0(sizeof(BdrvOpBlocker));
>> +    blocker->reason = reason;
>> +    QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
>> +}
>> +
>> +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
>
> What about making BlockOpType a bitmask, and having bdrv_op_{,un}block
> take multiple ORed BlockOpTypes?
>
> bdrv_op_{,un}block_all then are not necessary, you only need a
> BLOCK_OPERATION_ALL value.
>

Bitmap is not enough, at least it should be an array. For example when 
we enable multiple block jobs, there're two stoppers for drive_del, right?

And a bool or int is not as friendly as an error message, because when 
an operation is blocked we can't print a specific reason when we don't 
have this information at all. So let's be consistent with migration 
blockers.

Fam

Re: [Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Paolo Bonzini]

Il 26/11/2013 03:07, Fam Zheng ha scritto:
>>>
>>> +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error
>>> *reason)
>>
>> What about making BlockOpType a bitmask, and having bdrv_op_{,un}block
>> take multiple ORed BlockOpTypes?
>>
>> bdrv_op_{,un}block_all then are not necessary, you only need a
>> BLOCK_OPERATION_ALL value.
>>
> 
> Bitmap is not enough, at least it should be an array. For example when
> we enable multiple block jobs, there're two stoppers for drive_del, right?

I said bitmask, not bitmap. :)

So that you can add the same Error to multiple items of the array with a
single bdrv_op_block call (by ORing them into the second parameter).

Paolo

Re: [Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Fam Zheng]

On Tue, Nov 26, 2013 at 5:22 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 26/11/2013 03:07, Fam Zheng ha scritto:
>>>>
>>>> +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error
>>>> *reason)
>>>
>>> What about making BlockOpType a bitmask, and having bdrv_op_{,un}block
>>> take multiple ORed BlockOpTypes?
>>>
>>> bdrv_op_{,un}block_all then are not necessary, you only need a
>>> BLOCK_OPERATION_ALL value.
>>>
>>
>> Bitmap is not enough, at least it should be an array. For example when
>> we enable multiple block jobs, there're two stoppers for drive_del, right?
>
> I said bitmask, not bitmap. :)
>

OK. Sorry..

> So that you can add the same Error to multiple items of the array with a
> single bdrv_op_block call (by ORing them into the second parameter).
>

What data type to contain this? I'm not sure 64 is enough in long term...

Fam

Re: [Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Paolo Bonzini]

Il 26/11/2013 11:19, Fam Zheng ha scritto:
>> So that you can add the same Error to multiple items of the array with a
>> single bdrv_op_block call (by ORing them into the second parameter).
> 
> What data type to contain this? I'm not sure 64 is enough in long term...

I would just use an uint64_t, I think you have ~20 operations now.
There is still quite some breathing room.

Paolo

Re: [Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Fam Zheng]

On Tue, Nov 26, 2013 at 6:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 26/11/2013 11:19, Fam Zheng ha scritto:
>>> So that you can add the same Error to multiple items of the array with a
>>> single bdrv_op_block call (by ORing them into the second parameter).
>>
>> What data type to contain this? I'm not sure 64 is enough in long term...
>
> I would just use an uint64_t, I think you have ~20 operations now.
> There is still quite some breathing room.
>
OK. Then, can we still use QAPI enum? Because I think being possible
to query blockers makes sense, that way the user doesn't have to trial
and error to know what operation is blocked.

Fam

Re: [Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Paolo Bonzini]

Il 26/11/2013 11:31, Fam Zheng ha scritto:
>> > I would just use an uint64_t, I think you have ~20 operations now.
>> > There is still quite some breathing room.
>> >
> OK. Then, can we still use QAPI enum? Because I think being possible
> to query blockers makes sense, that way the user doesn't have to trial
> and error to know what operation is blocked.

Yes, we can.  You would still have to add a bunch of

#define BLOCK_OPERATION_MASK_RESIZE (1ULL << BLOCK_OPERATION_TYPE_RESIZE)

in block.h.

Paolo

Re: [Qemu-devel] [PATCH v4 2/7] block: Introduce op_blockers to BlockDriverState

[Fam Zheng]

On Tue, Nov 26, 2013 at 6:41 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 26/11/2013 11:31, Fam Zheng ha scritto:
>>> > I would just use an uint64_t, I think you have ~20 operations now.
>>> > There is still quite some breathing room.
>>> >
>> OK. Then, can we still use QAPI enum? Because I think being possible
>> to query blockers makes sense, that way the user doesn't have to trial
>> and error to know what operation is blocked.
>
> Yes, we can.  You would still have to add a bunch of
>
> #define BLOCK_OPERATION_MASK_RESIZE (1ULL << BLOCK_OPERATION_TYPE_RESIZE)
>
> in block.h.
>

Well that works, but then we have a repeated list of operations in two
places, I'm not sure that would be good to have.

Kevin, Stefan, any opinion?

Fam

[Qemu-devel] [PATCH v4 3/7] block: Replace in_use with operation blocker

[Fam Zheng]

This drops BlockDriverState.in_use with op_blockers:

  - Call bdrv_op_block_all in place of bdrv_set_in_use(bs, 1).
  - Call bdrv_op_unblock_all in place of bdrv_set_in_use(bs, 0).
  - Check bdrv_op_is_blocked() in place of bdrv_in_use(bs).
    The specific types are used, e.g. in place of starting block backup,
    bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP, ...).
  - Check bdrv_op_blocker_is_empty() in place of assert(!bs->in_use).

Note: there is no block for only specific types for now, i.e. a caller
blocks all or none. So although the checks are type specific, the above
changes can still be seen as identical in logic.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block-migration.c               |  8 ++++++--
 block.c                         | 34 +++++++++++++++++++++-------------
 blockdev.c                      | 29 +++++++++++++++++++----------
 blockjob.c                      | 12 +++++++-----
 hw/block/dataplane/virtio-blk.c | 16 ++++++++++------
 include/block/block.h           |  2 --
 include/block/block_int.h       |  1 -
 include/block/blockjob.h        |  3 +++
 8 files changed, 66 insertions(+), 39 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index daf9ec1..12afcfa 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -58,6 +58,8 @@ typedef struct BlkMigDevState {
     /* Protected by block migration lock.  */
     unsigned long *aio_bitmap;
     int64_t completed_sectors;
+
+    Error *blocker;
 } BlkMigDevState;
 
 typedef struct BlkMigBlock {
@@ -336,7 +338,8 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
         bmds->completed_sectors = 0;
         bmds->shared_base = block_mig_state.shared_base;
         alloc_aio_bitmap(bmds);
-        bdrv_set_in_use(bs, 1);
+        error_setg(&bmds->blocker, "block device is in use by migration");
+        bdrv_op_block_all(bs, bmds->blocker);
         bdrv_ref(bs);
 
         block_mig_state.total_sector_sum += sectors;
@@ -574,7 +577,8 @@ static void blk_mig_cleanup(void)
     blk_mig_lock();
     while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
         QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
-        bdrv_set_in_use(bmds->bs, 0);
+        bdrv_op_unblock_all(bmds->bs, bmds->blocker);
+        error_free(bmds->blocker);
         bdrv_unref(bmds->bs);
         g_free(bmds->aio_bitmap);
         g_free(bmds);
diff --git a/block.c b/block.c
index 2b18a43..3bf4c8a 100644
--- a/block.c
+++ b/block.c
@@ -1628,15 +1628,17 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     bs_dest->refcnt             = bs_src->refcnt;
 
     /* job */
-    bs_dest->in_use             = bs_src->in_use;
     bs_dest->job                = bs_src->job;
 
     /* keep the same entry in bdrv_states */
     pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
             bs_src->device_name);
+    memcpy(bs_dest->op_blockers, bs_src->op_blockers,
+           sizeof(bs_dest->op_blockers));
     bs_dest->list = bs_src->list;
 }
 
+static bool bdrv_op_blocker_is_empty(BlockDriverState *bs);
 /*
  * Swap bs contents for two image chains while they are live,
  * while keeping required fields on the BlockDriverState that is
@@ -1658,7 +1660,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
     assert(bs_new->dirty_bitmap == NULL);
     assert(bs_new->job == NULL);
     assert(bs_new->dev == NULL);
-    assert(bs_new->in_use == 0);
+    assert(bdrv_op_blocker_is_empty(bs_new));
     assert(bs_new->io_limits_enabled == false);
     assert(!throttle_have_timer(&bs_new->throttle_state));
 
@@ -1677,7 +1679,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
     /* Check a few fields that should remain attached to the device */
     assert(bs_new->dev == NULL);
     assert(bs_new->job == NULL);
-    assert(bs_new->in_use == 0);
+    assert(bdrv_op_blocker_is_empty(bs_new));
     assert(bs_new->io_limits_enabled == false);
     assert(!throttle_have_timer(&bs_new->throttle_state));
 
@@ -1714,7 +1716,7 @@ static void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);
     assert(!bs->job);
-    assert(!bs->in_use);
+    assert(bdrv_op_blocker_is_empty(bs));
     assert(!bs->refcnt);
 
     bdrv_close(bs);
@@ -1887,6 +1889,7 @@ int bdrv_commit(BlockDriverState *bs)
     int ret = 0;
     uint8_t *buf;
     char filename[PATH_MAX];
+    Error *local_err;
 
     if (!drv)
         return -ENOMEDIUM;
@@ -1895,7 +1898,9 @@ int bdrv_commit(BlockDriverState *bs)
         return -ENOTSUP;
     }
 
-    if (bdrv_in_use(bs) || bdrv_in_use(bs->backing_hd)) {
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, &local_err) ||
+        bdrv_op_is_blocked(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, &local_err)) {
+        error_free(local_err);
         return -EBUSY;
     }
 
@@ -2831,6 +2836,7 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
 int bdrv_truncate(BlockDriverState *bs, int64_t offset)
 {
     BlockDriver *drv = bs->drv;
+    Error *local_err;
     int ret;
     if (!drv)
         return -ENOMEDIUM;
@@ -2838,8 +2844,10 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
         return -ENOTSUP;
     if (bs->read_only)
         return -EACCES;
-    if (bdrv_in_use(bs))
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, &local_err)) {
+        error_free(local_err);
         return -EBUSY;
+    }
     ret = drv->bdrv_truncate(bs, offset);
     if (ret == 0) {
         ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
@@ -4480,15 +4488,15 @@ void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason)
     }
 }
 
-void bdrv_set_in_use(BlockDriverState *bs, int in_use)
+static bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
 {
-    assert(bs->in_use != in_use);
-    bs->in_use = in_use;
-}
+    bool ret = true;
+    int i;
 
-int bdrv_in_use(BlockDriverState *bs)
-{
-    return bs->in_use;
+    for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+        ret = ret && QLIST_EMPTY(&bs->op_blockers[i]);
+    }
+    return ret;
 }
 
 void bdrv_iostatus_enable(BlockDriverState *bs)
diff --git a/blockdev.c b/blockdev.c
index 330aa4a..eb55b74 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1224,8 +1224,8 @@ static void external_snapshot_prepare(BlkTransactionState *common,
         return;
     }
 
-    if (bdrv_in_use(state->old_bs)) {
-        error_set(errp, QERR_DEVICE_IN_USE, device);
+    if (bdrv_op_is_blocked(state->old_bs,
+                           BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) {
         return;
     }
 
@@ -1441,10 +1441,10 @@ exit:
 
 static void eject_device(BlockDriverState *bs, int force, Error **errp)
 {
-    if (bdrv_in_use(bs)) {
-        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
         return;
     }
+
     if (!bdrv_dev_has_removable_media(bs)) {
         error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
         return;
@@ -1637,14 +1637,16 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *id = qdict_get_str(qdict, "id");
     BlockDriverState *bs;
+    Error *local_err;
 
     bs = bdrv_find(id);
     if (!bs) {
         qerror_report(QERR_DEVICE_NOT_FOUND, id);
         return -1;
     }
-    if (bdrv_in_use(bs)) {
-        qerror_report(QERR_DEVICE_IN_USE, id);
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
         return -1;
     }
 
@@ -1755,6 +1757,10 @@ void qmp_block_stream(const char *device, bool has_base,
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
+        return;
+    }
+
     if (base) {
         base_bs = bdrv_find_backing_image(bs, base);
         if (base_bs == NULL) {
@@ -1795,6 +1801,10 @@ void qmp_block_commit(const char *device,
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
+        return;
+    }
+
     /* default top_bs is the active layer */
     top_bs = bs;
 
@@ -1881,8 +1891,7 @@ void qmp_drive_backup(const char *device, const char *target,
         }
     }
 
-    if (bdrv_in_use(bs)) {
-        error_set(errp, QERR_DEVICE_IN_USE, device);
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP, errp)) {
         return;
     }
 
@@ -2011,11 +2020,11 @@ void qmp_drive_mirror(const char *device, const char *target,
         }
     }
 
-    if (bdrv_in_use(bs)) {
-        error_set(errp, QERR_DEVICE_IN_USE, device);
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR, errp)) {
         return;
     }
 
+
     flags = bs->open_flags | BDRV_O_RDWR;
     source = bs->backing_hd;
     if (!source && sync == MIRROR_SYNC_MODE_TOP) {
diff --git a/blockjob.c b/blockjob.c
index 9e5fd5c..e198cb2 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -41,13 +41,11 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
 {
     BlockJob *job;
 
-    if (bs->job || bdrv_in_use(bs)) {
+    if (bs->job) {
         error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
         return NULL;
     }
     bdrv_ref(bs);
-    bdrv_set_in_use(bs, 1);
-
     job = g_malloc0(driver->instance_size);
     job->driver        = driver;
     job->bs            = bs;
@@ -64,11 +62,14 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
         if (error_is_set(&local_err)) {
             bs->job = NULL;
             g_free(job);
-            bdrv_set_in_use(bs, 0);
             error_propagate(errp, local_err);
             return NULL;
         }
     }
+    error_setg(&job->blocker, "block device is in use by block job: %s",
+               BlockJobType_lookup[driver->job_type]);
+    bdrv_op_block_all(bs, job->blocker);
+
     return job;
 }
 
@@ -79,8 +80,9 @@ void block_job_completed(BlockJob *job, int ret)
     assert(bs->job == job);
     job->cb(job->opaque, ret);
     bs->job = NULL;
+    bdrv_op_unblock_all(bs, job->blocker);
+    error_free(job->blocker);
     g_free(job);
-    bdrv_set_in_use(bs, 0);
 }
 
 void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index f2d7350..0a7b759 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -69,6 +69,9 @@ struct VirtIOBlockDataPlane {
                                              queue */
 
     unsigned int num_reqs;
+
+    /* Operation blocker on BDS */
+    Error *blocker;
 };
 
 /* Raise an interrupt to signal guest, if necessary */
@@ -385,6 +388,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
 {
     VirtIOBlockDataPlane *s;
     int fd;
+    Error *local_err = NULL;
 
     *dataplane = NULL;
 
@@ -406,8 +410,9 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     /* If dataplane is (re-)enabled while the guest is running there could be
      * block jobs that can conflict.
      */
-    if (bdrv_in_use(blk->conf.bs)) {
-        error_report("cannot start dataplane thread while device is in use");
+    if (bdrv_op_is_blocked(blk->conf.bs, BLOCK_OP_TYPE_DATAPLANE, &local_err)) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
         return false;
     }
 
@@ -422,9 +427,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     s->vdev = vdev;
     s->fd = fd;
     s->blk = blk;
-
-    /* Prevent block operations that conflict with data plane thread */
-    bdrv_set_in_use(blk->conf.bs, 1);
+    error_setg(&s->blocker, "block device is in use by data plane");
+    bdrv_op_block_all(blk->conf.bs, s->blocker);
 
     *dataplane = s;
     return true;
@@ -437,7 +441,7 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     }
 
     virtio_blk_data_plane_stop(s);
-    bdrv_set_in_use(s->blk->conf.bs, 0);
+    bdrv_op_unblock_all(s->blk->conf.bs, s->blocker);
     g_free(s);
 }
 
diff --git a/include/block/block.h b/include/block/block.h
index 693d305..173c09a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -400,8 +400,6 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
 
 void bdrv_ref(BlockDriverState *bs);
 void bdrv_unref(BlockDriverState *bs);
-void bdrv_set_in_use(BlockDriverState *bs, int in_use);
-int bdrv_in_use(BlockDriverState *bs);
 
 bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
 void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index d8cef85..60edc80 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -305,7 +305,6 @@ struct BlockDriverState {
     char device_name[32];
     HBitmap *dirty_bitmap;
     int refcnt;
-    int in_use; /* users other than guest access, eg. block migration */
     QTAILQ_ENTRY(BlockDriverState) list;
 
     QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index d76de62..c0a7875 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -106,6 +106,9 @@ struct BlockJob {
     /** The completion function that will be called when the job completes.  */
     BlockDriverCompletionFunc *cb;
 
+    /** Block other operations when block job is running */
+    Error *blocker;
+
     /** The opaque value that is passed to the completion function.  */
     void *opaque;
 };
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v4 3/7] block: Replace in_use with operation blocker

[Paolo Bonzini]

Il 22/11/2013 06:24, Fam Zheng ha scritto:
> @@ -41,13 +41,11 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>  {
>      BlockJob *job;
>  
> -    if (bs->job || bdrv_in_use(bs)) {
> +    if (bs->job) {
>          error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
>          return NULL;
>      }
>      bdrv_ref(bs);

Why is this check disappearing?

Perhaps a previous patch should hoist the in_use check to the callers of
block_job_create, and here you should just have an assertion that
bs->job == NULL.  Then this patch can change the checks to the
appropriate op.

Paolo

Re: [Qemu-devel] [PATCH v4 3/7] block: Replace in_use with operation blocker

[Fam Zheng]

On 2013年11月26日 01:11, Paolo Bonzini wrote:
> Il 22/11/2013 06:24, Fam Zheng ha scritto:
>> @@ -41,13 +41,11 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>>   {
>>       BlockJob *job;
>>
>> -    if (bs->job || bdrv_in_use(bs)) {
>> +    if (bs->job) {
>>           error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
>>           return NULL;
>>       }
>>       bdrv_ref(bs);
>
> Why is this check disappearing?
>

Because this patch is moving this check to callers, and changing it to 
op blocker check.

Fam

[Qemu-devel] [PATCH v4 4/7] block: Add checks of blocker in block operations

[Fam Zheng]

Before operate on a BlockDriverState, respective types are checked
against bs->op_blockers and it will error out if there's a blocker.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev-nbd.c |  4 ++++
 blockdev.c     | 25 +++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 922cf56..c49feae 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -92,6 +92,10 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_NBD_SERVER_ADD, errp)) {
+        return;
+    }
+
     if (!has_writable) {
         writable = false;
     }
diff --git a/blockdev.c b/blockdev.c
index eb55b74..1921d27 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1001,6 +1001,11 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
         return NULL;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
+                           errp)) {
+        return NULL;
+    }
+
     ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
@@ -1098,6 +1103,10 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) {
+        return;
+    }
+
     if (!bdrv_is_inserted(bs)) {
         error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return;
@@ -1485,6 +1494,10 @@ void qmp_block_passwd(const char *device, const char *password, Error **errp)
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_PASSWD, errp)) {
+        return;
+    }
+
     err = bdrv_set_key(bs, password);
     if (err == -EINVAL) {
         error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
@@ -1536,6 +1549,10 @@ void qmp_change_blockdev(const char *device, const char *filename,
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
+        return;
+    }
+
     if (format) {
         drv = bdrv_find_whitelisted_format(format, bs->read_only);
         if (!drv) {
@@ -1586,6 +1603,10 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_SET_IO_THROTTLE, errp)) {
+        return;
+    }
+
     memset(&cfg, 0, sizeof(cfg));
     cfg.buckets[THROTTLE_BPS_TOTAL].avg = bps;
     cfg.buckets[THROTTLE_BPS_READ].avg  = bps_rd;
@@ -1684,6 +1705,10 @@ void qmp_block_resize(const char *device, int64_t size, Error **errp)
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, errp)) {
+        return;
+    }
+
     if (size < 0) {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
         return;
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v4 4/7] block: Add checks of blocker in block operations

[Paolo Bonzini]

Il 22/11/2013 06:24, Fam Zheng ha scritto:
> Before operate on a BlockDriverState, respective types are checked
> against bs->op_blockers and it will error out if there's a blocker.

How did you choose them?  I understand blocking change (and eject?), but
not blocking set-io-throttle.

In particular, this means that old in_use users, which did not block
commands such as nbd-server-add, will now block it.

Paolo

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev-nbd.c |  4 ++++
>  blockdev.c     | 25 +++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 922cf56..c49feae 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -92,6 +92,10 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>          return;
>      }
>  
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_NBD_SERVER_ADD, errp)) {
> +        return;
> +    }
> +
>      if (!has_writable) {
>          writable = false;
>      }
> diff --git a/blockdev.c b/blockdev.c
> index eb55b74..1921d27 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1001,6 +1001,11 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
>          return NULL;
>      }
>  
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
> +                           errp)) {
> +        return NULL;
> +    }
> +
>      ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
> @@ -1098,6 +1103,10 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
>          return;
>      }
>  
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) {
> +        return;
> +    }
> +
>      if (!bdrv_is_inserted(bs)) {
>          error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>          return;
> @@ -1485,6 +1494,10 @@ void qmp_block_passwd(const char *device, const char *password, Error **errp)
>          return;
>      }
>  
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_PASSWD, errp)) {
> +        return;
> +    }
> +
>      err = bdrv_set_key(bs, password);
>      if (err == -EINVAL) {
>          error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
> @@ -1536,6 +1549,10 @@ void qmp_change_blockdev(const char *device, const char *filename,
>          return;
>      }
>  
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
> +        return;
> +    }
> +
>      if (format) {
>          drv = bdrv_find_whitelisted_format(format, bs->read_only);
>          if (!drv) {
> @@ -1586,6 +1603,10 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
>          return;
>      }
>  
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_SET_IO_THROTTLE, errp)) {
> +        return;
> +    }
> +
>      memset(&cfg, 0, sizeof(cfg));
>      cfg.buckets[THROTTLE_BPS_TOTAL].avg = bps;
>      cfg.buckets[THROTTLE_BPS_READ].avg  = bps_rd;
> @@ -1684,6 +1705,10 @@ void qmp_block_resize(const char *device, int64_t size, Error **errp)
>          return;
>      }
>  
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, errp)) {
> +        return;
> +    }
> +
>      if (size < 0) {
>          error_set(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
>          return;
> 

Re: [Qemu-devel] [PATCH v4 4/7] block: Add checks of blocker in block operations

[Fam Zheng]

On 2013年11月26日 01:15, Paolo Bonzini wrote:
> Il 22/11/2013 06:24, Fam Zheng ha scritto:
>> Before operate on a BlockDriverState, respective types are checked
>> against bs->op_blockers and it will error out if there's a blocker.
>
> How did you choose them?  I understand blocking change (and eject?), but
> not blocking set-io-throttle.
>
> In particular, this means that old in_use users, which did not block
> commands such as nbd-server-add, will now block it.
>

Right, I'll drop nbd, passwd and throttle checks.

Fam

>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>   blockdev-nbd.c |  4 ++++
>>   blockdev.c     | 25 +++++++++++++++++++++++++
>>   2 files changed, 29 insertions(+)
>>
>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>> index 922cf56..c49feae 100644
>> --- a/blockdev-nbd.c
>> +++ b/blockdev-nbd.c
>> @@ -92,6 +92,10 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>>           return;
>>       }
>>
>> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_NBD_SERVER_ADD, errp)) {
>> +        return;
>> +    }
>> +
>>       if (!has_writable) {
>>           writable = false;
>>       }
>> diff --git a/blockdev.c b/blockdev.c
>> index eb55b74..1921d27 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1001,6 +1001,11 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
>>           return NULL;
>>       }
>>
>> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
>> +                           errp)) {
>> +        return NULL;
>> +    }
>> +
>>       ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err);
>>       if (error_is_set(&local_err)) {
>>           error_propagate(errp, local_err);
>> @@ -1098,6 +1103,10 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
>>           return;
>>       }
>>
>> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) {
>> +        return;
>> +    }
>> +
>>       if (!bdrv_is_inserted(bs)) {
>>           error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>>           return;
>> @@ -1485,6 +1494,10 @@ void qmp_block_passwd(const char *device, const char *password, Error **errp)
>>           return;
>>       }
>>
>> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_PASSWD, errp)) {
>> +        return;
>> +    }
>> +
>>       err = bdrv_set_key(bs, password);
>>       if (err == -EINVAL) {
>>           error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
>> @@ -1536,6 +1549,10 @@ void qmp_change_blockdev(const char *device, const char *filename,
>>           return;
>>       }
>>
>> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
>> +        return;
>> +    }
>> +
>>       if (format) {
>>           drv = bdrv_find_whitelisted_format(format, bs->read_only);
>>           if (!drv) {
>> @@ -1586,6 +1603,10 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
>>           return;
>>       }
>>
>> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_SET_IO_THROTTLE, errp)) {
>> +        return;
>> +    }
>> +
>>       memset(&cfg, 0, sizeof(cfg));
>>       cfg.buckets[THROTTLE_BPS_TOTAL].avg = bps;
>>       cfg.buckets[THROTTLE_BPS_READ].avg  = bps_rd;
>> @@ -1684,6 +1705,10 @@ void qmp_block_resize(const char *device, int64_t size, Error **errp)
>>           return;
>>       }
>>
>> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, errp)) {
>> +        return;
>> +    }
>> +
>>       if (size < 0) {
>>           error_set(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
>>           return;
>>
>

[Qemu-devel] [PATCH v4 5/7] block: Parse "backing" option to reference existing BDS

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                   | 37 ++++++++++++++++++++++++++++++++-----
 include/block/block_int.h |  3 +++
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 3bf4c8a..a5da656 100644
--- a/block.c
+++ b/block.c
@@ -1166,11 +1166,33 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     /* If there is a backing file, use it */
     if ((flags & BDRV_O_NO_BACKING) == 0) {
         QDict *backing_options;
-
-        qdict_extract_subqdict(options, &backing_options, "backing.");
-        ret = bdrv_open_backing_file(bs, backing_options, &local_err);
-        if (ret < 0) {
-            goto close_and_fail;
+        const char *backing_bs;
+
+        backing_bs = qdict_get_try_str(options, "backing");
+        if (backing_bs) {
+            bs->backing_hd = bdrv_find(backing_bs);
+            qdict_del(options, "backing");
+            if (bs->backing_hd) {
+                bdrv_ref(bs->backing_hd);
+                assert(!bs->backing_blocker);
+                error_setg(&bs->backing_blocker,
+                           "device is used as backing hd of '%s'",
+                           bs->device_name);
+                bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
+                pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+                        bs->backing_hd->filename);
+                pstrcpy(bs->backing_format, sizeof(bs->backing_format),
+                        bs->backing_hd->drv->format_name);
+            } else {
+                error_setg(errp, "Backing device not found: %s", backing_bs);
+                goto close_and_fail;
+            }
+        } else {
+            qdict_extract_subqdict(options, &backing_options, "backing.");
+            ret = bdrv_open_backing_file(bs, backing_options, &local_err);
+            if (ret < 0) {
+                goto close_and_fail;
+            }
         }
     }
 
@@ -1461,6 +1483,11 @@ void bdrv_close(BlockDriverState *bs)
 
     if (bs->drv) {
         if (bs->backing_hd) {
+            if (error_is_set(&bs->backing_blocker)) {
+                bdrv_op_unblock_all(bs->backing_hd,
+                                    bs->backing_blocker);
+                error_free(bs->backing_blocker);
+            }
             bdrv_unref(bs->backing_hd);
             bs->backing_hd = NULL;
         }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 60edc80..6db30c9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -316,6 +316,9 @@ struct BlockDriverState {
     BlockJob *job;
 
     QDict *options;
+
+    /* For backing reference, block the operations of named backing device */
+    Error *backing_blocker;
 };
 
 int get_tmp_filename(char *filename, int size);
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v4 5/7] block: Parse "backing" option to reference existing BDS

[Kevin Wolf]

Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c                   | 37 ++++++++++++++++++++++++++++++++-----
>  include/block/block_int.h |  3 +++
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3bf4c8a..a5da656 100644
> --- a/block.c
> +++ b/block.c
> @@ -1166,11 +1166,33 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>      /* If there is a backing file, use it */
>      if ((flags & BDRV_O_NO_BACKING) == 0) {
>          QDict *backing_options;
> -
> -        qdict_extract_subqdict(options, &backing_options, "backing.");
> -        ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> -        if (ret < 0) {
> -            goto close_and_fail;
> +        const char *backing_bs;
> +
> +        backing_bs = qdict_get_try_str(options, "backing");
> +        if (backing_bs) {
> +            bs->backing_hd = bdrv_find(backing_bs);
> +            qdict_del(options, "backing");
> +            if (bs->backing_hd) {
> +                bdrv_ref(bs->backing_hd);
> +                assert(!bs->backing_blocker);
> +                error_setg(&bs->backing_blocker,
> +                           "device is used as backing hd of '%s'",
> +                           bs->device_name);
> +                bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
> +                pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> +                        bs->backing_hd->filename);
> +                pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> +                        bs->backing_hd->drv->format_name);
> +            } else {
> +                error_setg(errp, "Backing device not found: %s", backing_bs);
> +                goto close_and_fail;
> +            }
> +        } else {
> +            qdict_extract_subqdict(options, &backing_options, "backing.");
> +            ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> +            if (ret < 0) {
> +                goto close_and_fail;
> +            }
>          }
>      }

Can we move all of this code into bdrv_open_backing_file()?

I think the bdrv_op_block_all() part should apply to all kinds of
backing files. Currently you can't issue any monitor commands, so it
doesn't matter yet, but Benoît is working on adding per-node names and
then we might want this.

And consistency never hurts anyway.

> @@ -1461,6 +1483,11 @@ void bdrv_close(BlockDriverState *bs)
>  
>      if (bs->drv) {
>          if (bs->backing_hd) {
> +            if (error_is_set(&bs->backing_blocker)) {

Then this one should become unconditional as well, I think.

> +                bdrv_op_unblock_all(bs->backing_hd,
> +                                    bs->backing_blocker);
> +                error_free(bs->backing_blocker);
> +            }
>              bdrv_unref(bs->backing_hd);
>              bs->backing_hd = NULL;
>          }

Kevin

[Qemu-devel] [PATCH v4 6/7] qmp: add command 'blockdev-backup'

[Fam Zheng]

Similar to drive-backup, but this command uses a device id as target
instead of creating/opening an image file.

Also add blocker on target bs, since the target is also a named device
now.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c   | 22 ++++++++++++++++++++++
 blockdev.c       | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 163 insertions(+)

diff --git a/block/backup.c b/block/backup.c
index cad14c9..b608f08 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -338,6 +338,7 @@ static void coroutine_fn backup_run(void *opaque)
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
+    bdrv_op_unblock_all(target, job->common.blocker);
     bdrv_unref(target);
 
     block_job_completed(&job->common, ret);
@@ -363,6 +364,24 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    if (!bdrv_is_inserted(bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bs->device_name);
+        return;
+    }
+
+    if (!bdrv_is_inserted(target)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, target->device_name);
+        return;
+    }
+
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP, errp)) {
+        return;
+    }
+
+    if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP, errp)) {
+        return;
+    }
+
     len = bdrv_getlength(bs);
     if (len < 0) {
         error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -376,6 +395,9 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    bdrv_op_block_all(target, job->common.blocker);
+    bdrv_op_unblock(target, BLOCK_OP_TYPE_NBD_SERVER_ADD, job->common.blocker);
+
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
     job->target = target;
diff --git a/blockdev.c b/blockdev.c
index 1921d27..e62fb35 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1900,6 +1900,8 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
+    /* Although backup_run has this check too, we need to use bs->drv below, so
+     * do an early check redundantly. */
     if (!bdrv_is_inserted(bs)) {
         error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return;
@@ -1974,6 +1976,50 @@ void qmp_drive_backup(const char *device, const char *target,
     }
 }
 
+void qmp_blockdev_backup(const char *device, const char *target,
+                         enum MirrorSyncMode sync,
+                         bool has_speed, int64_t speed,
+                         bool has_on_source_error,
+                         BlockdevOnError on_source_error,
+                         bool has_on_target_error,
+                         BlockdevOnError on_target_error,
+                         Error **errp)
+{
+    BlockDriverState *bs;
+    BlockDriverState *target_bs;
+    Error *local_err = NULL;
+
+    if (!has_speed) {
+        speed = 0;
+    }
+    if (!has_on_source_error) {
+        on_source_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
+    if (!has_on_target_error) {
+        on_target_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    target_bs = bdrv_find(target);
+    if (!target_bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, target);
+        return;
+    }
+
+    bdrv_ref(target_bs);
+    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+                 block_job_cb, bs, &local_err);
+    if (local_err != NULL) {
+        bdrv_unref(target_bs);
+        error_propagate(errp, local_err);
+    }
+}
+
 #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
 
 void qmp_drive_mirror(const char *device, const char *target,
diff --git a/qapi-schema.json b/qapi-schema.json
index 4656e8c..bd3c55c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1844,6 +1844,40 @@
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
+# @BlockdevBackup
+#
+# @device: the name of the device which should be copied.
+#
+# @target: the name of the backup target device
+#
+# @sync: what parts of the disk image should be copied to the destination
+#        (all the disk, only the sectors allocated in the topmost image, or
+#        only new I/O).
+#
+# @speed: #optional the maximum speed, in bytes per second
+#
+# @on-source-error: #optional the action to take on an error on the source,
+#                   default 'report'.  'stop' and 'enospc' can only be used
+#                   if the block device supports io-status (see BlockInfo).
+#
+# @on-target-error: #optional the action to take on an error on the target,
+#                   default 'report' (no limitations, since this applies to
+#                   a different block device than @device).
+#
+# Note that @on-source-error and @on-target-error only affect background I/O.
+# If an error occurs during a guest write request, the device's rerror/werror
+# actions will be used.
+#
+# Since: 1.7
+##
+{ 'type': 'BlockdevBackup',
+  'data': { 'device': 'str', 'target': 'str',
+            'sync': 'MirrorSyncMode',
+            '*speed': 'int',
+            '*on-source-error': 'BlockdevOnError',
+            '*on-target-error': 'BlockdevOnError' } }
+
+##
 # @Abort
 #
 # This action can be used to test transaction failure.
@@ -2033,6 +2067,21 @@
 { 'command': 'drive-backup', 'data': 'DriveBackup' }
 
 ##
+# @blockdev-backup
+#
+# Block device version for drive-backup command. Use existing device as target
+# of backup.
+#
+# For the arguments, see the documentation of BlockdevBackup.
+#
+# Returns: nothing on success
+#          If @device or @target is not a valid block device, DeviceNotFound
+#
+# Since 1.7
+##
+{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' }
+
+##
 # @drive-mirror
 #
 # Start mirroring a block device's writes to a new destination.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fba15cd..4116a8d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -963,6 +963,52 @@ Example:
                                                "sync": "full",
                                                "target": "backup.img" } }
 <- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "blockdev-backup",
+        .args_type  = "sync:s,device:B,target:B,speed:i?,"
+                      "on-source-error:s?,on-target-error:s?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_backup,
+    },
+
+SQMP
+blockdev-backup
+------------
+
+The device version of drive-backup: this command takes a existing named device
+as backup target.
+
+Arguments:
+
+- "device": the name of the device which should be copied.
+            (json-string)
+- "target": the target of the new image. If the file exists, or if it is a
+            device, the existing file/device will be used as the new
+            destination.  If it does not exist, a new file will be created.
+            (json-string)
+- "sync": what parts of the disk image should be copied to the destination;
+  possibilities include "full" for all the disk, "top" for only the sectors
+  allocated in the topmost image, or "none" to only replicate new I/O
+  (MirrorSyncMode).
+- "mode": whether and how QEMU should create a new image
+          (NewImageMode, optional, default 'absolute-paths')
+- "speed": the maximum speed, in bytes per second (json-int, optional)
+- "on-source-error": the action to take on an error on the source, default
+                     'report'.  'stop' and 'enospc' can only be used
+                     if the block device supports io-status.
+                     (BlockdevOnError, optional)
+- "on-target-error": the action to take on an error on the target, default
+                     'report' (no limitations, since this applies to
+                     a different block device than device).
+                     (BlockdevOnError, optional)
+
+Example:
+-> { "execute": "blockdev-backup", "arguments": { "device": "src-id",
+                                                  "target": "tgt-id" } }
+<- { "return": {} }
+
 EQMP
 
     {
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v4 6/7] qmp: add command 'blockdev-backup'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

On 11/21/2013 10:24 PM, Fam Zheng wrote:
> Similar to drive-backup, but this command uses a device id as target
> instead of creating/opening an image file.
> 
> Also add blocker on target bs, since the target is also a named device
> now.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---

> +++ b/qapi-schema.json
> @@ -1844,6 +1844,40 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> +# @BlockdevBackup
> +#
> +# @device: the name of the device which should be copied.
> +#
> +# @target: the name of the backup target device

We're not very consistent on whether to end these with a '.'; but that's
not the end of the world :)

> +# Note that @on-source-error and @on-target-error only affect background I/O.
> +# If an error occurs during a guest write request, the device's rerror/werror
> +# actions will be used.
> +#
> +# Since: 1.7

1.8

> +##
> +{ 'type': 'BlockdevBackup',
> +  'data': { 'device': 'str', 'target': 'str',
> +            'sync': 'MirrorSyncMode',
> +            '*speed': 'int',
> +            '*on-source-error': 'BlockdevOnError',
> +            '*on-target-error': 'BlockdevOnError' } }
> +

> +# @blockdev-backup
> +#
> +# Block device version for drive-backup command. Use existing device as target
> +# of backup.
> +#
> +# For the arguments, see the documentation of BlockdevBackup.
> +#
> +# Returns: nothing on success
> +#          If @device or @target is not a valid block device, DeviceNotFound
> +#
> +# Since 1.7

1.8

Interface seems okay; I didn't closely review the code leading up to it
(but do think your array of blockers, containing a ready-to-return error
message, is kind of slick)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v4 6/7] qmp: add command 'blockdev-backup'

[Kevin Wolf]

Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben:
> Similar to drive-backup, but this command uses a device id as target
> instead of creating/opening an image file.
> 
> Also add blocker on target bs, since the target is also a named device
> now.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index fba15cd..4116a8d 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -963,6 +963,52 @@ Example:
>                                                 "sync": "full",
>                                                 "target": "backup.img" } }
>  <- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name       = "blockdev-backup",
> +        .args_type  = "sync:s,device:B,target:B,speed:i?,"
> +                      "on-source-error:s?,on-target-error:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_blockdev_backup,
> +    },
> +
> +SQMP
> +blockdev-backup
> +------------
> +
> +The device version of drive-backup: this command takes a existing named device
> +as backup target.
> +
> +Arguments:
> +
> +- "device": the name of the device which should be copied.
> +            (json-string)
> +- "target": the target of the new image. If the file exists, or if it is a
> +            device, the existing file/device will be used as the new
> +            destination.  If it does not exist, a new file will be created.
> +            (json-string)
> +- "sync": what parts of the disk image should be copied to the destination;
> +  possibilities include "full" for all the disk, "top" for only the sectors
> +  allocated in the topmost image, or "none" to only replicate new I/O
> +  (MirrorSyncMode).

What happened to the indentation?

> +- "mode": whether and how QEMU should create a new image
> +          (NewImageMode, optional, default 'absolute-paths')

"mode" doesn't exist for blockdev-backup.

> +- "speed": the maximum speed, in bytes per second (json-int, optional)
> +- "on-source-error": the action to take on an error on the source, default
> +                     'report'.  'stop' and 'enospc' can only be used
> +                     if the block device supports io-status.
> +                     (BlockdevOnError, optional)
> +- "on-target-error": the action to take on an error on the target, default
> +                     'report' (no limitations, since this applies to
> +                     a different block device than device).
> +                     (BlockdevOnError, optional)
> +
> +Example:
> +-> { "execute": "blockdev-backup", "arguments": { "device": "src-id",
> +                                                  "target": "tgt-id" } }
> +<- { "return": {} }
> +
>  EQMP

Kevin

[Qemu-devel] [PATCH v4 7/7] block: Allow backup on referenced named BlockDriverState

[Fam Zheng]

Drive backup is a read only operation on source bs. We want to allow
this specific case to enable image-fleecing. Note that when
image-fleecing job starts, the job still add its blocker to source bs,
and any other operation on it will be blocked by that.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index a5da656..d30be51 100644
--- a/block.c
+++ b/block.c
@@ -1179,6 +1179,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
                            "device is used as backing hd of '%s'",
                            bs->device_name);
                 bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
+                bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_BACKUP,
+                                bs->backing_blocker);
                 pstrcpy(bs->backing_file, sizeof(bs->backing_file),
                         bs->backing_hd->filename);
                 pstrcpy(bs->backing_format, sizeof(bs->backing_format),
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v4 7/7] block: Allow backup on referenced named BlockDriverState

[Kevin Wolf]

Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben:
> Drive backup is a read only operation on source bs. We want to allow
> this specific case to enable image-fleecing. Note that when
> image-fleecing job starts, the job still add its blocker to source bs,
> and any other operation on it will be blocked by that.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block.c b/block.c
> index a5da656..d30be51 100644
> --- a/block.c
> +++ b/block.c
> @@ -1179,6 +1179,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>                             "device is used as backing hd of '%s'",
>                             bs->device_name);
>                  bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
> +                bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_BACKUP,
> +                                bs->backing_blocker);
>                  pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>                          bs->backing_hd->filename);
>                  pstrcpy(bs->backing_format, sizeof(bs->backing_format),

We probably need separate blockers for "can be a backup source" and "can
be a backup target". Because I think this allows using it as a
read-write target as well, which was not intended.

Do we need to cover this in other parts of the code as well, like when
adding a new BDS during external snapshot creation?

Kevin

Re: [Qemu-devel] [PATCH v4 7/7] block: Allow backup on referenced named BlockDriverState

[Fam Zheng]

On 2013年11月25日 19:23, Kevin Wolf wrote:
> Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben:
>> Drive backup is a read only operation on source bs. We want to allow
>> this specific case to enable image-fleecing. Note that when
>> image-fleecing job starts, the job still add its blocker to source bs,
>> and any other operation on it will be blocked by that.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>   block.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index a5da656..d30be51 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1179,6 +1179,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>>                              "device is used as backing hd of '%s'",
>>                              bs->device_name);
>>                   bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
>> +                bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_BACKUP,
>> +                                bs->backing_blocker);
>>                   pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>>                           bs->backing_hd->filename);
>>                   pstrcpy(bs->backing_format, sizeof(bs->backing_format),
>
> We probably need separate blockers for "can be a backup source" and "can
> be a backup target". Because I think this allows using it as a
> read-write target as well, which was not intended.
>

Yes. Will do it.

> Do we need to cover this in other parts of the code as well, like when
> adding a new BDS during external snapshot creation?
>

Does it have a name? If not I think we are safe there.

Fam

Re: [Qemu-devel] [PATCH v4 7/7] block: Allow backup on referenced named BlockDriverState

[Kevin Wolf]

Am 26.11.2013 um 04:06 hat Fam Zheng geschrieben:
> On 2013年11月25日 19:23, Kevin Wolf wrote:
> >Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben:
> >>Drive backup is a read only operation on source bs. We want to allow
> >>this specific case to enable image-fleecing. Note that when
> >>image-fleecing job starts, the job still add its blocker to source bs,
> >>and any other operation on it will be blocked by that.
> >>
> >>Signed-off-by: Fam Zheng <famz@redhat.com>
> >>---
> >>  block.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >>diff --git a/block.c b/block.c
> >>index a5da656..d30be51 100644
> >>--- a/block.c
> >>+++ b/block.c
> >>@@ -1179,6 +1179,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> >>                             "device is used as backing hd of '%s'",
> >>                             bs->device_name);
> >>                  bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
> >>+                bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_BACKUP,
> >>+                                bs->backing_blocker);
> >>                  pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> >>                          bs->backing_hd->filename);
> >>                  pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> >
> >We probably need separate blockers for "can be a backup source" and "can
> >be a backup target". Because I think this allows using it as a
> >read-write target as well, which was not intended.
> >
> 
> Yes. Will do it.
> 
> >Do we need to cover this in other parts of the code as well, like when
> >adding a new BDS during external snapshot creation?
> >
> 
> Does it have a name? If not I think we are safe there.

Not yet, but I think it won't be long until we do have named nodes
created this way, so considering it now certainly can't hurt.

Kevin

Re: [Qemu-devel] [PATCH v4 0/7] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD

[Stefan Hajnoczi]

On Fri, Nov 22, 2013 at 01:24:47PM +0800, Fam Zheng wrote:
> This series adds for point-in-time snapshot NBD exporting based on
> blockdev-backup (variant of drive-backup with existing device as target).
> 
> We get a thin point-in-time snapshot by COW mechanism of drive-backup, and
> export it through built in NBD server. The steps are as below:
> 
>  1. (SHELL) qemu-img create -f qcow2 BACKUP.qcow2 <source size here>
> 
>     (Alternatively we can use -o backing_file=RUNNING-VM.img to omit explicitly
>     providing the size by ourselves, but it's risky because RUNNING-VM.qcow2 is
>     used r/w by guest. Whether or not setting backing file in the image file
>     doesn't matter, as we are going to override the backing hd in the next
>     step)
> 
>  2. (QMP) blockdev-add backing=source-drive file.driver=file file.filename=BACKUP.qcow2 id=target0 if=none driver=qcow2
> 
>     (where ide0-hd0 is the running BlockDriverState name for
>     RUNNING-VM.img. This patch implements "backing=" option to override
>     backing_hd for added drive)
> 
>  3. (QMP) blockdev-backup device=source-drive sync=none target=target0
> 
>     (this is the QMP command introduced by this series, which use a named
>     device as target of drive-backup)
> 
>  4. (QMP) nbd-server-add device=target0
> 
> When image fleecing done:
> 
>  1. (QMP) block-job-complete device=ide0-hd0
> 
>  2. (HMP) drive_del target0
> 
>  3. (SHELL) rm BACKUP.qcow2

Interesting implementation, it looks pretty good.  I'll need to review
it a second time to track all the operation block/unblocks.  It wasn't
immediately clear to me whether these patches will restrict something
that used to work.

Stefan

Re: [Qemu-devel] [PATCH v4 0/7] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD

[Fam Zheng]

On Fri, 11/22 17:58, Stefan Hajnoczi wrote:
> On Fri, Nov 22, 2013 at 01:24:47PM +0800, Fam Zheng wrote:
> > This series adds for point-in-time snapshot NBD exporting based on
> > blockdev-backup (variant of drive-backup with existing device as target).
> > 
> > We get a thin point-in-time snapshot by COW mechanism of drive-backup, and
> > export it through built in NBD server. The steps are as below:
> > 
> >  1. (SHELL) qemu-img create -f qcow2 BACKUP.qcow2 <source size here>
> > 
> >     (Alternatively we can use -o backing_file=RUNNING-VM.img to omit
> >     explicitly providing the size by ourselves, but it's risky because
> >     RUNNING-VM.qcow2 is used r/w by guest. Whether or not setting backing
> >     file in the image file doesn't matter, as we are going to override the
> >     backing hd in the next step)
> > 
> >  2. (QMP) blockdev-add backing=source-drive file.driver=file
> >  file.filename=BACKUP.qcow2 id=target0 if=none driver=qcow2
> > 
> >     (where ide0-hd0 is the running BlockDriverState name for
> >     RUNNING-VM.img. This patch implements "backing=" option to override
> >     backing_hd for added drive)
> > 
> >  3. (QMP) blockdev-backup device=source-drive sync=none target=target0
> > 
> >     (this is the QMP command introduced by this series, which use a named
> >     device as target of drive-backup)
> > 
> >  4. (QMP) nbd-server-add device=target0
> > 
> > When image fleecing done:
> > 
> >  1. (QMP) block-job-complete device=ide0-hd0
> > 
> >  2. (HMP) drive_del target0
> > 
> >  3. (SHELL) rm BACKUP.qcow2
> 
> Interesting implementation, it looks pretty good.  I'll need to review it a
> second time to track all the operation block/unblocks.  It wasn't immediately
> clear to me whether these patches will restrict something that used to work.
> 

Good question, I asked myself too. :)

In some point in the middle of the series it should be theoretically the same
as before. But I did add some more blocker checks, E.g. NBD is blocked if
there's a block job, but starting nbd add doesn't add blocker.  So we can
nbd_server_add then start block job, but not in the other order. This is kind
of weird. I will also look back again.

I think another option is a compatibility matrix, to simplify the blocker
interface to bdrv_op_try_start(bs, op) + bdrv_op_end(bs, op): We get less
flexibility, but don't need dynanically allocated blocker from caller. The
advantage is that it's more centralized logic, so it's easy to manage.

Fam

Re: [Qemu-devel] [PATCH v4 0/7] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD

[Kevin Wolf]

Am 22.11.2013 um 17:58 hat Stefan Hajnoczi geschrieben:
> On Fri, Nov 22, 2013 at 01:24:47PM +0800, Fam Zheng wrote:
> > This series adds for point-in-time snapshot NBD exporting based on
> > blockdev-backup (variant of drive-backup with existing device as target).
> > 
> > We get a thin point-in-time snapshot by COW mechanism of drive-backup, and
> > export it through built in NBD server. The steps are as below:
> > 
> >  1. (SHELL) qemu-img create -f qcow2 BACKUP.qcow2 <source size here>
> > 
> >     (Alternatively we can use -o backing_file=RUNNING-VM.img to omit explicitly
> >     providing the size by ourselves, but it's risky because RUNNING-VM.qcow2 is
> >     used r/w by guest. Whether or not setting backing file in the image file
> >     doesn't matter, as we are going to override the backing hd in the next
> >     step)
> > 
> >  2. (QMP) blockdev-add backing=source-drive file.driver=file file.filename=BACKUP.qcow2 id=target0 if=none driver=qcow2
> > 
> >     (where ide0-hd0 is the running BlockDriverState name for
> >     RUNNING-VM.img. This patch implements "backing=" option to override
> >     backing_hd for added drive)
> > 
> >  3. (QMP) blockdev-backup device=source-drive sync=none target=target0
> > 
> >     (this is the QMP command introduced by this series, which use a named
> >     device as target of drive-backup)
> > 
> >  4. (QMP) nbd-server-add device=target0
> > 
> > When image fleecing done:
> > 
> >  1. (QMP) block-job-complete device=ide0-hd0
> > 
> >  2. (HMP) drive_del target0
> > 
> >  3. (SHELL) rm BACKUP.qcow2

I haven't looked at the code yet, but the interface looks right.

> Interesting implementation, it looks pretty good.  I'll need to review
> it a second time to track all the operation block/unblocks.  It wasn't
> immediately clear to me whether these patches will restrict something
> that used to work.

We still have a lot of time for 1.8, so I think we can be radical here:
If something doesn't have a test case and we don't catch it during the
normal review, then let's just break it. Someone will find it and then
we'll get a test case for it, so that it doesn't happen again.

Kevin

Re: [Qemu-devel] [PATCH v4 0/7] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD

[Stefan Hajnoczi]

On Mon, Nov 25, 2013 at 10:29:44AM +0100, Kevin Wolf wrote:
> Am 22.11.2013 um 17:58 hat Stefan Hajnoczi geschrieben:
> > On Fri, Nov 22, 2013 at 01:24:47PM +0800, Fam Zheng wrote:
> > Interesting implementation, it looks pretty good.  I'll need to review
> > it a second time to track all the operation block/unblocks.  It wasn't
> > immediately clear to me whether these patches will restrict something
> > that used to work.
> 
> We still have a lot of time for 1.8, so I think we can be radical here:
> If something doesn't have a test case and we don't catch it during the
> normal review, then let's just break it. Someone will find it and then
> we'll get a test case for it, so that it doesn't happen again.

Don't hold it up on my account, but let's still review properly.

Stefan