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

[John Snow <jsnow@redhat.com>]

On 8/15/19 5:54 PM, Eric Blake wrote:
> 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.
> 

"Yes John, when it's an else-if it really does matter what specific
condition it's following."

(Ah, there it is.)

Yeah, I think if you have hopes to support this flag in the future for
writable exports, I think it might be nicer to reject this bit for RW;
and adjust the caller to only request it conditionally.

Or not. I guess we don't have to maintain backwards compatibility for
internal API like that, so ... dealer's choice:

Reviewed-by: John Snow <jsnow@redhat.com>