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

[Vladimir Sementsov-Ogievskiy]

16.08.2019 13:23, Vladimir Sementsov-Ogievskiy wrote:
> 15.08.2019 21:50, 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.
>>
>> See also: https://bugzilla.redhat.com/1708300
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>   docs/interop/nbd.txt | 1 +
>>   include/block/nbd.h  | 2 +-
>>   blockdev-nbd.c       | 2 +-
>>   nbd/server.c         | 4 +++-
>>   qemu-nbd.c           | 2 +-
>>   5 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
>> index fc64473e02b2..6dfec7f47647 100644
>> --- a/docs/interop/nbd.txt
>> +++ b/docs/interop/nbd.txt
>> @@ -53,3 +53,4 @@ the operation of that feature.
>>   * 2.12: NBD_CMD_BLOCK_STATUS for "base:allocation"
>>   * 3.0: NBD_OPT_STARTTLS with TLS Pre-Shared Keys (PSK),
>>   NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE
>> +* 4.2: NBD_FLAG_CAN_MULTI_CONN for sharable read-only exports
>> diff --git a/include/block/nbd.h b/include/block/nbd.h
>> index 7b36d672f046..991fd52a5134 100644
>> --- a/include/block/nbd.h
>> +++ b/include/block/nbd.h
>> @@ -326,7 +326,7 @@ typedef struct NBDClient NBDClient;
>>
>>   NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>>                             uint64_t size, const char *name, const char 
>> *desc,
>> -                          const char *bitmap, uint16_t nbdflags,
>> +                          const char *bitmap, uint16_t nbdflags, bool 
>> shared,
>>                             void (*close)(NBDExport *), bool writethrough,
>>                             BlockBackend *on_eject_blk, Error **errp);
>>   void nbd_export_close(NBDExport *exp);
>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>> index 66eebab31875..e5d228771292 100644
>> --- a/blockdev-nbd.c
>> +++ 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,
> 
> s/true/!writable ?

Oh, I see, John already noticed this, it's checked in nbd_export_new anyway..

> 
>>                            NULL, false, on_eject_blk, errp);
>>       if (!exp) {
>>           return;
>> diff --git a/nbd/server.c b/nbd/server.c
>> index a2cf085f7635..a602d85070ff 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -1460,7 +1460,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
>>
>>   NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>>                             uint64_t size, const char *name, const char 
>> *desc,
>> -                          const char *bitmap, uint16_t nbdflags,
>> +                          const char *bitmap, uint16_t nbdflags, bool 
>> shared,
>>                             void (*close)(NBDExport *), bool writethrough,
>>                             BlockBackend *on_eject_blk, Error **errp)
>>   {
>> @@ -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;

For me it looks a bit strange: we already have nbdflags parameter for 
nbd_export_new(), why
to add a separate boolean to pass one of nbdflags flags?

Also, for qemu-nbd, shouldn't we allow -e only together with -r ?

>>       }
>>       blk = blk_new(bdrv_get_aio_context(bs), perm,
>>                     BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 049645491dab..55f5ceaf5c92 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -1173,7 +1173,7 @@ int main(int argc, char **argv)
>>       }
>>
>>       export = nbd_export_new(bs, dev_offset, fd_size, export_name,
>> -                            export_description, bitmap, nbdflags,
>> +                            export_description, bitmap, nbdflags, shared > 
>> 1,
>>                               nbd_export_closed, writethrough, NULL,
>>                               &error_fatal);
>>
> 
> 


-- 
Best regards,
Vladimir