On Thu, May 22, 2025 at 08:38:34PM +0300, Andrey Drobyshev wrote:
> On 4/28/25 9:46 PM, Eric Blake wrote:
> > From: "Richard W.M. Jones" <[email protected]>
> >
> > 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 <[email protected]>
> > [eblake: also expose it through QMP]
> > Signed-off-by: Eric Blake <[email protected]>
> > ---
> > qapi/block-core.json | 8 +++++++-
> > block/nbd.c | 24 ++++++++++++++++++++++++
> > 2 files changed, 31 insertions(+), 1 deletion(-)
> >
>
> Pardon my nitpicking, but it seems to me that the name "multi-conn"
> doesn't really suggest "number of simultaneous NBD client connections",
> especially when similarly named NBD_FLAG_CAN_MULTI_CONN_BIT represents
> binary logic: supported or not supported. Maybe smth like
> "multi_conns_num" would be better? Or anything else as long as it's
> clear it's an int, not bool.
Maybe 'max-multi-conn-clients', since it is something that even if the
user sets it to larger than 1 but the server doesn't advertise the
bit, then we silently restrict to one client.
> > @@ -1895,6 +1906,10 @@ static int nbd_process_options(BlockDriverState *bs,
> > QDict *options,
> >
> > s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0);
> > s->open_timeout = qemu_opt_get_number(opts, "open-timeout", 0);
> > + s->multi_conn = qemu_opt_get_number(opts, "multi-conn", 1);
> > + if (s->multi_conn > MAX_MULTI_CONN) {
> > + s->multi_conn = MAX_MULTI_CONN;
> > + }
> >
>
> I agree with Markus that this setting value different from what's been
> directly requested by user shouldn't go silent. Having some kind of
> warning at the very least would be nice.
Okay, I'll make sure to warn if it exceeds the compile-time max.
>
> > ret = 0;
> >
> > @@ -1949,6 +1964,15 @@ static int nbd_open(BlockDriverState *bs, QDict
> > *options, int flags,
> >
> > nbd_client_connection_enable_retry(s->conn);
> >
> > + /*
> > + * We set s->multi_conn in nbd_process_options above, but now that
> > + * we have connected if the server doesn't advertise that it is
> > + * safe for multi-conn, force it to 1.
> > + */
> > + if (!(s->info.flags & NBD_FLAG_CAN_MULTI_CONN)) {
> > + s->multi_conn = 1;
> > + }
>
> Same here.
Here, I disagree. But that's where better naming comes into play: if
there is 'max-' in the name, then the user should not be surprised if
they don't actually achieve the max (because the server lacked
support). On the other hand, I could see how you might want to know
if you have a mismatched setup ("I think the server SHOULD be
supporting multi-conn, so I request multiple clients, and I want to be
informed if my expectations were not met because then I know to go
reconfigure the server"). Thoughts?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
