Re: [Qemu-devel] [PATCH v2 1/6] nbd: Only require disabled bitmap for read-only exports

[Vladimir Sementsov-Ogievskiy]

10.01.2019 10:13, Eric Blake wrote:
> Our initial implementation of x-nbd-server-add-bitmap put
> in a restriction because of incremental backups: in that
> usage, we are exporting one qcow2 file (the temporary overlay
> target of a blockdev-backup sync:none job) and a dirty bitmap
> owned by a second qcow2 file (the source of the
> blockdev-backup, which is the backing file of the temporary).
> While both qcow2 files are still writable (the target capture
> copy-on-write of old contents, the source to track live guest
> writes in the meantime), the NBD client expects to see
> constant data, including the dirty bitmap.  An enabled bitmap
> in the source would be modified by guest writes, which is at
> odds with the NBD export being a read-only constant view,
> hence the initial code choice of enforcing a disabled bitmap
> (the intent is that the exposed bitmap was disabled in the
> same transaction that started the blockdev-backup job,
> although we don't want to actually track enough state to
> actually enforce that).
> 
> However, in the case of image migration, where we WANT a
> writable export, it makes total sense that the NBD client
> could expect writes to change the status visible through
> the exposed dirty bitmap,

Don't follow.. Which kind of migration do you mean?

  and thus permitting an enabled
> bitmap for an export that is not read-only makes sense;
> although the current restriction prevents that usage.
> 
> Alternatively, consider the case when the active layer does
> not contain the bitmap but the backing layer does.  If the
> backing layer is read-only (which is different from the
> backing layer of a blockdev-backup job being writable),
> we know the backing layer cannot be modified, 

hmm, sounds a bit strange "in case of read-only backing, we know
that it cannot be modified". It's true for any read-only node,
so you can say just something like "read-only node (for example
regular backing file)"

and thus the
> bitmap will not change contents even if it is still marked
> enabled (for that matter, since the backing file is
> read-only, we couldn't change the bitmap to be disabled
> in the first place).  Again, the current restriction
> prevents this usage.
> 
> Solve both issues by gating the restriction against a
> disabled bitmap to only happen when the caller has
> requested a read-only export, and where the BDS that owns
> the bitmap (which might be a backing file of the BDS
> handed to nbd_export_new) is still writable.
> 
> Update iotest 223 to add some tests of the error paths.
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v2: new patch
> ---
>   nbd/server.c               |  7 +++++--
>   tests/qemu-iotests/223     | 14 ++++++++++++--
>   tests/qemu-iotests/223.out |  4 ++++
>   3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 7af0ddffb20..98327088cb4 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2456,8 +2456,11 @@ void nbd_export_bitmap(NBDExport *exp, const char 
> *bitmap,
>           return;
>       }
> 
> -    if (bdrv_dirty_bitmap_enabled(bm)) {
> -        error_setg(errp, "Bitmap '%s' is enabled", bitmap);
> +    if ((exp->nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) &&
> +        bdrv_dirty_bitmap_enabled(bm)) {
> +        error_setg(errp,
> +                   "Enabled bitmap '%s' incompatible with readonly export",
> +                   bitmap);
>           return;
>       }
> 
> diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
> index 5513dc62159..f1fbb9bc1c6 100755
> --- a/tests/qemu-iotests/223
> +++ b/tests/qemu-iotests/223
> @@ -61,6 +61,8 @@ echo "=== Create partially sparse image, then add dirty 
> bitmaps ==="
>   echo
> 
>   # Two bitmaps, to contrast granularity issues
> +# Also note that b will be disabled, while b2 is left enabled, to
> +# check for read-only interactions
>   _make_test_img -o cluster_size=4k 4M
>   $QEMU_IO -c 'w -P 0x11 1M 2M' "$TEST_IMG" | _filter_qemu_io
>   run_qemu <<EOF
> @@ -114,17 +116,23 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
>       "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
>     "arguments":{"node":"n", "name":"b"}}' "return"
> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
> -  "arguments":{"node":"n", "name":"b2"}}' "return"
>   _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"
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
> +  "arguments":{"device":"n"}}' "error"

twice add?

>   _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"
> +_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"
> 
> @@ -157,6 +165,8 @@ _send_qemu_cmd $QEMU_HANDLE 
> '{"execute":"nbd-server-remove",
>     "arguments":{"name":"n"}}' "return"
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
>     "arguments":{"name":"n2"}}' "return"
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
> +  "arguments":{"name":"n2"}}' "error"]

and here?

aha, I've just realized that it's "some tests of the error paths" not related 
to bitmaps..

So, I'd prefer to keep them in separate patch

>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return"
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return"
> 
> diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
> index 99ca172fbb8..5ed2e322e19 100644
> --- a/tests/qemu-iotests/223.out
> +++ b/tests/qemu-iotests/223.out
> @@ -29,8 +29,11 @@ wrote 2097152/2097152 bytes at offset 2097152
>   {"return": {}}
>   {"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": {}}
>   {"return": {}}
> 
> @@ -62,6 +65,7 @@ read 2097152/2097152 bytes at offset 2097152
> 
>   {"return": {}}
>   {"return": {}}
> +{"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
>   {"return": {}}
>   {"return": {}}
>   *** done
> 

New restriction looks OK for me.


-- 
Best regards,
Vladimir