Re: [Qemu-devel] [PATCH v2 3/6] nbd: Allow bitmap export during QMP nbd-server-add

[Vladimir Sementsov-Ogievskiy]

10.01.2019 10:13, Eric Blake wrote:
> With the experimental x-nbd-server-add-bitmap command, there was
> a window of time where an NBD client could see the export but not
> the associated dirty bitmap, which can cause a client that planned
> on using the dirty bitmap to be forced to treat the entire image
> as dirty as a safety fallback.  Furthermore, if the QMP client
> successfully exports a disk but then fails to add the bitmap, it
> has to take on the burden of removing the export.  Since we don't
> allow changing the exposed dirty bitmap (whether to a different
> bitmap, or removing advertisement of the bitmap), it is nicer to
> make the bitmap tied to the export at the time the export is
> created, with automatic failure to export if the bitmap is not
> available.
> 
> The experimental command included an optional 'bitmap-export-name'
> field for remapping the name exposed over NBD to be different from
> the bitmap name stored on disk.  However, my libvirt demo code
> for implementing differential backups on top of persistent bitmaps
> did not need to take advantage of that feature (it is instead
> possible to create a new temporary bitmap with the desired name,
> use block-dirty-bitmap-merge to merge one or more persistent
> bitmaps into the temporary, then associate the temporary with the
> NBD export, if control is needed over the exported bitmap name).
> Hence, I'm not copying that part of the experiment over to the
> stable addition. For more details on the libvirt demo, see
> https://www.redhat.com/archives/libvir-list/2018-October/msg01254.html,
> https://kvmforum2018.sched.com/event/FzuB/facilitating-incremental-backup-eric-blake-red-hat
> 
> This patch focuses on the user interface, and reduces (but does
> not completely eliminate) the window where an NBD client can see
> the export but not the dirty bitmap.  Later patches will add
> further cleanups now that this interface is declared stable via a
> single QMP command, including removing the race window.
> 
> Update test 223 to use the new interface, including a demonstration
> that it is now easier to handle failures.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   qapi/block.json            |  7 ++++++-
>   blockdev-nbd.c             | 12 +++++++++++-
>   hmp.c                      |  5 +++--
>   tests/qemu-iotests/223     | 17 ++++++-----------
>   tests/qemu-iotests/223.out |  5 +----
>   5 files changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index 11f01f28efe..3d70420f763 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -246,6 +246,10 @@
>   #
>   # @writable: Whether clients should be able to write to the device via the
>   #     NBD connection (default false).
> +
> +# @bitmap: Also export the dirty bitmap reachable from @device, so the
> +#          NBD client can use NBD_OPT_SET_META_CONTEXT with
> +#          "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
>   #
>   # Returns: error if the server is not running, or export with the same name
>   #          already exists.
> @@ -253,7 +257,8 @@
>   # Since: 1.3.0
>   ##
>   { 'command': 'nbd-server-add',
> -  'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
> +  'data': {'device': 'str', '*name': 'str', '*writable': 'bool',
> +           '*bitmap': 'str' } }
> 
>   ##
>   # @NbdServerRemoveMode:
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index f5edbc27d88..ac7e993c35f 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -140,7 +140,8 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
>   }
> 
>   void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
> -                        bool has_writable, bool writable, Error **errp)
> +                        bool has_writable, bool writable,
> +                        bool has_bitmap, const char *bitmap, Error **errp)
>   {
>       BlockDriverState *bs = NULL;
>       BlockBackend *on_eject_blk;
> @@ -185,6 +186,15 @@ void qmp_nbd_server_add(const char *device, bool 
> has_name, const char *name,
>        * our only way of accessing it is through nbd_export_find(), so we can 
> drop
>        * the strong reference that is @exp. */
>       nbd_export_put(exp);
> +
> +    if (has_bitmap) {
> +        Error *err = NULL;

Hm, I though that local_err is most popular name for it, check:

# git grep 'error_propagate(errp, err)' | wc -l
352
# git grep 'error_propagate(errp, local_err)' | wc -l
540

- hm, surprise for me, err is very popular too.. but not in block layer:

# git grep 'error_propagate(errp, err)' -- block* | wc -l
6
# git grep 'error_propagate(errp, local_err)' -- block* | wc -l
179
# git grep 'error_propagate(errp, err)' -- *nbd* block/nbd* | wc -l
0
# git grep 'error_propagate(errp, local_err)' -- *nbd* block/nbd* | wc -l
3

sorry for that very-very nitpicking, with or without s/err/local_err/:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


> +        nbd_export_bitmap(exp, bitmap, bitmap, &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            nbd_export_remove(exp, NBD_SERVER_REMOVE_MODE_HARD, NULL);
> +        }
> +    }
>   }
> 
>   void qmp_nbd_server_remove(const char *name,
> diff --git a/hmp.c b/hmp.c
> index 80aa5ab504b..8da5fd8760a 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2326,7 +2326,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict 
> *qdict)
>           }
> 
>           qmp_nbd_server_add(info->value->device, false, NULL,
> -                           true, writable, &local_err);
> +                           true, writable, false, NULL, &local_err);
> 
>           if (local_err != NULL) {
>               qmp_nbd_server_stop(NULL);
> @@ -2347,7 +2347,8 @@ void hmp_nbd_server_add(Monitor *mon, const QDict 
> *qdict)
>       bool writable = qdict_get_try_bool(qdict, "writable", false);
>       Error *local_err = NULL;
> 
> -    qmp_nbd_server_add(device, !!name, name, true, writable, &local_err);
> +    qmp_nbd_server_add(device, !!name, name, true, writable,
> +                       false, NULL, &local_err);
>       hmp_handle_error(mon, &local_err);
>   }
> 
> diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
> index f1fbb9bc1c6..1f6822f9489 100755
> --- a/tests/qemu-iotests/223
> +++ b/tests/qemu-iotests/223
> @@ -120,21 +120,16 @@ _send_qemu_cmd $QEMU_HANDLE 
> '{"execute":"nbd-server-start",
>     "arguments":{"addr":{"type":"unix",
>       "data":{"path":"'"$TEST_DIR/nbd"'"}}}}' "return"
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
> -  "arguments":{"device":"n"}}' "return"
> +  "arguments":{"device":"n", "bitmap":"b"}}' "return"
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
>     "arguments":{"device":"n"}}' "error"
> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
> -  "arguments":{"name":"n", "bitmap":"b"}}' "return"
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
> -  "arguments":{"device":"n", "name":"n2"}}' "return"
> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
> -  "arguments":{"name":"n2", "bitmap":"b2"}}' "error"
> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
> -  "arguments":{"name":"n2"}}' "return"

Aha, so, you've added this recently for this demonstration, it makes sense.

> +  "arguments":{"device":"n", "name":"n2", "bitmap":"b2"}}' "error"
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
> -  "arguments":{"device":"n", "name":"n2", "writable":true}}' "return"
> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
> -  "arguments":{"name":"n2", "bitmap":"b2"}}' "return"
> +  "arguments":{"device":"n", "name":"n2", "bitmap":"b3"}}' "error"

new error path, it demonstrates error handling with new interface too..

> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
> +  "arguments":{"device":"n", "name":"n2", "writable":true,
> +  "bitmap":"b2"}}' "return"
> 
>   echo
>   echo "=== Contrast normal status to large granularity dirty-bitmap ==="
> diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
> index 5ed2e322e19..7135bf59bb8 100644
> --- a/tests/qemu-iotests/223.out
> +++ b/tests/qemu-iotests/223.out
> @@ -30,11 +30,8 @@ wrote 2097152/2097152 bytes at offset 2097152
>   {"return": {}}
>   {"return": {}}
>   {"error": {"class": "GenericError", "desc": "NBD server already has export 
> named 'n'"}}
> -{"return": {}}
> -{"return": {}}
>   {"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' 
> incompatible with readonly export"}}
> -{"return": {}}
> -{"return": {}}
> +{"error": {"class": "GenericError", "desc": "Bitmap 'b3' is not found"}}
>   {"return": {}}
> 
>   === Contrast normal status to large granularity dirty-bitmap ===
> 


-- 
Best regards,
Vladimir