Re: [Qemu-devel] [PATCH v2 1/2] nbd/client: Add x-dirty-bitmap to query bitmap from server

[Eric Blake <eblake@redhat.com>]

On 07/03/2018 04:46 AM, Vladimir Sementsov-Ogievskiy wrote:

>>   #
>> +# @x-dirty-bitmap: A "qemu:dirty-bitmap:NAME" string to query in 
>> place of
>> +#                  traditional "base:allocation" block status (see
>> +#                  NBD_OPT_LIST_META_CONTEXT in the NBD protocol) 
>> (since 3.0)
>> +#
> 
> "x-dirty-bitmap=qemu:dirty-bitmap:NAME", is a bit strange, looks like it 
> should be "x-dirty-bitmap=NAME", and "qemu:dirty-bitmap" added 
> automatically. (and you don't check it, so actually this parameter is 
> x-meta-context, and user can select any context, for example, 
> "base:allocation", so "x-meta-context=qemu:dirty-bitmap:NAME" sounds 
> better too). But I'm ok to leave it as is for now, with x- prefix.

Good point on 'x-meta-context' being slightly nicer; but bikeshedding an 
experimental name doesn't really impact the release.

For 3.1, I'd love to have:

qemu-img map --output=json --image-opts driver=nbd,...,bitmap=FOO

automatically connect to qemu:dirty-bitmap:FOO in addition to block 
status, resulting in output for iotest 223 that resembles:

[{ "start": 0, "length": 1048576, "depth": 0, "zero": true, "data": false},
{ "start": 1048576, "length": 1048576, "depth": 0, "zero": false, 
"data": true},
{ "start": 2097152, "length": 2097152, "depth": 0, "zero": false, 
"data": true, "dirty": true}]

(that is, an optional 'dirty' member added to the JSON to identify the 
dirty portions, in addition to everything else already identified).  I 
also want to enhance that test to show that discarding clusters works 
during dirty tracking (that is, a "data":false, "dirty":true entry 
should be demonstrated).

Getting to that point will mean adding a new BDRV_BLOCK_DIRTY bit to the 
output of bdrv_block_status() (even if only the NBD and passthrough 
drivers set it), as well as teaching qemu-img map to optionally honor 
that bit when present.  It also means the NBD client code will be 
subscribing to status from two meta contexts at once, with the second 
being exactly a qemu:dirty-bitmap (rather than a free-for-all string 
that x-dirty-bitmap currently gives us).

It's too late to get these improvements into 3.0, but should be a good 
path forward in the next few months.


>> @@ -982,9 +983,11 @@ int nbd_client_init(BlockDriverState *bs,
>>       client->info.request_sizes = true;
>>       client->info.structured_reply = true;
>>       client->info.base_allocation = true;
>> +    client->info.x_dirty_bitmap = g_strdup(x_dirty_bitmap);
>>       ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
>>                                   tlscreds, hostname,
>>                                   &client->ioc, &client->info, errp);
>> +    g_free(client->info.x_dirty_bitmap);
> 
> hm, pointer remains invalid. If you want free it here, what is the 
> reason to strdup it? At least, it worth to zero out the pointer after 
> g_free.. Or, free it not here but in client close.

The g_strdup() is necessary since client.x_dirty_bitmap is 'const char 
*'.  Nothing used the pointer after it is freed, although I could agree 
that assigning it to NULL to make that point obvious wouldn't hurt; 
however, the pull request was already taken as-is, so we can just live 
with it until the 3.1 improvements.

> 
> with pointer zeroed or g_free to client destroy procedure,
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 
> 

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