"Richard W.M. Jones" <rjo...@redhat.com> writes: > On Tue, Apr 29, 2025 at 01:01:34PM +0200, Markus Armbruster wrote: >> "Richard W.M. Jones" <rjo...@redhat.com> writes: >> >> > On Tue, Apr 29, 2025 at 07:49:00AM +0200, Markus Armbruster wrote: >> >> Eric Blake <ebl...@redhat.com> writes: >> >> >> >> > From: "Richard W.M. Jones" <rjo...@redhat.com> >> >> > >> >> > Add multi-conn option to the NBD client. This commit just adds the >> >> > option, it is not functional. >> >> > >> >> > Setting this to a value > 1 permits multiple connections to the NBD >> >> > server; a typical value might be 4. The default is 1, meaning only a >> >> > single connection is made. If the NBD server does not advertise that >> >> > it is safe for multi-conn then this setting is forced to 1. >> >> > >> >> > Signed-off-by: Richard W.M. Jones <rjo...@redhat.com> >> >> > [eblake: also expose it through QMP] >> >> > Signed-off-by: Eric Blake <ebl...@redhat.com> >> >> > --- >> >> > qapi/block-core.json | 8 +++++++- >> >> > block/nbd.c | 24 ++++++++++++++++++++++++ >> >> > 2 files changed, 31 insertions(+), 1 deletion(-) >> >> > >> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json >> >> > index 7f70ec6d3cb..5c10824f35b 100644 >> >> > --- a/qapi/block-core.json >> >> > +++ b/qapi/block-core.json >> >> > @@ -4545,6 +4545,11 @@ >> >> > # until successful or until @open-timeout seconds have elapsed. >> >> > # Default 0 (Since 7.0) >> >> > # >> >> > +# @multi-conn: Request the number of parallel client connections to >> >> > make >> >> > +# to the server, up to 16. If the server does not advertise >> >> > support >> >> > +# for multiple connections, or if this value is 0 or 1, all traffic >> >> > +# is sent through a single connection. Default 1 (Since 10.1) >> >> > +# >> >> >> >> So we silently ignore @multi-conn when its value is (nonsensical) zero, >> >> and when the server doesn't let us honor the value. Hmm. Silently >> >> ignoring the user's wishes can result in confusion. Should we reject >> >> instead? >> > >> > We could certainly reject 0. It's also possible to reject the case >> > where multi-conn is not supported by the server, but is requested by >> > the client, but I feel that's a bit user-unfriendly. After all, >> > multi-conn isn't essential for it to work, it's needed if you want >> > best performance. (Maybe issue a warning in the code - below - where >> > we set multi-conn back to 1? I don't know what qemu thinks about >> > warnings.) >> >> QMP doesn't support warnings, so they go to stderr instead, where they >> may or may not be seen. >> >> When I instruct a program to do X, I prefer it to do exactly X, and fail >> when that's not possible. Correcting X behind my back may be friendly, >> until the day I spent quality time figuring out WTF is going on. >> >> Perhaps this one is a matter of documentation. As is, @multi-conn feels >> like "set the number of connections" to me, until I read the fine print, >> which contradicts it. We could perhaps phrase it as a limit instead: >> enable multiple connections and simultaneously limit their number. > > It is tricky. In nbdcopy we've preferred to go with "you suggest some > numbers and we'll pick something that works": > > https://gitlab.com/nbdkit/libnbd/-/blob/master/copy/main.c?ref_type=heads#L446-L493 > > but also we do provide a way for you to find out what was selected: > > https://gitlab.com/nbdkit/libnbd/-/blob/master/copy/main.c?ref_type=heads#L521 > > (I'm not claiming this is the best approach or suitable for everyone.) > > In the context of qemu that might suggest having separate > multi_conn_requested and multi_conn fields, where the latter can be > queried over QMP to find out what is actually going on. Could even > add multi_conn_max to allow MAX_MULTI_CONN constant to be read out.
You decide what to do with my feedback :) [...]
