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 <ebl...@redhat.com>
>> ---
>> 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
