Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] nbd: Improve per-export flag handling in server
23.08.2019 17:37, Eric Blake wrote: > When creating a read-only image, we are still advertising support for > TRIM and WRITE_ZEROES to the client, even though the client should not > be issuing those commands. But seeing this requires looking across > multiple functions: > > All callers to nbd_export_new() passed a single flag based solely on > whether the export allows writes. Later, we then pass a constant set > of flags to nbd_negotiate_options() (namely, the set of flags which we > always support, at least for writable images), which is then further > dynamically modified based on client requests for structured options. > Finally, when processing NBD_OPT_EXPORT_NAME or NBD_OPT_EXPORT_GO we > bitwise-or the original caller's flag with the runtime set of flags > we've built up over several functions. > > Let's refactor things to instead compute a baseline of flags as soon > as possible, in nbd_export_new(), and changing the signature for the > callers to pass in a simpler bool rather than having to figure out > flags. We can then get rid of the 'myflags' parameter to various > functions, and instead refer to client for everything we need (we > still have to perform a bitwise-OR during NBD_OPT_EXPORT_NAME and > NBD_OPT_EXPORT_GO, but it's easier to see what is being computed). > This lets us quit advertising senseless flags for read-only images, as > well as making the next patch for exposing FAST_ZERO support easier to > write. > > Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] nbd: Improve per-export flag handling in server
On 8/30/19 6:32 PM, Eric Blake wrote:
@@ -458,10 +458,13 @@ static int
nbd_negotiate_handle_export_name(NBDClient *client,
return -EINVAL;
}
-trace_nbd_negotiate_new_style_size_flags(client->exp->size,
- client->exp->nbdflags |
myflags);
+myflags = client->exp->nbdflags;
+if (client->structured_reply) {
+myflags |= NBD_FLAG_SEND_DF;
+}
>>>
>>>
>>> why we cant do just
>>> client->exp->nbdflags |= NBD_FLAG_SEND_DF ?
>>
>> Because myflags is the runtime flags for _this_ client, while
>> client->exp->nbdflags are the base flags shared by _all_ clients. If
>> client A requests structured reply, but client B does not, then we don't
>> want to advertise DF to client B; but amending client->exp->nbdflags
>> would have that effect.
>
> I stand corrected - it looks like a fresh client->exp is created per
> client, as evidenced by:
I need to quit replying to myself, but my test was flawed. Modern
clients don't go through NBD_OPT_EXPORT_NAME, so my added line...
> +++ w/nbd/server.c
> @@ -457,6 +457,7 @@ static int
> nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
> myflags = client->exp->nbdflags;
> if (client->structured_reply) {
> myflags |= NBD_FLAG_SEND_DF;
> +client->exp->nbdflags |= NBD_FLAG_SEND_DF;
> }
...was not getting reached. If I instead tweak NBD_OPT_GO:
diff --git i/nbd/server.c w/nbd/server.c
index 6f3a83704fb3..da1ef793f6df 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -640,6 +640,7 @@ static int nbd_negotiate_handle_info(NBDClient
*client, Error **errp)
myflags = exp->nbdflags;
if (client->structured_reply) {
myflags |= NBD_FLAG_SEND_DF;
+exp->nbdflags |= NBD_FLAG_SEND_DF;
}
trace_nbd_negotiate_new_style_size_flags(exp->size, myflags);
stq_be_p(buf, exp->size);
> $ ./qemu-nbd -r -f raw file -t &
>
> $ ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
> nbd://localhost:10809 -c quit
> 32145@1567207628.519883:nbd_receive_negotiate_size_flags Size is
> 1049088, export flags 0x48f
>
> $ MY_HACK=1 ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
> nbd://localhost:10809 -c quit
> 32156@1567207630.417815:nbd_receive_negotiate_size_flags Size is
> 1049088, export flags 0x40f
>
Then this reports 0x48f, proving that my initial reaction was correct:
client->exp is a shared resource across multiple connections, but
advertising DF must be a per-connection decision.
> $ ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
> nbd://localhost:10809 -c quit
> 32167@1567207635.202940:nbd_receive_negotiate_size_flags Size is
> 1049088, export flags 0x48f
>
> The export flags change per client, so I _can_ store into
> client->exp->nbdflags. Will do that for v2.
I see nothing to change for v2, so I'm inclined to take this patch as is.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] nbd: Improve per-export flag handling in server
On 8/30/19 6:10 PM, Eric Blake wrote:
> On 8/30/19 1:00 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 23.08.2019 17:37, Eric Blake wrote:
>>> When creating a read-only image, we are still advertising support for
>>> TRIM and WRITE_ZEROES to the client, even though the client should not
>>> be issuing those commands. But seeing this requires looking across
>>> multiple functions:
>>>
>
>>> @@ -458,10 +458,13 @@ static int nbd_negotiate_handle_export_name(NBDClient
>>> *client,
>>> return -EINVAL;
>>> }
>>>
>>> -trace_nbd_negotiate_new_style_size_flags(client->exp->size,
>>> - client->exp->nbdflags |
>>> myflags);
>>> +myflags = client->exp->nbdflags;
>>> +if (client->structured_reply) {
>>> +myflags |= NBD_FLAG_SEND_DF;
>>> +}
>>
>>
>> why we cant do just
>> client->exp->nbdflags |= NBD_FLAG_SEND_DF ?
>
> Because myflags is the runtime flags for _this_ client, while
> client->exp->nbdflags are the base flags shared by _all_ clients. If
> client A requests structured reply, but client B does not, then we don't
> want to advertise DF to client B; but amending client->exp->nbdflags
> would have that effect.
I stand corrected - it looks like a fresh client->exp is created per
client, as evidenced by:
diff --git i/nbd/client.c w/nbd/client.c
index b9dc829175f9..9e05f1a0e2a3 100644
--- i/nbd/client.c
+++ w/nbd/client.c
@@ -1011,6 +1011,8 @@ int nbd_receive_negotiate(AioContext *aio_context,
QIOChannel *ioc,
assert(info->name);
trace_nbd_receive_negotiate_name(info->name);
+if (getenv ("MY_HACK"))
+info->structured_reply = false;
result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname,
outioc,
info->structured_reply, , errp);
diff --git i/nbd/server.c w/nbd/server.c
index d5078f7468af..6f3a83704fb3 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -457,6 +457,7 @@ static int
nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
myflags = client->exp->nbdflags;
if (client->structured_reply) {
myflags |= NBD_FLAG_SEND_DF;
+client->exp->nbdflags |= NBD_FLAG_SEND_DF;
}
trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags);
stq_be_p(buf, client->exp->size);
$ ./qemu-nbd -r -f raw file -t &
$ ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
nbd://localhost:10809 -c quit
32145@1567207628.519883:nbd_receive_negotiate_size_flags Size is
1049088, export flags 0x48f
$ MY_HACK=1 ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
nbd://localhost:10809 -c quit
32156@1567207630.417815:nbd_receive_negotiate_size_flags Size is
1049088, export flags 0x40f
$ ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
nbd://localhost:10809 -c quit
32167@1567207635.202940:nbd_receive_negotiate_size_flags Size is
1049088, export flags 0x48f
The export flags change per client, so I _can_ store into
client->exp->nbdflags. Will do that for v2.
Meanwhile, this points out a missing feature in libnbd - for testing
purposes, it would be really nice to be able to purposefully cripple the
client to NOT request structured replies automatically (default enabled,
but the ability to turn it off is useful for interop testing, as in this
thread). I already recently added a --no-sr flag to nbdkit for a
similar reason (but that's creating a server which refuses to advertise,
where here I want a guest that refuses to ask). Guess I'll be adding a
patch for that, too :)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] nbd: Improve per-export flag handling in server
On 8/30/19 1:00 PM, Vladimir Sementsov-Ogievskiy wrote:
> 23.08.2019 17:37, Eric Blake wrote:
>> When creating a read-only image, we are still advertising support for
>> TRIM and WRITE_ZEROES to the client, even though the client should not
>> be issuing those commands. But seeing this requires looking across
>> multiple functions:
>>
>> @@ -458,10 +458,13 @@ static int nbd_negotiate_handle_export_name(NBDClient
>> *client,
>> return -EINVAL;
>> }
>>
>> -trace_nbd_negotiate_new_style_size_flags(client->exp->size,
>> - client->exp->nbdflags |
>> myflags);
>> +myflags = client->exp->nbdflags;
>> +if (client->structured_reply) {
>> +myflags |= NBD_FLAG_SEND_DF;
>> +}
>
>
> why we cant do just
> client->exp->nbdflags |= NBD_FLAG_SEND_DF ?
Because myflags is the runtime flags for _this_ client, while
client->exp->nbdflags are the base flags shared by _all_ clients. If
client A requests structured reply, but client B does not, then we don't
want to advertise DF to client B; but amending client->exp->nbdflags
would have that effect.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] nbd: Improve per-export flag handling in server
23.08.2019 17:37, Eric Blake wrote:
> When creating a read-only image, we are still advertising support for
> TRIM and WRITE_ZEROES to the client, even though the client should not
> be issuing those commands. But seeing this requires looking across
> multiple functions:
>
> All callers to nbd_export_new() passed a single flag based solely on
> whether the export allows writes. Later, we then pass a constant set
> of flags to nbd_negotiate_options() (namely, the set of flags which we
> always support, at least for writable images), which is then further
> dynamically modified based on client requests for structured options.
> Finally, when processing NBD_OPT_EXPORT_NAME or NBD_OPT_EXPORT_GO we
> bitwise-or the original caller's flag with the runtime set of flags
> we've built up over several functions.
>
> Let's refactor things to instead compute a baseline of flags as soon
> as possible, in nbd_export_new(), and changing the signature for the
> callers to pass in a simpler bool rather than having to figure out
> flags. We can then get rid of the 'myflags' parameter to various
> functions, and instead refer to client for everything we need (we
> still have to perform a bitwise-OR during NBD_OPT_EXPORT_NAME and
> NBD_OPT_EXPORT_GO, but it's easier to see what is being computed).
> This lets us quit advertising senseless flags for read-only images, as
> well as making the next patch for exposing FAST_ZERO support easier to
> write.
>
> Signed-off-by: Eric Blake
> ---
> include/block/nbd.h | 2 +-
> blockdev-nbd.c | 3 +--
> nbd/server.c| 62 +
> qemu-nbd.c | 6 ++---
> 4 files changed, 39 insertions(+), 34 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 991fd52a5134..2c87b42dfd48 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, bool shared,
> + const char *bitmap, bool readonly, 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 ecfa2ef0adb5..1cdffab4fce1 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -187,8 +187,7 @@ void qmp_nbd_server_add(const char *device, bool
> has_name, const char *name,
> writable = false;
> }
>
> -exp = nbd_export_new(bs, 0, len, name, NULL, bitmap,
> - writable ? 0 : NBD_FLAG_READ_ONLY, !writable,
> +exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable,
> !writable,
>NULL, false, on_eject_blk, errp);
> if (!exp) {
> return;
> diff --git a/nbd/server.c b/nbd/server.c
> index 0fb41c6c50ea..b5577828aa44 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -423,14 +423,14 @@ static void nbd_check_meta_export(NBDClient *client)
>
> /* Send a reply to NBD_OPT_EXPORT_NAME.
>* Return -errno on error, 0 on success. */
> -static int nbd_negotiate_handle_export_name(NBDClient *client,
> -uint16_t myflags, bool no_zeroes,
> +static int nbd_negotiate_handle_export_name(NBDClient *client, bool
> no_zeroes,
> Error **errp)
> {
> char name[NBD_MAX_NAME_SIZE + 1];
> char buf[NBD_REPLY_EXPORT_NAME_SIZE] = "";
> size_t len;
> int ret;
> +uint16_t myflags;
>
> /* Client sends:
> [20 .. xx] export name (length bytes)
> @@ -458,10 +458,13 @@ static int nbd_negotiate_handle_export_name(NBDClient
> *client,
> return -EINVAL;
> }
>
> -trace_nbd_negotiate_new_style_size_flags(client->exp->size,
> - client->exp->nbdflags |
> myflags);
> +myflags = client->exp->nbdflags;
> +if (client->structured_reply) {
> +myflags |= NBD_FLAG_SEND_DF;
> +}
why we cant do just
client->exp->nbdflags |= NBD_FLAG_SEND_DF ?
Or even do it earlier, when client->structured_reply becomes true?
> +trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags);
> stq_be_p(buf, client->exp->size);
> -stw_be_p(buf + 8, client->exp->nbdflags | myflags);
> +stw_be_p(buf + 8, myflags);
> len = no_zeroes ? 10 : sizeof(buf);
> ret = nbd_write(client->ioc, buf, len, errp);
> if (ret < 0) {
> @@ -526,8 +529,7 @@ static int nbd_reject_length(NBDClient *client, bool
> fatal, Error **errp)
> /* Handle NBD_OPT_INFO and NBD_OPT_GO.
>* Return -errno on error, 0 if ready for next option, and 1 to move
>* into
