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 in
order to capture copy-on-write of old contents, and the
source in order 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 track enough state to actually enforce that).

However, consider the case of a bitmap contained in a read-only
node (including when the bitmap is found in a backing layer of
the active image).  Because the node can't be modified, the
bitmap won't change due to writes, regardless of whether it is
still enabled.  Forbidding the export unless the bitmap is
disabled is awkward, paritcularly since we can't change the
bitmap to be disabled (because the node is read-only).

Alternatively, consider the case of live storage migration,
where management directs the destination to create a writable
NBD server, then performs a drive-mirror from the source to
the mirror, prior to doing the rest of the live migration.
Since storage migration can be time-consuming, it may be wise
to let the destination include a dirty bitmap to track which
portions it has already received, where even if the migration
is interrupted and restarted, the source can query the
destination block status in order to minimize re-sending
data that has not changed in the meantime on a second attempt.
Such code has not been written, but it makes sense that
letting an active dirty bitmap be exposed and changing
alongside writes may prove useful in the future.

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
(whether or not it is the BDS handed to nbd_export_new() or
from its backing chain) is still writable.

Update iotest 223 to show the looser behavior by leaving
a bitmap enabled the whole run; note that we have to tear
down and re-export a node when handling an error.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v3: split out unrelated test changes, improve commit message [Vladimir]
v2: new patch
---
 nbd/server.c               |  7 +++++--
 tests/qemu-iotests/223     | 10 +++++++---
 tests/qemu-iotests/223.out |  3 ++-
 3 files changed, 14 insertions(+), 6 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 a4016091b21..f200e313c06 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
@@ -134,9 +136,11 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
 _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" # Attempt enabled bitmap
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
-  "arguments":{"node":"n", "name":"b2"}}' "return"
+  "arguments":{"name":"n2", "bitmap":"b2"}}' "error" # Enabled vs. read-only
+_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"

diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 8a4d63a4fc2..7d147291d48 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -35,7 +35,8 @@ wrote 2097152/2097152 bytes at offset 2097152
 {"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}}
 {"return": {}}
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "Bitmap 'b2' is enabled"}}
+{"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}}
+{"return": {}}
 {"return": {}}
 {"return": {}}

-- 
2.20.1

11.01.2019 22:47, 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 in
> order to capture copy-on-write of old contents, and the
> source in order 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 track enough state to actually enforce that).
> 
> However, consider the case of a bitmap contained in a read-only
> node (including when the bitmap is found in a backing layer of
> the active image).  Because the node can't be modified, the
> bitmap won't change due to writes, regardless of whether it is
> still enabled.  Forbidding the export unless the bitmap is
> disabled is awkward, paritcularly since we can't change the
> bitmap to be disabled (because the node is read-only).
> 
> Alternatively, consider the case of live storage migration,
> where management directs the destination to create a writable
> NBD server, then performs a drive-mirror from the source to
> the mirror, prior to doing the rest of the live migration.

s/mirror/target ?

> Since storage migration can be time-consuming, it may be wise
> to let the destination include a dirty bitmap to track which
> portions it has already received, where even if the migration
> is interrupted and restarted, the source can query the
> destination block status in order to minimize re-sending
> data that has not changed in the meantime on a second attempt.

hmm, dirty bitmap dirtiness don't guarantee that data was written,
due to:

1. philosofy: we generally don't care, when something is reported
    as dirty, when actually it is clean, we assume that it is always
    safe to consider things to be dirty.

2. granularity: really, at least granularity of target bitmap should
    correspond to mirror cluster size and target block size, if one of
    these three things is out of sync, we definitely get [1]

3. errors: bitmaps are set dirty for failed writes too, so [1]

I'm not totally against, I just note that the feature you mean is
at least not as simple to implement. And for me it looks like it is more
correct to track progress of mirror on source, assuming all success
nbd write requests as successful story. At least, as all vm metadata
is on source, when storage migration not finished yet.

> Such code has not been written, but it makes sense that
> letting an active dirty bitmap be exposed and changing
> alongside writes may prove useful in the future.
> 
> 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
> (whether or not it is the BDS handed to nbd_export_new() or
> from its backing chain) is still writable.
> 
> Update iotest 223 to show the looser behavior by leaving
> a bitmap enabled the whole run; note that we have to tear
> down and re-export a node when handling an error.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

anyway, I'm OK with new if-condition, and for me something like

   No real reasons to forbid export of enabled bitmap, user should understand,
   that block-status result may not correspond to reality if bitmap may be
   changed by guest-write or something in parallel (including client's self requests).
   So, it should be OK to drop the restriction at all, but let's keep a smaller
   one to prevent dangerous wrong management error..

would be enough description so,
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Or, we can allow exporting enabled bitmaps only for read-only nodes, until we
don't have real reasoning for your second case.

> 
> ---
> v3: split out unrelated test changes, improve commit message [Vladimir]
> v2: new patch
> ---
>   nbd/server.c               |  7 +++++--
>   tests/qemu-iotests/223     | 10 +++++++---
>   tests/qemu-iotests/223.out |  3 ++-
>   3 files changed, 14 insertions(+), 6 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 a4016091b21..f200e313c06 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
> @@ -134,9 +136,11 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
>   _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" # Attempt enabled bitmap
> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
> -  "arguments":{"node":"n", "name":"b2"}}' "return"
> +  "arguments":{"name":"n2", "bitmap":"b2"}}' "error" # Enabled vs. read-only
> +_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"
> 
> diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
> index 8a4d63a4fc2..7d147291d48 100644
> --- a/tests/qemu-iotests/223.out
> +++ b/tests/qemu-iotests/223.out
> @@ -35,7 +35,8 @@ wrote 2097152/2097152 bytes at offset 2097152
>   {"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}}
>   {"return": {}}
>   {"return": {}}
> -{"error": {"class": "GenericError", "desc": "Bitmap 'b2' is enabled"}}
> +{"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}}
> +{"return": {}}
>   {"return": {}}
>   {"return": {}}
> 


-- 
Best regards,
Vladimir

On 1/14/19 3:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.01.2019 22:47, 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 in
>> order to capture copy-on-write of old contents, and the
>> source in order 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 track enough state to actually enforce that).
>>
>> However, consider the case of a bitmap contained in a read-only
>> node (including when the bitmap is found in a backing layer of
>> the active image).  Because the node can't be modified, the
>> bitmap won't change due to writes, regardless of whether it is
>> still enabled.  Forbidding the export unless the bitmap is
>> disabled is awkward, paritcularly since we can't change the
>> bitmap to be disabled (because the node is read-only).
>>
>> Alternatively, consider the case of live storage migration,
>> where management directs the destination to create a writable
>> NBD server, then performs a drive-mirror from the source to
>> the mirror, prior to doing the rest of the live migration.
> 
> s/mirror/target ?

Yes.

> 
>> Since storage migration can be time-consuming, it may be wise
>> to let the destination include a dirty bitmap to track which
>> portions it has already received, where even if the migration
>> is interrupted and restarted, the source can query the
>> destination block status in order to minimize re-sending
>> data that has not changed in the meantime on a second attempt.
> 
> hmm, dirty bitmap dirtiness don't guarantee that data was written,
> due to:
> 
> 1. philosofy: we generally don't care, when something is reported
>     as dirty, when actually it is clean, we assume that it is always
>     safe to consider things to be dirty.
> 
> 2. granularity: really, at least granularity of target bitmap should
>     correspond to mirror cluster size and target block size, if one of
>     these three things is out of sync, we definitely get [1]
> 
> 3. errors: bitmaps are set dirty for failed writes too, so [1]

Good points.

> 
> I'm not totally against, I just note that the feature you mean is
> at least not as simple to implement. And for me it looks like it is more
> correct to track progress of mirror on source, assuming all success
> nbd write requests as successful story. At least, as all vm metadata
> is on source, when storage migration not finished yet.

I'm fine adding a caveat that while allowing reads from a modifying
dirty bitmap may help, they may not be trivial to use correctly.

> 
>> Such code has not been written, but it makes sense that
>> letting an active dirty bitmap be exposed and changing
>> alongside writes may prove useful in the future.

to go along with my caveat that such code does not yet exist anyways :)

>>
>> 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
>> (whether or not it is the BDS handed to nbd_export_new() or
>> from its backing chain) is still writable.
>>
>> Update iotest 223 to show the looser behavior by leaving
>> a bitmap enabled the whole run; note that we have to tear
>> down and re-export a node when handling an error.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> anyway, I'm OK with new if-condition, and for me something like
> 
>    No real reasons to forbid export of enabled bitmap, user should understand,
>    that block-status result may not correspond to reality if bitmap may be
>    changed by guest-write or something in parallel (including client's self requests).
>    So, it should be OK to drop the restriction at all, but let's keep a smaller
>    one to prevent dangerous wrong management error..
> 
> would be enough description so,
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Okay. Thanks for the review!

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org