On 5/22/25 9:44 PM, Eric Blake wrote:
> 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" <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(-)
>>>
>>
>> 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?
>
Doesn't the "max-" part suggest that there might be anything in between
[1..N]? When in reality it's either of {1, min(N, MAX_MULTI_CONN)}.
But you're right, my initial argument was that this mismatch shouldn't
go unnoticed as well. Although I agree that it's part of the expected
behavior and therefore might not deserve a warning. Maybe smth like
info_report() will do? We might even print it unconditionally, so that
there's always a way to tell the actual number of connections chosen.
Somewhat similar to what Richard pointed out at in nbdcopy.
Andrey
