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 <ebl...@redhat.com>
>
> ---
> 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
