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, 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, 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 <eblake@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"
 _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"
 _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
-- 
2.20.1

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 <eblake@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

On 1/10/19 4:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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?

When libvirt does block migration, it opens up an NBD server on the
destination, does a block-mirror from the source to copy the contents to
the destination, then when the two are in sync does the actual VM state
migration. There may be a use case where the destination already has
some of the state (say that we started a migration, then got
interrupted, and now want to restart but not lose the progress of what
has already been sync'd previously) - in which case, the destination
would expose a dirty bitmap of what has been already transferred, and
the source can use that information to avoid re-sending data that has
not changed since the previous partial transfer.  There may be other
uses for exposing a live dirty bitmap for a writeable NBD export; and
it's more a question of not forbidding this case even if I don't have a
strong use case for why it will be useful.

> 
>   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)"

Nicer wording indeed. And, since my first paragraph was harder to
justify, I'll swap the two in order to emphasize the case I actually hit
over the one that is just hand-wavy "we might find a use for it".

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

>>   _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?
> 

> 
> 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

Could do.  I can split it if I have to respin.

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

10.01.2019 17:38, Eric Blake wrote:
> On 1/10/19 4:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 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?
> 
> When libvirt does block migration, it opens up an NBD server on the
> destination, does a block-mirror from the source to copy the contents to
> the destination, then when the two are in sync does the actual VM state
> migration. There may be a use case where the destination already has
> some of the state (say that we started a migration, then got
> interrupted, and now want to restart but not lose the progress of what
> has already been sync'd previously) - in which case, the destination
> would expose a dirty bitmap of what has been already transferred, and
> the source can use that information to avoid re-sending data that has
> not changed since the previous partial transfer.  There may be other
> uses for exposing a live dirty bitmap for a writeable NBD export; and
> it's more a question of not forbidding this case even if I don't have a
> strong use case for why it will be useful.
> 
>>
>>    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)"
> 
> Nicer wording indeed. And, since my first paragraph was harder to
> justify, I'll swap the two in order to emphasize the case I actually hit
> over the one that is just hand-wavy "we might find a use for it".
> 
>>
>> 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 <eblake@redhat.com>
>>>
> 
>>>    _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?
>>
> 
>>
>> 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
> 
> Could do.  I can split it if I have to respin.
> 

Ok.
with or without dropped unrelated error paths:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir