Re: [Qemu-devel] [Qemu-block] [PATCH] nbd: Advertise multi-conn for shared read-only connections

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2415 bytes --]

On 8/15/19 4:45 PM, John Snow wrote:
> 
> 
> On 8/15/19 2:50 PM, Eric Blake wrote:
>> The NBD specification defines NBD_FLAG_CAN_MULTI_CONN, which can be
>> advertised when the server promises cache consistency between
>> simultaneous clients (basically, rules that determine what FUA and
>> flush from one client are able to guarantee for reads from another
>> client).  When we don't permit simultaneous clients (such as qemu-nbd
>> without -e), the bit makes no sense; and for writable images, we
>> probably have a lot more work before we can declare that actions from
>> one client are cache-consistent with actions from another.  But for
>> read-only images, where flush isn't changing any data, we might as
>> well advertise multi-conn support.  What's more, advertisement of the
>> bit makes it easier for clients to determine if 'qemu-nbd -e' was in
>> use, where a second connection will succeed rather than hang until the
>> first client goes away.
>>
>> This patch affects qemu as server in advertising the bit.  We may want
>> to consider patches to qemu as client to attempt parallel connections
>> for higher throughput by spreading the load over those connections
>> when a server advertises multi-conn, but for now sticking to one
>> connection per nbd:// BDS is okay.
>>

>> +++ b/blockdev-nbd.c
>> @@ -189,7 +189,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>>      }
>>
>>      exp = nbd_export_new(bs, 0, len, name, NULL, bitmap,
>> -                         writable ? 0 : NBD_FLAG_READ_ONLY,
>> +                         writable ? 0 : NBD_FLAG_READ_ONLY, true,
>>                           NULL, false, on_eject_blk, errp);
> 
> Why is it okay to force the share bit on regardless of the value of
> 'writable' ?

Well, it's probably not, except that...


>> @@ -1486,6 +1486,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>>      perm = BLK_PERM_CONSISTENT_READ;
>>      if ((nbdflags & NBD_FLAG_READ_ONLY) == 0) {
>>          perm |= BLK_PERM_WRITE;
>> +    } else if (shared) {
>> +        nbdflags |= NBD_FLAG_CAN_MULTI_CONN;
>>      }

requesting shared=true has no effect for a writable export.

I can tweak it for less confusion, though.

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