| Message ID | 20250203222722.650694-6-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nbd: Allow debugging tuning of handshake limit | expand |
Eric Blake <eblake@redhat.com> writes: > Although defaulting the handshake limit to 10 seconds was a nice QoI > change to weed out intentionally slow clients, it can interfere with > integration testing done with manual NBD_OPT commands over 'nbdsh > --opt-mode'. Expose a QMP knob 'handshake-max-secs' to allow the user > to alter the timeout away from the default. > > The parameter name here intentionally matches the spelling of the > constant added in commit fb1c2aaa98, and not the command-line spelling > added in the previous patch for qemu-nbd; that's because in QMP, > longer names serve as good self-documentation, and unlike the command > line, machines don't have problems generating longer spellings. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > qapi/block-export.json | 10 ++++++++++ > include/block/nbd.h | 6 +++--- > block/monitor/block-hmp-cmds.c | 4 ++-- > blockdev-nbd.c | 26 ++++++++++++++++++-------- > 4 files changed, 33 insertions(+), 13 deletions(-) > > diff --git a/qapi/block-export.json b/qapi/block-export.json > index ce33fe378df..58ae6a5e1d7 100644 > --- a/qapi/block-export.json > +++ b/qapi/block-export.json > @@ -17,6 +17,10 @@ > # > # @addr: Address on which to listen. > # > +# @handshake-max-secs: Time limit, in seconds, at which a client that > +# has not completed the negotiation handshake will be disconnected, > +# or 0 for no limit (since 10.0; default: 10). > +# > # @tls-creds: ID of the TLS credentials object (since 2.6). > # > # @tls-authz: ID of the QAuthZ authorization object used to validate > @@ -34,6 +38,7 @@ > ## > { 'struct': 'NbdServerOptions', > 'data': { 'addr': 'SocketAddress', > + '*handshake-max-secs': 'uint32', > '*tls-creds': 'str', > '*tls-authz': 'str', > '*max-connections': 'uint32' } } Standard question on time: are we confident the granularity will suffice? On naming... We use "seconds" (StatsUnit in qapi/stats.json), and "sec" (SnapshotInfo in qapi/block-core.json), but not "secs". Do we care? > @@ -52,6 +57,10 @@ > # > # @addr: Address on which to listen. > # > +# @handshake-max-secs: Time limit, in seconds, at which a client that > +# has not completed the negotiation handshake will be disconnected, > +# or 0 for no limit (since 10.0; default: 10). > +# > # @tls-creds: ID of the TLS credentials object (since 2.6). > # > # @tls-authz: ID of the QAuthZ authorization object used to validate > @@ -72,6 +81,7 @@ > ## > { 'command': 'nbd-server-start', > 'data': { 'addr': 'SocketAddressLegacy', > + '*handshake-max-secs': 'uint32', > '*tls-creds': 'str', > '*tls-authz': 'str', > '*max-connections': 'uint32' }, [...]
On Wed, Feb 05, 2025 at 07:55:56AM +0100, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > > > Although defaulting the handshake limit to 10 seconds was a nice QoI > > change to weed out intentionally slow clients, it can interfere with > > integration testing done with manual NBD_OPT commands over 'nbdsh > > --opt-mode'. Expose a QMP knob 'handshake-max-secs' to allow the user > > to alter the timeout away from the default. > > > > The parameter name here intentionally matches the spelling of the > > constant added in commit fb1c2aaa98, and not the command-line spelling > > added in the previous patch for qemu-nbd; that's because in QMP, > > longer names serve as good self-documentation, and unlike the command > > line, machines don't have problems generating longer spellings. > > > > { 'struct': 'NbdServerOptions', > > 'data': { 'addr': 'SocketAddress', > > + '*handshake-max-secs': 'uint32', > > '*tls-creds': 'str', > > '*tls-authz': 'str', > > '*max-connections': 'uint32' } } > > Standard question on time: are we confident the granularity will > suffice? The default if this is unspecified is 10 seconds. Anything subsecond requires a fast negotiation between client and server; the more likely use of this parameter is setting it extremely large (or to 0 to disable) in order to allow lengthy gdb sessions without the timeout cutting things short. So I think seconds is okay. > > On naming... We use "seconds" (StatsUnit in qapi/stats.json), and "sec" > (SnapshotInfo in qapi/block-core.json), but not "secs". Do we care? Avoiding abbreviations and using 'seconds' is fine by me, especially since this won't be typed by many users but remains more of a tool for use in a debugging arsenel. I can respin with the longer name, but will wait for any other review comments first.
Eric Blake <eblake@redhat.com> writes: > On Wed, Feb 05, 2025 at 07:55:56AM +0100, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >> > Although defaulting the handshake limit to 10 seconds was a nice QoI >> > change to weed out intentionally slow clients, it can interfere with >> > integration testing done with manual NBD_OPT commands over 'nbdsh >> > --opt-mode'. Expose a QMP knob 'handshake-max-secs' to allow the user >> > to alter the timeout away from the default. >> > >> > The parameter name here intentionally matches the spelling of the >> > constant added in commit fb1c2aaa98, and not the command-line spelling >> > added in the previous patch for qemu-nbd; that's because in QMP, >> > longer names serve as good self-documentation, and unlike the command >> > line, machines don't have problems generating longer spellings. >> > > >> > { 'struct': 'NbdServerOptions', >> > 'data': { 'addr': 'SocketAddress', >> > + '*handshake-max-secs': 'uint32', >> > '*tls-creds': 'str', >> > '*tls-authz': 'str', >> > '*max-connections': 'uint32' } } >> >> Standard question on time: are we confident the granularity will >> suffice? > > The default if this is unspecified is 10 seconds. Anything subsecond > requires a fast negotiation between client and server; the more likely > use of this parameter is setting it extremely large (or to 0 to > disable) in order to allow lengthy gdb sessions without the timeout > cutting things short. So I think seconds is okay. ACK >> On naming... We use "seconds" (StatsUnit in qapi/stats.json), and "sec" >> (SnapshotInfo in qapi/block-core.json), but not "secs". Do we care? > > Avoiding abbreviations and using 'seconds' is fine by me, especially > since this won't be typed by many users but remains more of a tool for > use in a debugging arsenel. I can respin with the longer name, but > will wait for any other review comments first. I like "seconds". Thanks!
On 04.02.25 01:26, Eric Blake wrote: > Although defaulting the handshake limit to 10 seconds was a nice QoI > change to weed out intentionally slow clients, it can interfere with > integration testing done with manual NBD_OPT commands over 'nbdsh > --opt-mode'. Expose a QMP knob 'handshake-max-secs' to allow the user > to alter the timeout away from the default. > > The parameter name here intentionally matches the spelling of the > constant added in commit fb1c2aaa98, and not the command-line spelling > added in the previous patch for qemu-nbd; that's because in QMP, > longer names serve as good self-documentation, and unlike the command > line, machines don't have problems generating longer spellings. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> (renaming to -seconds is, of course, OK) > --- > qapi/block-export.json | 10 ++++++++++ > include/block/nbd.h | 6 +++--- [..] > @@ -52,6 +57,10 @@ > # > # @addr: Address on which to listen. > # > +# @handshake-max-secs: Time limit, in seconds, at which a client that > +# has not completed the negotiation handshake will be disconnected, > +# or 0 for no limit (since 10.0; default: 10). > +# Hmm. [not about the series], shouldn't we finally deprecate older interface? > # @tls-creds: ID of the TLS credentials object (since 2.6). > # > # @tls-authz: ID of the QAuthZ authorization object used to validate > @@ -72,6 +81,7 @@ > ## > { 'command': 'nbd-server-start', > 'data': { 'addr': 'SocketAddressLegacy', [..]
On Thu, Feb 06, 2025 at 10:20:09AM +0300, Vladimir Sementsov-Ogievskiy wrote: > > --- > > qapi/block-export.json | 10 ++++++++++ > > include/block/nbd.h | 6 +++--- > > [..] > > > @@ -52,6 +57,10 @@ > > # > > # @addr: Address on which to listen. > > # > > +# @handshake-max-secs: Time limit, in seconds, at which a client that > > +# has not completed the negotiation handshake will be disconnected, > > +# or 0 for no limit (since 10.0; default: 10). > > +# > > Hmm. [not about the series], shouldn't we finally deprecate older interface? By older interface, you are asking about the QMP command 'nbd-server-start' as compared to struct NbdServerOptions. But the struct is not directly present in any QMP commands; rather, it only appears to be used by qemu-storage-daemon as one of its command line options that needs to set up an NBD server with a JSON-like syntax that has less nesting than QMP nbd-server-start. blockdev-nbd.c has two functions [nbd_server_start_options(NbdServerOPtions *arg...) and qmp_nbd_server_start(args...)] that both unpack their slightly different forms and pass them as parameters to nbd_server_start() that is then agnostic to whether the older QMP command or newer q-s-d CLI option was specified. It looks like libvirt is still using QMP nbd-server-start. If we were to start the deprecation process for qemu 10.0, what would the new command look like? What would everyone be required to use by qemu 10.2?
On 11.02.25 00:46, Eric Blake wrote: > On Thu, Feb 06, 2025 at 10:20:09AM +0300, Vladimir Sementsov-Ogievskiy wrote: >>> --- >>> qapi/block-export.json | 10 ++++++++++ >>> include/block/nbd.h | 6 +++--- >> >> [..] >> >>> @@ -52,6 +57,10 @@ >>> # >>> # @addr: Address on which to listen. >>> # >>> +# @handshake-max-secs: Time limit, in seconds, at which a client that >>> +# has not completed the negotiation handshake will be disconnected, >>> +# or 0 for no limit (since 10.0; default: 10). >>> +# >> >> Hmm. [not about the series], shouldn't we finally deprecate older interface? > > By older interface, you are asking about the QMP command > 'nbd-server-start' as compared to struct NbdServerOptions. But the > struct is not directly present in any QMP commands; rather, it only > appears to be used by qemu-storage-daemon as one of its command line > options that needs to set up an NBD server with a JSON-like syntax > that has less nesting than QMP nbd-server-start. blockdev-nbd.c has > two functions [nbd_server_start_options(NbdServerOPtions *arg...) and > qmp_nbd_server_start(args...)] that both unpack their slightly > different forms and pass them as parameters to nbd_server_start() that > is then agnostic to whether the older QMP command or newer q-s-d CLI > option was specified. > > It looks like libvirt is still using QMP nbd-server-start. If we were > to start the deprecation process for qemu 10.0, what would the new > command look like? What would everyone be required to use by qemu > 10.2? > Oh you are right, I was inattentive. So, probably thing to be deprecated is actually SocketAddressLegacy, which is the only difference. And why just not move common part of the structures to common base? I'll send a patch with a try.
diff --git a/qapi/block-export.json b/qapi/block-export.json index ce33fe378df..58ae6a5e1d7 100644 --- a/qapi/block-export.json +++ b/qapi/block-export.json @@ -17,6 +17,10 @@ # # @addr: Address on which to listen. # +# @handshake-max-secs: Time limit, in seconds, at which a client that +# has not completed the negotiation handshake will be disconnected, +# or 0 for no limit (since 10.0; default: 10). +# # @tls-creds: ID of the TLS credentials object (since 2.6). # # @tls-authz: ID of the QAuthZ authorization object used to validate @@ -34,6 +38,7 @@ ## { 'struct': 'NbdServerOptions', 'data': { 'addr': 'SocketAddress', + '*handshake-max-secs': 'uint32', '*tls-creds': 'str', '*tls-authz': 'str', '*max-connections': 'uint32' } } @@ -52,6 +57,10 @@ # # @addr: Address on which to listen. # +# @handshake-max-secs: Time limit, in seconds, at which a client that +# has not completed the negotiation handshake will be disconnected, +# or 0 for no limit (since 10.0; default: 10). +# # @tls-creds: ID of the TLS credentials object (since 2.6). # # @tls-authz: ID of the QAuthZ authorization object used to validate @@ -72,6 +81,7 @@ ## { 'command': 'nbd-server-start', 'data': { 'addr': 'SocketAddressLegacy', + '*handshake-max-secs': 'uint32', '*tls-creds': 'str', '*tls-authz': 'str', '*max-connections': 'uint32' }, diff --git a/include/block/nbd.h b/include/block/nbd.h index d4f8b21aecc..92987c76fd6 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -428,9 +428,9 @@ void nbd_client_put(NBDClient *client); void nbd_server_is_qemu_nbd(int max_connections); bool nbd_server_is_running(void); int nbd_server_max_connections(void); -void nbd_server_start(SocketAddress *addr, const char *tls_creds, - const char *tls_authz, uint32_t max_connections, - Error **errp); +void nbd_server_start(SocketAddress *addr, uint32_t handshake_max_secs, + const char *tls_creds, const char *tls_authz, + uint32_t max_connections, Error **errp); void nbd_server_start_options(NbdServerOptions *arg, Error **errp); /* nbd_read diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c index 1d312513fc4..0cfcbfe7c21 100644 --- a/block/monitor/block-hmp-cmds.c +++ b/block/monitor/block-hmp-cmds.c @@ -402,8 +402,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict) goto exit; } - nbd_server_start(addr, NULL, NULL, NBD_DEFAULT_MAX_CONNECTIONS, - &local_err); + nbd_server_start(addr, NBD_DEFAULT_HANDSHAKE_MAX_SECS, NULL, NULL, + NBD_DEFAULT_MAX_CONNECTIONS, &local_err); qapi_free_SocketAddress(addr); if (local_err != NULL) { goto exit; diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 9e61fbaf2b2..e9f53e83d48 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -28,6 +28,7 @@ typedef struct NBDConn { typedef struct NBDServerData { QIONetListener *listener; + uint32_t handshake_max_secs; QCryptoTLSCreds *tlscreds; char *tlsauthz; uint32_t max_connections; @@ -84,8 +85,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, nbd_update_server_watch(nbd_server); qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server"); - /* TODO - expose handshake timeout as QMP option */ - nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_MAX_SECS, + nbd_client_new(cioc, nbd_server->handshake_max_secs, nbd_server->tlscreds, nbd_server->tlsauthz, nbd_blockdev_client_closed, conn); } @@ -162,9 +162,9 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp) } -void nbd_server_start(SocketAddress *addr, const char *tls_creds, - const char *tls_authz, uint32_t max_connections, - Error **errp) +void nbd_server_start(SocketAddress *addr, uint32_t handshake_max_secs, + const char *tls_creds, const char *tls_authz, + uint32_t max_connections, Error **errp) { if (nbd_server) { error_setg(errp, "NBD server already running"); @@ -173,6 +173,7 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds, nbd_server = g_new0(NBDServerData, 1); nbd_server->max_connections = max_connections; + nbd_server->handshake_max_secs = handshake_max_secs; nbd_server->listener = qio_net_listener_new(); qio_net_listener_set_name(nbd_server->listener, @@ -210,12 +211,17 @@ void nbd_server_start_options(NbdServerOptions *arg, Error **errp) if (!arg->has_max_connections) { arg->max_connections = NBD_DEFAULT_MAX_CONNECTIONS; } + if (!arg->has_handshake_max_secs) { + arg->handshake_max_secs = NBD_DEFAULT_HANDSHAKE_MAX_SECS; + } - nbd_server_start(arg->addr, arg->tls_creds, arg->tls_authz, - arg->max_connections, errp); + nbd_server_start(arg->addr, arg->handshake_max_secs, arg->tls_creds, + arg->tls_authz, arg->max_connections, errp); } void qmp_nbd_server_start(SocketAddressLegacy *addr, + bool has_handshake_max_secs, + uint32_t handshake_max_secs, const char *tls_creds, const char *tls_authz, bool has_max_connections, uint32_t max_connections, @@ -226,8 +232,12 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr, if (!has_max_connections) { max_connections = NBD_DEFAULT_MAX_CONNECTIONS; } + if (!has_handshake_max_secs) { + handshake_max_secs = NBD_DEFAULT_HANDSHAKE_MAX_SECS; + } - nbd_server_start(addr_flat, tls_creds, tls_authz, max_connections, errp); + nbd_server_start(addr_flat, handshake_max_secs, tls_creds, tls_authz, + max_connections, errp); qapi_free_SocketAddress(addr_flat); }
Although defaulting the handshake limit to 10 seconds was a nice QoI change to weed out intentionally slow clients, it can interfere with integration testing done with manual NBD_OPT commands over 'nbdsh --opt-mode'. Expose a QMP knob 'handshake-max-secs' to allow the user to alter the timeout away from the default. The parameter name here intentionally matches the spelling of the constant added in commit fb1c2aaa98, and not the command-line spelling added in the previous patch for qemu-nbd; that's because in QMP, longer names serve as good self-documentation, and unlike the command line, machines don't have problems generating longer spellings. Signed-off-by: Eric Blake <eblake@redhat.com> --- qapi/block-export.json | 10 ++++++++++ include/block/nbd.h | 6 +++--- block/monitor/block-hmp-cmds.c | 4 ++-- blockdev-nbd.c | 26 ++++++++++++++++++-------- 4 files changed, 33 insertions(+), 13 deletions(-)