[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] block: nbd: Fix dirty bitmap context name
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH] block: nbd: Fix dirty bitmap context name |
Date: |
Thu, 19 Dec 2019 15:06:11 +0000 |
19.12.2019 17:49, Nir Soffer wrote:
> On Thu, Dec 19, 2019 at 3:42 PM Vladimir Sementsov-Ogievskiy
> <address@hidden> wrote:
>>
>> I'd not call it a "fix".. As it implies something broken.
>>
>> [edit: OK, now I see that something is broken, and why you called it "fix",
>> see below]
>>
>> 19.12.2019 15:51, Nir Soffer wrote:
>>> When adding an export with a dirty bitmap, expose the bitmap at:
>>>
>>> qemu:dirty-bitmap:export-name
>>
>> export-name? But it would be extra information, as client already knows
>> with which export it works.
>
> Right, using empty string would be good as well.
It will conflict with already documented in docs/interop/nbd.txt:
* "qemu:dirty-bitmap:" - returns list of all available dirty-bitmap
metadata contexts.
>
>> NBD commands NBD_OPT_GET/SET_META_CONTEXT includes export name as a
>> parameter, so, any queried metadata (bitmaps, etc) is always bound to
>> specified export.
>>
>>>
>>> This matches qapi documentation, and user expectations.
>>
>> Hmmm,
>> "qemu" namespace is documented in docs/interop/nbd.txt, not in Qapi,
>> which is also mention in official NBD spec.
>>
>>
>> Ahh, I see, it's documented as
>>
>> +# @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)
>>
>> and it is logical to assume that export name (which is @name argument) is
>> mentioned. But we never mentioned it. This is just documented after
>> removed experimenatl command x-nbd-server-add-bitmap,
>>
>> look at
>>
>> commit 7dc570b3806e5b0a4c9219061556ed5a4a0de80c
>> Author: Eric Blake <address@hidden>
>> Date: Fri Jan 11 13:47:18 2019 -0600
>>
>> nbd: Remove x-nbd-server-add-bitmap
>>
>> ...
>>
>> -# @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.
>>
>>
>> So, this "NAME" is saved and now looks incorrect. What should be fixed, is
>> Qapi
>> documentation.
>>
>>
>>>
>>> Without this, qemu leaks libvirt implementations details to clients by
>>> exposing the bitmap using the actual bitmap name:
>>>
>>> qemu:dirty-bitmap:bitmap-name
>>
>> Yes, "qemu" namespace specification says:
>> qemu:dirty-bitmap:<dirty-bitmap-export-name>
>>
>> so, <dirty-bitmap-export-name> may be exact bitmap name or may be something
>> other.
>>
>> We just don't have an interface to set such name. It was in removed
>> x-nbd-server-add-bitmap
>>
>> So, if you need this possibility now, the correct way is to add
>> 'export-bitmap-name'
>> optional parameter to nbd-server-add, like it was in x-nbd-server-add-bitmap
>
> I don't think we need such API. How would it help users trying to get
> dirty extents
> from an image?
It will solve the issue, which you mentioned in your commit message:
"qemu leaks libvirt implementations details"
>
>>> And all clients need to duplicate code like:
>>>
>>> meta_context = "qemu:dirty-bitmap:backup-" + export_name
>>>
>>> NBD allows exposing multiple bitmaps under "qemu:dirty-bitmap:"
>>> namespace, and clients can query the available bitmaps, but it is not
>>> clear what a client should do if a server provides multiple bitmaps.
>>> ---
>>> nbd/server.c | 2 +-
>>> tests/qemu-iotests/223 | 16 ++++++++--------
>>> tests/qemu-iotests/223.out | 8 ++++----
>>> 3 files changed, 13 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/nbd/server.c b/nbd/server.c
>>> index 24ebc1a805..f20f2994c0 100644
>>> --- a/nbd/server.c
>>> +++ b/nbd/server.c
>>> @@ -1574,7 +1574,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
>>> uint64_t dev_offset,
>>> exp->export_bitmap = bm;
>>> assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
>>> exp->export_bitmap_context =
>>> g_strdup_printf("qemu:dirty-bitmap:%s",
>>> - bitmap);
>>> + name);
>>
>> I think it's a bad idea to automatically name bitmap after export. Actually
>> export may
>> have several bitmaps (we just don't support it).
>
> What are the semantics of multiple dirty bitmaps for same export? How
> users are going
> to use this?
semantic of the protocol defined in docs/interop/nbd.txt:
...
The "qemu" namespace currently contains only one type of context,
related to exposing the contents of a dirty bitmap alongside the
associated disk contents. That context has the following form:
qemu:dirty-bitmap:<dirty-bitmap-export-name>
...
We may export several bitmaps. Still, currently Qemu can export only one bitmap.
>
>> "NAME" in Qapi spec is a mistake.
>>
>>> assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
>>> }
>>>
>>> diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
>>> index ea69cd4b8b..3068a7c280 100755
>>> --- a/tests/qemu-iotests/223
>>> +++ b/tests/qemu-iotests/223
>>> @@ -167,7 +167,7 @@ $QEMU_IO -r -c 'r -P 0x22 512 512' -c 'r -P 0 512k
>>> 512k' -c 'r -P 0x11 1m 1m' \
>>> $QEMU_IMG map --output=json --image-opts \
>>> "$IMG" | _filter_qemu_img_map
>>> $QEMU_IMG map --output=json --image-opts \
>>> - "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
>>> + "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
>>>
>>> echo
>>> echo "=== Contrast to small granularity dirty-bitmap ==="
>>> @@ -175,7 +175,7 @@ echo
>>>
>>> IMG="driver=nbd,export=n2,server.type=unix,server.path=$SOCK_DIR/nbd"
>>> $QEMU_IMG map --output=json --image-opts \
>>> - "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
>>> + "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n2" | _filter_qemu_img_map
>>>
>>> echo
>>> echo "=== End qemu NBD server ==="
>>> @@ -199,15 +199,15 @@ 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"
>>> +nbd_server_start_unix_socket -r -f $IMGFMT -x n -B b "$TEST_IMG"
>>> +IMG="driver=nbd,export=n,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
>>> + "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _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"
>>> +nbd_server_start_unix_socket -f $IMGFMT -x n -B b2 "$TEST_IMG"
>>> +IMG="driver=nbd,export=n,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
>>> + "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:n" | _filter_qemu_img_map
>>>
>>> # success, all done
>>> echo '*** done'
>>> diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
>>> index f175598802..9f879add60 100644
>>> --- a/tests/qemu-iotests/223.out
>>> +++ b/tests/qemu-iotests/223.out
>>> @@ -61,7 +61,7 @@ exports available: 2
>>> max block: 33554432
>>> available meta contexts: 2
>>> base:allocation
>>> - qemu:dirty-bitmap:b
>>> + qemu:dirty-bitmap:n
>>> export: 'n2'
>>> size: 4194304
>>> flags: 0xced ( flush fua trim zeroes df cache fast-zero )
>>> @@ -70,7 +70,7 @@ exports available: 2
>>> max block: 33554432
>>> available meta contexts: 2
>>> base:allocation
>>> - qemu:dirty-bitmap:b2
>>> + qemu:dirty-bitmap:n2
>>>
>>> === Contrast normal status to large granularity dirty-bitmap ===
>>>
>>> @@ -141,7 +141,7 @@ exports available: 2
>>> max block: 33554432
>>> available meta contexts: 2
>>> base:allocation
>>> - qemu:dirty-bitmap:b
>>> + qemu:dirty-bitmap:n
>>> export: 'n2'
>>> size: 4194304
>>> flags: 0xced ( flush fua trim zeroes df cache fast-zero )
>>> @@ -150,7 +150,7 @@ exports available: 2
>>> max block: 33554432
>>> available meta contexts: 2
>>> base:allocation
>>> - qemu:dirty-bitmap:b2
>>> + qemu:dirty-bitmap:n2
>>>
>>> === Contrast normal status to large granularity dirty-bitmap ===
>>>
>>>
>>
>>
>> --
>> Best regards,
>> Vladimir
>
--
Best regards,
Vladimir
- Re: [PATCH] block: nbd: Fix dirty bitmap context name, (continued)
- Re: [PATCH] block: nbd: Fix dirty bitmap context name, Nir Soffer, 2019/12/19
- Re: [PATCH] block: nbd: Fix dirty bitmap context name, Vladimir Sementsov-Ogievskiy, 2019/12/19
- Re: [PATCH] block: nbd: Fix dirty bitmap context name, Nir Soffer, 2019/12/19
- Re: [PATCH] block: nbd: Fix dirty bitmap context name, Vladimir Sementsov-Ogievskiy, 2019/12/20
- Re: [PATCH] block: nbd: Fix dirty bitmap context name, Peter Krempa, 2019/12/20
- Re: [PATCH] block: nbd: Fix dirty bitmap context name, Vladimir Sementsov-Ogievskiy, 2019/12/20
- Re: [PATCH] block: nbd: Fix dirty bitmap context name, Peter Krempa, 2019/12/20
- Re: [PATCH] block: nbd: Fix dirty bitmap context name, Vladimir Sementsov-Ogievskiy, 2019/12/20
- Re: [PATCH] block: nbd: Fix dirty bitmap context name, Nir Soffer, 2019/12/25
Re: [PATCH] block: nbd: Fix dirty bitmap context name, Nir Soffer, 2019/12/19
- Re: [PATCH] block: nbd: Fix dirty bitmap context name,
Vladimir Sementsov-Ogievskiy <=