[Qemu-devel] [PATCH v2 0/6] Promote x-nbd-server-add-bitmap to stable

[Qemu-devel] [PATCH v2 0/6] Promote x-nbd-server-add-bitmap to stable

[Eric Blake]

Or rather, move its functionality into nbd-server-add.  And as
a side effect, teach qemu-nbd how to export a persistent bitmap
without having to go through a qemu process and several QMP
commands.

Based-on: <20181221093529.23855-1-jsnow@redhat.com>
[0/11 bitmaps: remove x- prefix from QMP api]

Available at: https://repo.or.cz/qemu/ericb.git nbd-bitmap-add-v2

Since v1:
- add new patch 1 allowing bitmaps with writable exports
- add coverage in iotest 223
- fix logic bug that rendered qemu-nbd -B useless
- drop support for bitmap-export-name remapping

001/6:[down] 'nbd: Only require disabled bitmap for read-only exports'
002/6:[----] [--] 'nbd: Merge nbd_export_set_name into nbd_export_new'
003/6:[0056] [FC] 'nbd: Allow bitmap export during QMP nbd-server-add'
004/6:[----] [--] 'nbd: Remove x-nbd-server-add-bitmap'
005/6:[0042] [FC] 'nbd: Merge nbd_export_bitmap into nbd_export_new'
006/6:[0041] [FC] 'qemu-nbd: Add --bitmap=NAME option'

Eric Blake (6):
  nbd: Only require disabled bitmap for read-only exports
  nbd: Merge nbd_export_set_name into nbd_export_new
  nbd: Allow bitmap export during QMP nbd-server-add
  nbd: Remove x-nbd-server-add-bitmap
  nbd: Merge nbd_export_bitmap into nbd_export_new
  qemu-nbd: Add --bitmap=NAME option

 qemu-nbd.texi              |   4 ++
 qapi/block.json            |  30 ++-------
 include/block/nbd.h        |  12 ++--
 blockdev-nbd.c             |  31 ++-------
 hmp.c                      |   5 +-
 nbd/server.c               | 129 ++++++++++++++++---------------------
 qemu-nbd.c                 |  15 +++--
 tests/qemu-iotests/223     |  39 ++++++++---
 tests/qemu-iotests/223.out |  19 ++++--
 9 files changed, 132 insertions(+), 152 deletions(-)

-- 
2.20.1

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

[Eric Blake]

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

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4467 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

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

[Qemu-devel] [PATCH v2 2/6] nbd: Merge nbd_export_set_name into nbd_export_new

[Eric Blake]

The existing NBD code had a weird split where nbd_export_new()
created an export but did not add it to the list of exported
names until a later nbd_export_set_name() came along and grabbed
a second reference on the object; later, nbd_export_close()
drops the second reference.  But since we never change the
name of an NBD export while it is exposed, it is easier to just
inline the process of setting the name as part of creating the
export.

Inline the contents of nbd_export_set_name() and
nbd_export_set_description() into the two points in an export
lifecycle where they matter, then adjust both callers to pass
the name up front.  Note that all callers pass a non-NULL name,
(passing NULL at creation was for old style servers, but we
removed support for that in commit 7f7dfe2a).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  3 +--
 blockdev-nbd.c      |  5 ++---
 nbd/server.c        | 45 ++++++++++++++++-----------------------------
 qemu-nbd.c          |  5 ++---
 4 files changed, 21 insertions(+), 37 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 65402d33964..2f9a2aeb73c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -295,6 +295,7 @@ typedef struct NBDExport NBDExport;
 typedef struct NBDClient NBDClient;

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
+                          const char *name, const char *description,
                           uint16_t nbdflags, void (*close)(NBDExport *),
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp);
@@ -306,8 +307,6 @@ void nbd_export_put(NBDExport *exp);
 BlockBackend *nbd_export_get_blockdev(NBDExport *exp);

 NBDExport *nbd_export_find(const char *name);
-void nbd_export_set_name(NBDExport *exp, const char *name);
-void nbd_export_set_description(NBDExport *exp, const char *description);
 void nbd_export_close_all(void);

 void nbd_client_new(QIOChannelSocket *sioc,
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 1d170c80b82..f5edbc27d88 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -174,14 +174,13 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         writable = false;
     }

-    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
+    exp = nbd_export_new(bs, 0, -1, name, NULL,
+                         writable ? 0 : NBD_FLAG_READ_ONLY,
                          NULL, false, on_eject_blk, errp);
     if (!exp) {
         return;
     }

-    nbd_export_set_name(exp, name);
-
     /* The list of named exports has a strong reference to this export now and
      * our only way of accessing it is through nbd_export_find(), so we can drop
      * the strong reference that is @exp. */
diff --git a/nbd/server.c b/nbd/server.c
index 98327088cb4..676fb4886d0 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1456,6 +1456,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
 }

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
+                          const char *name, const char *description,
                           uint16_t nbdflags, void (*close)(NBDExport *),
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp)
@@ -1471,6 +1472,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
      * that BDRV_O_INACTIVE is cleared and the image is ready for write
      * access since the export could be available before migration handover.
      */
+    assert(name);
     ctx = bdrv_get_aio_context(bs);
     aio_context_acquire(ctx);
     bdrv_invalidate_cache(bs, NULL);
@@ -1494,6 +1496,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
     QTAILQ_INIT(&exp->clients);
     exp->blk = blk;
     exp->dev_offset = dev_offset;
+    exp->name = g_strdup(name);
+    exp->description = g_strdup(description);
     exp->nbdflags = nbdflags;
     exp->size = size < 0 ? blk_getlength(blk) : size;
     if (exp->size < 0) {
@@ -1513,10 +1517,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
         exp->eject_notifier.notify = nbd_eject_notifier;
         blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier);
     }
+    QTAILQ_INSERT_TAIL(&exports, exp, next);
+    nbd_export_get(exp);
     return exp;

 fail:
     blk_unref(blk);
+    g_free(exp->name);
+    g_free(exp->description);
     g_free(exp);
     return NULL;
 }
@@ -1533,33 +1541,6 @@ NBDExport *nbd_export_find(const char *name)
     return NULL;
 }

-void nbd_export_set_name(NBDExport *exp, const char *name)
-{
-    if (exp->name == name) {
-        return;
-    }
-
-    nbd_export_get(exp);
-    if (exp->name != NULL) {
-        g_free(exp->name);
-        exp->name = NULL;
-        QTAILQ_REMOVE(&exports, exp, next);
-        nbd_export_put(exp);
-    }
-    if (name != NULL) {
-        nbd_export_get(exp);
-        exp->name = g_strdup(name);
-        QTAILQ_INSERT_TAIL(&exports, exp, next);
-    }
-    nbd_export_put(exp);
-}
-
-void nbd_export_set_description(NBDExport *exp, const char *description)
-{
-    g_free(exp->description);
-    exp->description = g_strdup(description);
-}
-
 void nbd_export_close(NBDExport *exp)
 {
     NBDClient *client, *next;
@@ -1568,8 +1549,14 @@ void nbd_export_close(NBDExport *exp)
     QTAILQ_FOREACH_SAFE(client, &exp->clients, next, next) {
         client_close(client, true);
     }
-    nbd_export_set_name(exp, NULL);
-    nbd_export_set_description(exp, NULL);
+    if (exp->name) {
+        nbd_export_put(exp);
+        g_free(exp->name);
+        exp->name = NULL;
+        QTAILQ_REMOVE(&exports, exp, next);
+    }
+    g_free(exp->description);
+    exp->description = NULL;
     nbd_export_put(exp);
 }

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 2807e132396..696bd78a2e2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1015,10 +1015,9 @@ int main(int argc, char **argv)
         }
     }

-    exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, nbd_export_closed,
+    exp = nbd_export_new(bs, dev_offset, fd_size, export_name,
+                         export_description, nbdflags, nbd_export_closed,
                          writethrough, NULL, &error_fatal);
-    nbd_export_set_name(exp, export_name);
-    nbd_export_set_description(exp, export_description);

     if (device) {
 #if HAVE_NBD_DEVICE
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 2/6] nbd: Merge nbd_export_set_name into nbd_export_new

[Vladimir Sementsov-Ogievskiy]

10.01.2019 10:13, Eric Blake wrote:
> The existing NBD code had a weird split where nbd_export_new()
> created an export but did not add it to the list of exported
> names until a later nbd_export_set_name() came along and grabbed
> a second reference on the object; later, nbd_export_close()
> drops the second reference.  But since we never change the
> name of an NBD export while it is exposed, it is easier to just
> inline the process of setting the name as part of creating the
> export.
> 
> Inline the contents of nbd_export_set_name() and
> nbd_export_set_description() into the two points in an export
> lifecycle where they matter, then adjust both callers to pass
> the name up front.  Note that all callers pass a non-NULL name,
> (passing NULL at creation was for old style servers, but we
> removed support for that in commit 7f7dfe2a).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   include/block/nbd.h |  3 +--
>   blockdev-nbd.c      |  5 ++---
>   nbd/server.c        | 45 ++++++++++++++++-----------------------------
>   qemu-nbd.c          |  5 ++---
>   4 files changed, 21 insertions(+), 37 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 65402d33964..2f9a2aeb73c 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -295,6 +295,7 @@ typedef struct NBDExport NBDExport;
>   typedef struct NBDClient NBDClient;
> 
>   NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
> +                          const char *name, const char *description,
>                             uint16_t nbdflags, void (*close)(NBDExport *),
>                             bool writethrough, BlockBackend *on_eject_blk,
>                             Error **errp);
> @@ -306,8 +307,6 @@ void nbd_export_put(NBDExport *exp);
>   BlockBackend *nbd_export_get_blockdev(NBDExport *exp);
> 
>   NBDExport *nbd_export_find(const char *name);
> -void nbd_export_set_name(NBDExport *exp, const char *name);
> -void nbd_export_set_description(NBDExport *exp, const char *description);
>   void nbd_export_close_all(void);
> 
>   void nbd_client_new(QIOChannelSocket *sioc,
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 1d170c80b82..f5edbc27d88 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -174,14 +174,13 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>           writable = false;
>       }
> 
> -    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
> +    exp = nbd_export_new(bs, 0, -1, name, NULL,
> +                         writable ? 0 : NBD_FLAG_READ_ONLY,
>                            NULL, false, on_eject_blk, errp);
>       if (!exp) {
>           return;
>       }
> 
> -    nbd_export_set_name(exp, name);
> -
>       /* The list of named exports has a strong reference to this export now and
>        * our only way of accessing it is through nbd_export_find(), so we can drop
>        * the strong reference that is @exp. */
> diff --git a/nbd/server.c b/nbd/server.c
> index 98327088cb4..676fb4886d0 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1456,6 +1456,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
>   }
> 
>   NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
> +                          const char *name, const char *description,
>                             uint16_t nbdflags, void (*close)(NBDExport *),
>                             bool writethrough, BlockBackend *on_eject_blk,
>                             Error **errp)
> @@ -1471,6 +1472,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
>        * that BDRV_O_INACTIVE is cleared and the image is ready for write
>        * access since the export could be available before migration handover.
>        */
> +    assert(name);
>       ctx = bdrv_get_aio_context(bs);
>       aio_context_acquire(ctx);
>       bdrv_invalidate_cache(bs, NULL);
> @@ -1494,6 +1496,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
>       QTAILQ_INIT(&exp->clients);
>       exp->blk = blk;
>       exp->dev_offset = dev_offset;
> +    exp->name = g_strdup(name);
> +    exp->description = g_strdup(description);
>       exp->nbdflags = nbdflags;
>       exp->size = size < 0 ? blk_getlength(blk) : size;
>       if (exp->size < 0) {
> @@ -1513,10 +1517,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
>           exp->eject_notifier.notify = nbd_eject_notifier;
>           blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier);
>       }
> +    QTAILQ_INSERT_TAIL(&exports, exp, next);
> +    nbd_export_get(exp);
>       return exp;
> 
>   fail:
>       blk_unref(blk);
> +    g_free(exp->name);
> +    g_free(exp->description);
>       g_free(exp);
>       return NULL;
>   }
> @@ -1533,33 +1541,6 @@ NBDExport *nbd_export_find(const char *name)
>       return NULL;
>   }
> 
> -void nbd_export_set_name(NBDExport *exp, const char *name)
> -{
> -    if (exp->name == name) {
> -        return;
> -    }
> -
> -    nbd_export_get(exp);
> -    if (exp->name != NULL) {
> -        g_free(exp->name);
> -        exp->name = NULL;
> -        QTAILQ_REMOVE(&exports, exp, next);
> -        nbd_export_put(exp);
> -    }
> -    if (name != NULL) {
> -        nbd_export_get(exp);
> -        exp->name = g_strdup(name);
> -        QTAILQ_INSERT_TAIL(&exports, exp, next);
> -    }
> -    nbd_export_put(exp);
> -}
> -
> -void nbd_export_set_description(NBDExport *exp, const char *description)
> -{
> -    g_free(exp->description);
> -    exp->description = g_strdup(description);
> -}
> -
>   void nbd_export_close(NBDExport *exp)
>   {
>       NBDClient *client, *next;
> @@ -1568,8 +1549,14 @@ void nbd_export_close(NBDExport *exp)
>       QTAILQ_FOREACH_SAFE(client, &exp->clients, next, next) {
>           client_close(client, true);
>       }
> -    nbd_export_set_name(exp, NULL);
> -    nbd_export_set_description(exp, NULL);
> +    if (exp->name) {
> +        nbd_export_put(exp);
> +        g_free(exp->name);
> +        exp->name = NULL;
> +        QTAILQ_REMOVE(&exports, exp, next);

exp->name is always non-zero, and _get and _INSERT_TAIL were done independently from name,
so with these four lines done unconditionally:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> +    }
> +    g_free(exp->description);
> +    exp->description = NULL;
>       nbd_export_put(exp);
>   }
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 2807e132396..696bd78a2e2 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -1015,10 +1015,9 @@ int main(int argc, char **argv)
>           }
>       }
> 
> -    exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, nbd_export_closed,
> +    exp = nbd_export_new(bs, dev_offset, fd_size, export_name,
> +                         export_description, nbdflags, nbd_export_closed,
>                            writethrough, NULL, &error_fatal);
> -    nbd_export_set_name(exp, export_name);
> -    nbd_export_set_description(exp, export_description);
> 
>       if (device) {
>   #if HAVE_NBD_DEVICE
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/6] nbd: Merge nbd_export_set_name into nbd_export_new

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3873 bytes --]

On 1/10/19 4:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> 10.01.2019 10:13, Eric Blake wrote:
>> The existing NBD code had a weird split where nbd_export_new()
>> created an export but did not add it to the list of exported
>> names until a later nbd_export_set_name() came along and grabbed
>> a second reference on the object; later, nbd_export_close()
>> drops the second reference.  But since we never change the
>> name of an NBD export while it is exposed, it is easier to just
>> inline the process of setting the name as part of creating the
>> export.
>>
>> Inline the contents of nbd_export_set_name() and
>> nbd_export_set_description() into the two points in an export
>> lifecycle where they matter, then adjust both callers to pass
>> the name up front.  Note that all callers pass a non-NULL name,
>> (passing NULL at creation was for old style servers, but we
>> removed support for that in commit 7f7dfe2a).

I need to fix that sentence:


>> -void nbd_export_set_name(NBDExport *exp, const char *name)
>> -{
>> -    if (exp->name == name) {
>> -        return;
>> -    }
>> -
>> -    nbd_export_get(exp);
>> -    if (exp->name != NULL) {
>> -        g_free(exp->name);
>> -        exp->name = NULL;
>> -        QTAILQ_REMOVE(&exports, exp, next);
>> -        nbd_export_put(exp);
>> -    }

In the old code, exp->name == NULL was possible during a second
nbd_export_close(), so this conditional needs to be preserved if
nbd_export_close() can be called more than once on the same exp.

>> -    if (name != NULL) {
>> -        nbd_export_get(exp);
>> -        exp->name = g_strdup(name);
>> -        QTAILQ_INSERT_TAIL(&exports, exp, next);
>> -    }

Whereas this conditional was only possible right after nbd_export_new(),
and was always non-NULL, hence I converted it to an assert() for a
non-NULL name and unconditional code.

>> -    nbd_export_put(exp);
>> -}
>> -
>> -void nbd_export_set_description(NBDExport *exp, const char *description)
>> -{
>> -    g_free(exp->description);
>> -    exp->description = g_strdup(description);
>> -}
>> -
>>   void nbd_export_close(NBDExport *exp)
>>   {
>>       NBDClient *client, *next;
>> @@ -1568,8 +1549,14 @@ void nbd_export_close(NBDExport *exp)
>>       QTAILQ_FOREACH_SAFE(client, &exp->clients, next, next) {
>>           client_close(client, true);
>>       }
>> -    nbd_export_set_name(exp, NULL);
>> -    nbd_export_set_description(exp, NULL);
>> +    if (exp->name) {
>> +        nbd_export_put(exp);
>> +        g_free(exp->name);
>> +        exp->name = NULL;
>> +        QTAILQ_REMOVE(&exports, exp, next);
> 
> exp->name is always non-zero, and _get and _INSERT_TAIL were done independently from name,
> so with these four lines done unconditionally:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Now, on to the analysis of whether the code in nbd_export_close() needs
a conditional - yes, it does. Making it unconditional breaks the fact
that we have nested referencing semantics: You can create an export,
then call nbd_export_get(exp) to hold a second reference to it for as
long as the export remains alive. The first time nbd_export_close() is
called, you are telling the NBD server to no longer advertise the export
(no new clients can connect, because exp->name is now NULL), but all
existing clients continue to access the export just fine. The second
time nbd_export_close() is called, exp->name is already NULL, and we are
closing out all existing clients (if any are left) - it's how we
implemented the nbd-server-remove safe vs. hard mode, and how we could
add the hide/soft modes in the future (see block.json:NbdServerRemoveMode).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 2/6] nbd: Merge nbd_export_set_name into nbd_export_new

[Vladimir Sementsov-Ogievskiy]

10.01.2019 17:44, Eric Blake wrote:
> On 1/10/19 4:40 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 10.01.2019 10:13, Eric Blake wrote:
>>> The existing NBD code had a weird split where nbd_export_new()
>>> created an export but did not add it to the list of exported
>>> names until a later nbd_export_set_name() came along and grabbed
>>> a second reference on the object; later, nbd_export_close()
>>> drops the second reference.  But since we never change the
>>> name of an NBD export while it is exposed, it is easier to just
>>> inline the process of setting the name as part of creating the
>>> export.
>>>
>>> Inline the contents of nbd_export_set_name() and
>>> nbd_export_set_description() into the two points in an export
>>> lifecycle where they matter, then adjust both callers to pass
>>> the name up front.  Note that all callers pass a non-NULL name,
>>> (passing NULL at creation was for old style servers, but we
>>> removed support for that in commit 7f7dfe2a).
> 
> I need to fix that sentence:
> 
> 
>>> -void nbd_export_set_name(NBDExport *exp, const char *name)
>>> -{
>>> -    if (exp->name == name) {
>>> -        return;
>>> -    }
>>> -
>>> -    nbd_export_get(exp);
>>> -    if (exp->name != NULL) {
>>> -        g_free(exp->name);
>>> -        exp->name = NULL;
>>> -        QTAILQ_REMOVE(&exports, exp, next);
>>> -        nbd_export_put(exp);
>>> -    }
> 
> In the old code, exp->name == NULL was possible during a second
> nbd_export_close(), so this conditional needs to be preserved if
> nbd_export_close() can be called more than once on the same exp.
> 
>>> -    if (name != NULL) {
>>> -        nbd_export_get(exp);
>>> -        exp->name = g_strdup(name);
>>> -        QTAILQ_INSERT_TAIL(&exports, exp, next);
>>> -    }
> 
> Whereas this conditional was only possible right after nbd_export_new(),
> and was always non-NULL, hence I converted it to an assert() for a
> non-NULL name and unconditional code.
> 
>>> -    nbd_export_put(exp);
>>> -}
>>> -
>>> -void nbd_export_set_description(NBDExport *exp, const char *description)
>>> -{
>>> -    g_free(exp->description);
>>> -    exp->description = g_strdup(description);
>>> -}
>>> -
>>>    void nbd_export_close(NBDExport *exp)
>>>    {
>>>        NBDClient *client, *next;
>>> @@ -1568,8 +1549,14 @@ void nbd_export_close(NBDExport *exp)
>>>        QTAILQ_FOREACH_SAFE(client, &exp->clients, next, next) {
>>>            client_close(client, true);
>>>        }
>>> -    nbd_export_set_name(exp, NULL);
>>> -    nbd_export_set_description(exp, NULL);
>>> +    if (exp->name) {
>>> +        nbd_export_put(exp);
>>> +        g_free(exp->name);
>>> +        exp->name = NULL;
>>> +        QTAILQ_REMOVE(&exports, exp, next);
>>
>> exp->name is always non-zero, and _get and _INSERT_TAIL were done independently from name,
>> so with these four lines done unconditionally:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Now, on to the analysis of whether the code in nbd_export_close() needs
> a conditional - yes, it does. Making it unconditional breaks the fact
> that we have nested referencing semantics: You can create an export,
> then call nbd_export_get(exp) to hold a second reference to it for as
> long as the export remains alive. The first time nbd_export_close() is
> called, you are telling the NBD server to no longer advertise the export
> (no new clients can connect, because exp->name is now NULL), but all
> existing clients continue to access the export just fine. The second
> time nbd_export_close() is called, exp->name is already NULL, and we are
> closing out all existing clients (if any are left) - it's how we

hm, looks like it works in other way
void nbd_export_close(NBDExport *exp)
{
     NBDClient *client, *next;

     nbd_export_get(exp);
     QTAILQ_FOREACH_SAFE(client, &exp->clients, next, next) {
         client_close(client, true);
     }
     nbd_export_set_name(exp, NULL);
     nbd_export_set_description(exp, NULL);
     nbd_export_put(exp);
}

first or second time you call it, it will close all clients.

> implemented the nbd-server-remove safe vs. hard mode, and how we could
> add the hide/soft modes in the future (see block.json:NbdServerRemoveMode).
> 

and nbd_export_remove don't call nbd_export_close if it is safe mode and there are active clients.

aha, remember, we decided to not implement middle mode, which removes export from the list but keeps
existing connections.


But you are right that it is at least not obvious, that nbd_export_close not called twice.. And the fact
that it depends on (seems unrelated) name variable together with nbd_export_close called from
nbd_export_put and from other places (consider also my comment in nbd_export_put) - this all looks really
weird, and hope we'll refactor it one day.

Ok, for now it's safe to keep old condition, so exp->name != NULL is a sign that export exists in the list
(it worth a comment, as after removing nbd_export_set_name() it becomes more weird), anyway patch makes
things better, hope you'll add a comment:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

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

[Eric Blake]

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 <eblake@redhat.com>
---
 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;
+        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"
+  "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"
+_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 ===
-- 
2.20.1

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


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

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

[Vladimir Sementsov-Ogievskiy]

cc Nikolay

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.


Nikolay, do you have comments on this?


   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 <eblake@redhat.com>
> ---
>   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;
> +        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"
> +  "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"
> +_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

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

[Nikolay Shirokovskiy]

On 10.01.2019 15:02, Vladimir Sementsov-Ogievskiy wrote:
> cc Nikolay
> 
> 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.
> 
> 
> Nikolay, do you have comments on this?
> 
> 
>    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).

With bitmap-export-name we can use shorter bitmap names in NBD export.
For example it can be just UUID of corresponding snapshot. With merge
strategy this should be at least export-$UUID because $UUID is already
used for bitmaps created during snapshots.

Nikolay

>> 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 <eblake@redhat.com>
>> ---
>>   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;
>> +        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"
>> +  "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"
>> +_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 ===
>>
> 
> 

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]

On 1/10/19 9:11 AM, Nikolay Shirokovskiy wrote:

>>> 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.
>>
>>
>> Nikolay, do you have comments on this?
>>
>>
>>    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).
> 
> With bitmap-export-name we can use shorter bitmap names in NBD export.
> For example it can be just UUID of corresponding snapshot. With merge
> strategy this should be at least export-$UUID because $UUID is already
> used for bitmaps created during snapshots.
> 
> Nikolay

If I need to add in an x-bitmap-export-name, as a separate patch, we can
still do that; but I still think libvirt found it fairly easy to just
always create a non-persistent bitmap with the desired name, merge in
the bitmap(s) that belong to the incremental or differential snapshots,
and then expose that temporary bitmap (at which point, you are no longer
tied to the name of the persistent bitmaps, and thus don't need
bitmap-export-name to provide a shorter export name of an actual
persistent bitmap).  Be aware that the rest of this series drops
x-nbd-server-add-bitmap entirely; as well as John's pending changes that
drop the x- prefix from dirty-bitmap-enable/merge/disable, and changed
the merge semantics to take an array; so even if you've had demo code
working against the experimental names, you do have to plan to make some
tweaks. Speak up now [well, before 4.0 is final] if we are about to lock
something into stable that you can't manage to work with (as I have been
doing the same testing with my libvirt code, updating it to use the
stable names and retesting).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v2 4/6] nbd: Remove x-nbd-server-add-bitmap

[Eric Blake]

Now that nbd-server-add can do the same functionality, we no
longer need the experimental separate command.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/block.json | 23 -----------------------
 blockdev-nbd.c  | 23 -----------------------
 2 files changed, 46 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index 3d70420f763..5a79d639e8c 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -301,29 +301,6 @@
 { 'command': 'nbd-server-remove',
   'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }

-##
-# @x-nbd-server-add-bitmap:
-#
-# Expose a dirty bitmap associated with the selected export. The bitmap search
-# starts at the device attached to the export, and includes all backing files.
-# The exported bitmap is then locked until the NBD export is removed.
-#
-# @name: Export name.
-#
-# @bitmap: Bitmap name to search for.
-#
-# @bitmap-export-name: How the bitmap will be seen by nbd clients
-#                      (default @bitmap)
-#
-# Note: the client must use NBD_OPT_SET_META_CONTEXT with a query of
-# "qemu:dirty-bitmap:NAME" (where NAME matches @bitmap-export-name) to access
-# the exposed bitmap.
-#
-# Since: 3.0
-##
-  { 'command': 'x-nbd-server-add-bitmap',
-    'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': 'str'} }
-
 ##
 # @nbd-server-stop:
 #
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index ac7e993c35f..003ba7d7180 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -228,26 +228,3 @@ void qmp_nbd_server_stop(Error **errp)
     nbd_server_free(nbd_server);
     nbd_server = NULL;
 }
-
-void qmp_x_nbd_server_add_bitmap(const char *name, const char *bitmap,
-                                 bool has_bitmap_export_name,
-                                 const char *bitmap_export_name,
-                                 Error **errp)
-{
-    NBDExport *exp;
-
-    if (!nbd_server) {
-        error_setg(errp, "NBD server not running");
-        return;
-    }
-
-    exp = nbd_export_find(name);
-    if (exp == NULL) {
-        error_setg(errp, "Export '%s' is not found", name);
-        return;
-    }
-
-    nbd_export_bitmap(exp, bitmap,
-                      has_bitmap_export_name ? bitmap_export_name : bitmap,
-                      errp);
-}
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 4/6] nbd: Remove x-nbd-server-add-bitmap

[Vladimir Sementsov-Ogievskiy]

10.01.2019 10:13, Eric Blake wrote:
> Now that nbd-server-add can do the same functionality, we no
> longer need the experimental separate command.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v2 5/6] nbd: Merge nbd_export_bitmap into nbd_export_new

[Eric Blake]

We only have one caller that wants to export a bitmap name,
which it does right after creation of the export. But there is
still a brief window of time where an NBD client could see the
export but not the dirty bitmap, which a robust client would
have to interpret as meaning the entire image should be treated
as dirty.  Better is to eliminate the window entirely, by
inlining nbd_export_bitmap() into nbd_export_new(), and refusing
to create the bitmap in the first place if the requested bitmap
can't be located.

We also no longer need logic for setting a different bitmap
name compared to the bitmap being exported.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  9 ++---
 blockdev-nbd.c      | 11 +-----
 nbd/server.c        | 87 +++++++++++++++++++++------------------------
 qemu-nbd.c          |  4 +--
 4 files changed, 46 insertions(+), 65 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 2f9a2aeb73c..1971b557896 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -296,9 +296,9 @@ typedef struct NBDClient NBDClient;

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
                           const char *name, const char *description,
-                          uint16_t nbdflags, void (*close)(NBDExport *),
-                          bool writethrough, BlockBackend *on_eject_blk,
-                          Error **errp);
+                          const char *bitmap, uint16_t nbdflags,
+                          void (*close)(NBDExport *), bool writethrough,
+                          BlockBackend *on_eject_blk, Error **errp);
 void nbd_export_close(NBDExport *exp);
 void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
 void nbd_export_get(NBDExport *exp);
@@ -319,9 +319,6 @@ void nbd_client_put(NBDClient *client);
 void nbd_server_start(SocketAddress *addr, const char *tls_creds,
                       Error **errp);

-void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
-                       const char *bitmap_export_name, Error **errp);
-
 /* nbd_read
  * Reads @size bytes from @ioc. Returns 0 on success.
  */
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 003ba7d7180..0df6307be2d 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -175,7 +175,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         writable = false;
     }

-    exp = nbd_export_new(bs, 0, -1, name, NULL,
+    exp = nbd_export_new(bs, 0, -1, name, NULL, bitmap,
                          writable ? 0 : NBD_FLAG_READ_ONLY,
                          NULL, false, on_eject_blk, errp);
     if (!exp) {
@@ -186,15 +186,6 @@ 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;
-        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/nbd/server.c b/nbd/server.c
index 676fb4886d0..e613594fde0 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1457,9 +1457,9 @@ static void nbd_eject_notifier(Notifier *n, void *data)

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
                           const char *name, const char *description,
-                          uint16_t nbdflags, void (*close)(NBDExport *),
-                          bool writethrough, BlockBackend *on_eject_blk,
-                          Error **errp)
+                          const char *bitmap, uint16_t nbdflags,
+                          void (*close)(NBDExport *), bool writethrough,
+                          BlockBackend *on_eject_blk, Error **errp)
 {
     AioContext *ctx;
     BlockBackend *blk;
@@ -1507,6 +1507,43 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
     }
     exp->size -= exp->size % BDRV_SECTOR_SIZE;

+    if (bitmap) {
+        BdrvDirtyBitmap *bm = NULL;
+        BlockDriverState *bs = blk_bs(blk);
+
+        while (true) {
+            bm = bdrv_find_dirty_bitmap(bs, bitmap);
+            if (bm != NULL || bs->backing == NULL) {
+                break;
+            }
+
+            bs = bs->backing->bs;
+        }
+
+        if (bm == NULL) {
+            error_setg(errp, "Bitmap '%s' is not found", bitmap);
+            goto fail;
+        }
+
+        if ((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);
+            goto fail;
+        }
+
+        if (bdrv_dirty_bitmap_user_locked(bm)) {
+            error_setg(errp, "Bitmap '%s' is in use", bitmap);
+            goto fail;
+        }
+
+        bdrv_dirty_bitmap_set_qmp_locked(bm, true);
+        exp->export_bitmap = bm;
+        exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
+                                                     bitmap);
+    }
+
     exp->close = close;
     exp->ctx = blk_get_aio_context(blk);
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
@@ -2417,47 +2454,3 @@ void nbd_client_new(QIOChannelSocket *sioc,
     co = qemu_coroutine_create(nbd_co_client_start, client);
     qemu_coroutine_enter(co);
 }
-
-void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
-                       const char *bitmap_export_name, Error **errp)
-{
-    BdrvDirtyBitmap *bm = NULL;
-    BlockDriverState *bs = blk_bs(exp->blk);
-
-    if (exp->export_bitmap) {
-        error_setg(errp, "Export bitmap is already set");
-        return;
-    }
-
-    while (true) {
-        bm = bdrv_find_dirty_bitmap(bs, bitmap);
-        if (bm != NULL || bs->backing == NULL) {
-            break;
-        }
-
-        bs = bs->backing->bs;
-    }
-
-    if (bm == NULL) {
-        error_setg(errp, "Bitmap '%s' is not found", bitmap);
-        return;
-    }
-
-    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;
-    }
-
-    if (bdrv_dirty_bitmap_user_locked(bm)) {
-        error_setg(errp, "Bitmap '%s' is in use", bitmap);
-        return;
-    }
-
-    bdrv_dirty_bitmap_set_qmp_locked(bm, true);
-    exp->export_bitmap = bm;
-    exp->export_bitmap_context =
-            g_strdup_printf("qemu:dirty-bitmap:%s", bitmap_export_name);
-}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 696bd78a2e2..03813187846 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1016,8 +1016,8 @@ int main(int argc, char **argv)
     }

     exp = nbd_export_new(bs, dev_offset, fd_size, export_name,
-                         export_description, nbdflags, nbd_export_closed,
-                         writethrough, NULL, &error_fatal);
+                         export_description, NULL, nbdflags,
+                         nbd_export_closed, writethrough, NULL, &error_fatal);

     if (device) {
 #if HAVE_NBD_DEVICE
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 5/6] nbd: Merge nbd_export_bitmap into nbd_export_new

[Vladimir Sementsov-Ogievskiy]

10.01.2019 10:13, Eric Blake wrote:
> We only have one caller that wants to export a bitmap name,
> which it does right after creation of the export. But there is
> still a brief window of time where an NBD client could see the
> export but not the dirty bitmap, which a robust client would
> have to interpret as meaning the entire image should be treated
> as dirty.  Better is to eliminate the window entirely, by
> inlining nbd_export_bitmap() into nbd_export_new(), and refusing
> to create the bitmap in the first place if the requested bitmap
> can't be located.
> 
> We also no longer need logic for setting a different bitmap
> name compared to the bitmap being exported.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   include/block/nbd.h |  9 ++---
>   blockdev-nbd.c      | 11 +-----
>   nbd/server.c        | 87 +++++++++++++++++++++------------------------
>   qemu-nbd.c          |  4 +--
>   4 files changed, 46 insertions(+), 65 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 2f9a2aeb73c..1971b557896 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -296,9 +296,9 @@ typedef struct NBDClient NBDClient;
> 
>   NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
>                             const char *name, const char *description,
> -                          uint16_t nbdflags, void (*close)(NBDExport *),
> -                          bool writethrough, BlockBackend *on_eject_blk,
> -                          Error **errp);
> +                          const char *bitmap, uint16_t nbdflags,
> +                          void (*close)(NBDExport *), bool writethrough,
> +                          BlockBackend *on_eject_blk, Error **errp);
>   void nbd_export_close(NBDExport *exp);
>   void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
>   void nbd_export_get(NBDExport *exp);
> @@ -319,9 +319,6 @@ void nbd_client_put(NBDClient *client);
>   void nbd_server_start(SocketAddress *addr, const char *tls_creds,
>                         Error **errp);
> 
> -void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
> -                       const char *bitmap_export_name, Error **errp);
> -
>   /* nbd_read
>    * Reads @size bytes from @ioc. Returns 0 on success.
>    */
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 003ba7d7180..0df6307be2d 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -175,7 +175,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>           writable = false;
>       }
> 
> -    exp = nbd_export_new(bs, 0, -1, name, NULL,
> +    exp = nbd_export_new(bs, 0, -1, name, NULL, bitmap,

bitmap is guaranteed to be NULL if has_bitmap is false? Or should we do has_bitmap ? bitmap : NULL?

if yes or with fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 5/6] nbd: Merge nbd_export_bitmap into nbd_export_new

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]

On 1/10/19 6:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> 10.01.2019 10:13, Eric Blake wrote:
>> We only have one caller that wants to export a bitmap name,
>> which it does right after creation of the export. But there is
>> still a brief window of time where an NBD client could see the
>> export but not the dirty bitmap, which a robust client would
>> have to interpret as meaning the entire image should be treated
>> as dirty.  Better is to eliminate the window entirely, by
>> inlining nbd_export_bitmap() into nbd_export_new(), and refusing
>> to create the bitmap in the first place if the requested bitmap
>> can't be located.
>>
>> We also no longer need logic for setting a different bitmap
>> name compared to the bitmap being exported.
>>

>> +++ b/blockdev-nbd.c
>> @@ -175,7 +175,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>>           writable = false;
>>       }
>>
>> -    exp = nbd_export_new(bs, 0, -1, name, NULL,
>> +    exp = nbd_export_new(bs, 0, -1, name, NULL, bitmap,
> 
> bitmap is guaranteed to be NULL if has_bitmap is false? Or should we do has_bitmap ? bitmap : NULL?

We fixed our QAPI generators to guarantee that has_foo = false implies
foo == NULL, several years ago.  If you populate a QAPI struct by hand
instead of via the generated visitors, you should use g_malloc0() to
match the same guarantees.  So I don't need the ?:.

> 
> if yes or with fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v2 6/6] qemu-nbd: Add --bitmap=NAME option

[Eric Blake]

Having to fire up qemu, then use QMP commands for nbd-server-start
and nbd-server-add, just to expose a persistent dirty bitmap, is
rather tedious.  Make it possible to expose a dirty bitmap using
just qemu-nbd (of course, for now this only works when qemu-nbd is
visiting a BDS formatted as qcow2).

Of course, any good feature also needs unit testing, so expand
iotest 223 to cover it.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qemu-nbd.texi              |  4 ++++
 qemu-nbd.c                 | 10 ++++++++--
 tests/qemu-iotests/223     | 18 +++++++++++++++++-
 tests/qemu-iotests/223.out | 12 +++++++++++-
 4 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 9a84e81eed9..96b1546006a 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -45,6 +45,10 @@ auto-detecting
 Export the disk as read-only
 @item -P, --partition=@var{num}
 Only expose partition @var{num}
+@item -B, --bitmap=@var{name}
+If @var{filename} has a qcow2 persistent bitmap @var{name}, expose
+that bitmap via the ``qemu:dirty-bitmap:@var{name}'' context
+accessible through NBD_OPT_SET_META_CONTEXT.
 @item -s, --snapshot
 Use @var{filename} as an external snapshot, create a temporary
 file with backing_file=@var{filename}, redirect the write to
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 03813187846..26f5c08821d 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -95,6 +95,7 @@ static void usage(const char *name)
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET       offset into the image\n"
 "  -P, --partition=NUM       only expose partition NUM\n"
+"  -B, --bitmap=NAME         expose a persistent dirty bitmap\n"
 "\n"
 "General purpose options:\n"
 "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
@@ -509,7 +510,7 @@ int main(int argc, char **argv)
     off_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:";
+    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:";
     struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -519,6 +520,7 @@ int main(int argc, char **argv)
         { "offset", required_argument, NULL, 'o' },
         { "read-only", no_argument, NULL, 'r' },
         { "partition", required_argument, NULL, 'P' },
+        { "bitmap", required_argument, NULL, 'B' },
         { "connect", required_argument, NULL, 'c' },
         { "disconnect", no_argument, NULL, 'd' },
         { "snapshot", no_argument, NULL, 's' },
@@ -558,6 +560,7 @@ int main(int argc, char **argv)
     QDict *options = NULL;
     const char *export_name = ""; /* Default export name */
     const char *export_description = NULL;
+    const char *bitmap = NULL;
     const char *tlscredsid = NULL;
     bool imageOpts = false;
     bool writethrough = true;
@@ -695,6 +698,9 @@ int main(int argc, char **argv)
                 exit(EXIT_FAILURE);
             }
             break;
+        case 'B':
+            bitmap = optarg;
+            break;
         case 'k':
             sockpath = optarg;
             if (sockpath[0] != '/') {
@@ -1016,7 +1022,7 @@ int main(int argc, char **argv)
     }

     exp = nbd_export_new(bs, dev_offset, fd_size, export_name,
-                         export_description, NULL, nbdflags,
+                         export_description, bitmap, nbdflags,
                          nbd_export_closed, writethrough, NULL, &error_fatal);

     if (device) {
diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 1f6822f9489..d7c4a31a181 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -25,6 +25,7 @@ status=1 # failure is the default!

 _cleanup()
 {
+    nbd_server_stop
     _cleanup_test_img
     _cleanup_qemu
     rm -f "$TEST_DIR/nbd"
@@ -35,6 +36,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 . ./common.qemu
+. ./common.nbd

 _supported_fmt qcow2
 _supported_proto file # uses NBD as well
@@ -153,7 +155,7 @@ $QEMU_IMG map --output=json --image-opts \
   "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map

 echo
-echo "=== End NBD server ==="
+echo "=== End qemu NBD server ==="
 echo

 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
@@ -165,6 +167,20 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return"

+echo
+echo "=== Use qemu-nbd as server ==="
+echo
+
+nbd_server_start_unix_socket -r -f $IMGFMT -B b "$TEST_IMG"
+IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
+$QEMU_IMG map --output=json --image-opts \
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
+
+nbd_server_start_unix_socket -f $IMGFMT -B b2 "$TEST_IMG"
+IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
+$QEMU_IMG map --output=json --image-opts \
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 7135bf59bb8..ad3b2727ba8 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -58,11 +58,21 @@ read 2097152/2097152 bytes at offset 2097152
 { "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true},
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]

-=== End NBD server ===
+=== End qemu NBD server ===

 {"return": {}}
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
 {"return": {}}
 {"return": {}}
+
+=== Use qemu-nbd as server ===
+
+[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": false},
+{ "start": 65536, "length": 2031616, "depth": 0, "zero": false, "data": true},
+{ "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
+[{ "start": 0, "length": 512, "depth": 0, "zero": false, "data": true},
+{ "start": 512, "length": 512, "depth": 0, "zero": false, "data": false},
+{ "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true},
+{ "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
 *** done
-- 
2.20.1