From: "Richard W.M. Jones" <rjones@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 <rjones@redhat.com>
[eblake: also expose it through QMP]
Signed-off-by: Eric Blake <eblake@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)
+#
 # Features:
 #
 # @unstable: Member @x-dirty-bitmap is experimental.
@@ -4558,7 +4563,8 @@
             '*tls-hostname': 'str',
             '*x-dirty-bitmap': { 'type': 'str', 'features': [ 'unstable' ] },
             '*reconnect-delay': 'uint32',
-            '*open-timeout': 'uint32' } }
+            '*open-timeout': 'uint32',
+            '*multi-conn': 'uint32' } }

 ##
 # @BlockdevOptionsRaw:
diff --git a/block/nbd.c b/block/nbd.c
index d5a2b21c6d1..5eb00e360af 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -48,6 +48,7 @@

 #define EN_OPTSTR ":exportname="
 #define MAX_NBD_REQUESTS    16
+#define MAX_MULTI_CONN      16

 #define COOKIE_TO_INDEX(cookie) ((cookie) - 1)
 #define INDEX_TO_COOKIE(index)  ((index) + 1)
@@ -97,6 +98,7 @@ typedef struct BDRVNBDState {
     /* Connection parameters */
     uint32_t reconnect_delay;
     uint32_t open_timeout;
+    uint32_t multi_conn;
     SocketAddress *saddr;
     char *export;
     char *tlscredsid;
@@ -1840,6 +1842,15 @@ static QemuOptsList nbd_runtime_opts = {
                     "attempts until successful or until @open-timeout seconds "
                     "have elapsed. Default 0",
         },
+        {
+            .name = "multi-conn",
+            .type = QEMU_OPT_NUMBER,
+            .help = "If > 1 permit up to this number of connections to the "
+                    "server. The server must also advertise multi-conn "
+                    "support.  If <= 1, only a single connection is made "
+                    "to the server even if the server advertises multi-conn. "
+                    "Default 1",
+        },
         { /* end of list */ }
     },
 };
@@ -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;
+    }

     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;
+    }
+
     return 0;

 fail:
-- 
2.49.0

On 4/28/25 9:46 PM, Eric Blake wrote:
> From: "Richard W.M. Jones" <rjones@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 <rjones@redhat.com>
> [eblake: also expose it through QMP]
> Signed-off-by: Eric Blake <eblake@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.

> 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)
> +#
>  # Features:
>  #
>  # @unstable: Member @x-dirty-bitmap is experimental.
> @@ -4558,7 +4563,8 @@
>              '*tls-hostname': 'str',
>              '*x-dirty-bitmap': { 'type': 'str', 'features': [ 'unstable' ] },
>              '*reconnect-delay': 'uint32',
> -            '*open-timeout': 'uint32' } }
> +            '*open-timeout': 'uint32',
> +            '*multi-conn': 'uint32' } }
> 
>  ##
>  # @BlockdevOptionsRaw:
> diff --git a/block/nbd.c b/block/nbd.c
> index d5a2b21c6d1..5eb00e360af 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -48,6 +48,7 @@
> 
>  #define EN_OPTSTR ":exportname="
>  #define MAX_NBD_REQUESTS    16
> +#define MAX_MULTI_CONN      16
> 
>  #define COOKIE_TO_INDEX(cookie) ((cookie) - 1)
>  #define INDEX_TO_COOKIE(index)  ((index) + 1)
> @@ -97,6 +98,7 @@ typedef struct BDRVNBDState {
>      /* Connection parameters */
>      uint32_t reconnect_delay;
>      uint32_t open_timeout;
> +    uint32_t multi_conn;
>      SocketAddress *saddr;
>      char *export;
>      char *tlscredsid;
> @@ -1840,6 +1842,15 @@ static QemuOptsList nbd_runtime_opts = {
>                      "attempts until successful or until @open-timeout seconds "
>                      "have elapsed. Default 0",
>          },
> +        {
> +            .name = "multi-conn",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "If > 1 permit up to this number of connections to the "
> +                    "server. The server must also advertise multi-conn "
> +                    "support.  If <= 1, only a single connection is made "
> +                    "to the server even if the server advertises multi-conn. "
> +                    "Default 1",
> +        },
>          { /* end of list */ }
>      },
>  };
> @@ -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.

>      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.

> +
>      return 0;
> 
>  fail:

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" <rjones@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 <rjones@redhat.com>
> > [eblake: also expose it through QMP]
> > Signed-off-by: Eric Blake <eblake@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?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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" <rjones@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 <rjones@redhat.com>
>>> [eblake: also expose it through QMP]
>>> Signed-off-by: Eric Blake <eblake@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

On Fri, May 23, 2025 at 02:03:14PM +0300, Andrey Drobyshev wrote:
> >> 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.

Rather, refuse the QMP command with an error.

> > 
> >>
> >>>      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.

Warnings in QMP are difficult.  It's easy to fail a command, but hard
to print non-fatal messages.  However, I'm not opposed to having a way
to use a QMP query-* command as a followup for the user who is curious
on how many connections a given block export is using (either we can
already query all existing block exports, or that's something that I
should add independent of the new multi-conn setting).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Eric Blake <eblake@redhat.com> writes:

> From: "Richard W.M. Jones" <rjones@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 <rjones@redhat.com>
> [eblake: also expose it through QMP]
> Signed-off-by: Eric Blake <eblake@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?

>  # Features:
>  #
>  # @unstable: Member @x-dirty-bitmap is experimental.
> @@ -4558,7 +4563,8 @@
>              '*tls-hostname': 'str',
>              '*x-dirty-bitmap': { 'type': 'str', 'features': [ 'unstable' ] },
>              '*reconnect-delay': 'uint32',
> -            '*open-timeout': 'uint32' } }
> +            '*open-timeout': 'uint32',
> +            '*multi-conn': 'uint32' } }
>
>  ##
>  # @BlockdevOptionsRaw:
> diff --git a/block/nbd.c b/block/nbd.c
> index d5a2b21c6d1..5eb00e360af 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -48,6 +48,7 @@
>
>  #define EN_OPTSTR ":exportname="
>  #define MAX_NBD_REQUESTS    16
> +#define MAX_MULTI_CONN      16

Out of curiosity: where does this value come from?

>
>  #define COOKIE_TO_INDEX(cookie) ((cookie) - 1)
>  #define INDEX_TO_COOKIE(index)  ((index) + 1)
> @@ -97,6 +98,7 @@ typedef struct BDRVNBDState {
>      /* Connection parameters */
>      uint32_t reconnect_delay;
>      uint32_t open_timeout;
> +    uint32_t multi_conn;
>      SocketAddress *saddr;
>      char *export;
>      char *tlscredsid;
> @@ -1840,6 +1842,15 @@ static QemuOptsList nbd_runtime_opts = {
>                      "attempts until successful or until @open-timeout seconds "
>                      "have elapsed. Default 0",
>          },
> +        {
> +            .name = "multi-conn",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "If > 1 permit up to this number of connections to the "
> +                    "server. The server must also advertise multi-conn "
> +                    "support.  If <= 1, only a single connection is made "
> +                    "to the server even if the server advertises multi-conn. "
> +                    "Default 1",

This text implies the requested value is silently limited to the value
provided by the server, unlike the doc comment above.  Although the
"must" in "the sever must" could also be understood as "error when it
doesn't".

> +        },
>          { /* end of list */ }
>      },
>  };
> @@ -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;
> +    }

We silently cap the user's requested number to 16.  Not clear from QAPI
schema doc comment; the "up to 16" there suggests more is an error.
Should we error out instead?

>
>      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;
> +    }
> +
>      return 0;
>
>  fail:

On Tue, Apr 29, 2025 at 07:49:00AM +0200, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > From: "Richard W.M. Jones" <rjones@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 <rjones@redhat.com>
> > [eblake: also expose it through QMP]
> > Signed-off-by: Eric Blake <eblake@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.)

> >  # Features:
> >  #
> >  # @unstable: Member @x-dirty-bitmap is experimental.
> > @@ -4558,7 +4563,8 @@
> >              '*tls-hostname': 'str',
> >              '*x-dirty-bitmap': { 'type': 'str', 'features': [ 'unstable' ] },
> >              '*reconnect-delay': 'uint32',
> > -            '*open-timeout': 'uint32' } }
> > +            '*open-timeout': 'uint32',
> > +            '*multi-conn': 'uint32' } }
> >
> >  ##
> >  # @BlockdevOptionsRaw:
> > diff --git a/block/nbd.c b/block/nbd.c
> > index d5a2b21c6d1..5eb00e360af 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -48,6 +48,7 @@
> >
> >  #define EN_OPTSTR ":exportname="
> >  #define MAX_NBD_REQUESTS    16
> > +#define MAX_MULTI_CONN      16
> 
> Out of curiosity: where does this value come from?

So I should note first this is a maximum, not a recommendation.

nbdcopy defaults to 4, which was derived from testing on a high end
(for 2024) AMD machine.  Above 4 performance doesn't increase any
further on that machine.  It's going to very much depend on how many
cores you have spare, how many TCP connections you want to open, and
how effectively the client and server handle parallelism.

And imponderables like one we hit in virt-v2v: If accessing a VMware
server, the VMware server actually slows down as you add more
connections, even though it should theoretically support multi-conn.
We ended up forcing multi-conn to 1 in this case.  You can't know this
in advance from the client side.

> >
> >  #define COOKIE_TO_INDEX(cookie) ((cookie) - 1)
> >  #define INDEX_TO_COOKIE(index)  ((index) + 1)
> > @@ -97,6 +98,7 @@ typedef struct BDRVNBDState {
> >      /* Connection parameters */
> >      uint32_t reconnect_delay;
> >      uint32_t open_timeout;
> > +    uint32_t multi_conn;
> >      SocketAddress *saddr;
> >      char *export;
> >      char *tlscredsid;
> > @@ -1840,6 +1842,15 @@ static QemuOptsList nbd_runtime_opts = {
> >                      "attempts until successful or until @open-timeout seconds "
> >                      "have elapsed. Default 0",
> >          },
> > +        {
> > +            .name = "multi-conn",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "If > 1 permit up to this number of connections to the "
> > +                    "server. The server must also advertise multi-conn "
> > +                    "support.  If <= 1, only a single connection is made "
> > +                    "to the server even if the server advertises multi-conn. "
> > +                    "Default 1",
> 
> This text implies the requested value is silently limited to the value
> provided by the server, unlike the doc comment above.  Although the
> "must" in "the sever must" could also be understood as "error when it
> doesn't".

I'll just note that multi-conn is a boolean flag advertised by the
server.  Servers don't advertise any preferred number of connections.
I don't know how to improve the text.

> > +        },
> >          { /* end of list */ }
> >      },
> >  };
> > @@ -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;
> > +    }
> 
> We silently cap the user's requested number to 16.  Not clear from QAPI
> schema doc comment; the "up to 16" there suggests more is an error.
> Should we error out instead?
> 
> >
> >      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;
> > +    }
> > +
> >      return 0;
> >
> >  fail:

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

"Richard W.M. Jones" <rjones@redhat.com> writes:

> On Tue, Apr 29, 2025 at 07:49:00AM +0200, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > From: "Richard W.M. Jones" <rjones@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 <rjones@redhat.com>
>> > [eblake: also expose it through QMP]
>> > Signed-off-by: Eric Blake <eblake@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.

>> >  # Features:
>> >  #
>> >  # @unstable: Member @x-dirty-bitmap is experimental.
>> > @@ -4558,7 +4563,8 @@
>> >              '*tls-hostname': 'str',
>> >              '*x-dirty-bitmap': { 'type': 'str', 'features': [ 'unstable' ] },
>> >              '*reconnect-delay': 'uint32',
>> > -            '*open-timeout': 'uint32' } }
>> > +            '*open-timeout': 'uint32',
>> > +            '*multi-conn': 'uint32' } }
>> >
>> >  ##
>> >  # @BlockdevOptionsRaw:
>> > diff --git a/block/nbd.c b/block/nbd.c
>> > index d5a2b21c6d1..5eb00e360af 100644
>> > --- a/block/nbd.c
>> > +++ b/block/nbd.c
>> > @@ -48,6 +48,7 @@
>> >
>> >  #define EN_OPTSTR ":exportname="
>> >  #define MAX_NBD_REQUESTS    16
>> > +#define MAX_MULTI_CONN      16
>> 
>> Out of curiosity: where does this value come from?
>
> So I should note first this is a maximum, not a recommendation.

Is it the arbitrarily chosen maximum for QEMU, or is it the well-known
maximum for NBD, or is it something else?

> nbdcopy defaults to 4, which was derived from testing on a high end
> (for 2024) AMD machine.  Above 4 performance doesn't increase any
> further on that machine.  It's going to very much depend on how many
> cores you have spare, how many TCP connections you want to open, and
> how effectively the client and server handle parallelism.
>
> And imponderables like one we hit in virt-v2v: If accessing a VMware
> server, the VMware server actually slows down as you add more
> connections, even though it should theoretically support multi-conn.
> We ended up forcing multi-conn to 1 in this case.  You can't know this
> in advance from the client side.
>
>> >
>> >  #define COOKIE_TO_INDEX(cookie) ((cookie) - 1)
>> >  #define INDEX_TO_COOKIE(index)  ((index) + 1)
>> > @@ -97,6 +98,7 @@ typedef struct BDRVNBDState {
>> >      /* Connection parameters */
>> >      uint32_t reconnect_delay;
>> >      uint32_t open_timeout;
>> > +    uint32_t multi_conn;
>> >      SocketAddress *saddr;
>> >      char *export;
>> >      char *tlscredsid;
>> > @@ -1840,6 +1842,15 @@ static QemuOptsList nbd_runtime_opts = {
>> >                      "attempts until successful or until @open-timeout seconds "
>> >                      "have elapsed. Default 0",
>> >          },
>> > +        {
>> > +            .name = "multi-conn",
>> > +            .type = QEMU_OPT_NUMBER,
>> > +            .help = "If > 1 permit up to this number of connections to the "
>> > +                    "server. The server must also advertise multi-conn "
>> > +                    "support.  If <= 1, only a single connection is made "
>> > +                    "to the server even if the server advertises multi-conn. "
>> > +                    "Default 1",
>> 
>> This text implies the requested value is silently limited to the value
>> provided by the server, unlike the doc comment above.  Although the
>> "must" in "the sever must" could also be understood as "error when it
>> doesn't".
>
> I'll just note that multi-conn is a boolean flag advertised by the
> server.  Servers don't advertise any preferred number of connections.
> I don't know how to improve the text.

Let's get the QAPI schema doc comment right before we worry about this
one.

>> > +        },
>> >          { /* end of list */ }
>> >      },
>> >  };
>> > @@ -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;
>> > +    }
>> 
>> We silently cap the user's requested number to 16.  Not clear from QAPI
>> schema doc comment; the "up to 16" there suggests more is an error.
>> Should we error out instead?
>> 
>> >
>> >      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;
>> > +    }
>> > +
>> >      return 0;
>> >
>> >  fail:
>
> Rich.

On Tue, Apr 29, 2025 at 01:01:34PM +0200, Markus Armbruster wrote:
> "Richard W.M. Jones" <rjones@redhat.com> writes:
> 
> > On Tue, Apr 29, 2025 at 07:49:00AM +0200, Markus Armbruster wrote:
> >> Eric Blake <eblake@redhat.com> writes:
> >> 
> >> > From: "Richard W.M. Jones" <rjones@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 <rjones@redhat.com>
> >> > [eblake: also expose it through QMP]
> >> > Signed-off-by: Eric Blake <eblake@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.

> >> >  # Features:
> >> >  #
> >> >  # @unstable: Member @x-dirty-bitmap is experimental.
> >> > @@ -4558,7 +4563,8 @@
> >> >              '*tls-hostname': 'str',
> >> >              '*x-dirty-bitmap': { 'type': 'str', 'features': [ 'unstable' ] },
> >> >              '*reconnect-delay': 'uint32',
> >> > -            '*open-timeout': 'uint32' } }
> >> > +            '*open-timeout': 'uint32',
> >> > +            '*multi-conn': 'uint32' } }
> >> >
> >> >  ##
> >> >  # @BlockdevOptionsRaw:
> >> > diff --git a/block/nbd.c b/block/nbd.c
> >> > index d5a2b21c6d1..5eb00e360af 100644
> >> > --- a/block/nbd.c
> >> > +++ b/block/nbd.c
> >> > @@ -48,6 +48,7 @@
> >> >
> >> >  #define EN_OPTSTR ":exportname="
> >> >  #define MAX_NBD_REQUESTS    16
> >> > +#define MAX_MULTI_CONN      16
> >> 
> >> Out of curiosity: where does this value come from?
> >
> > So I should note first this is a maximum, not a recommendation.
> 
> Is it the arbitrarily chosen maximum for QEMU, or is it the well-known
> maximum for NBD, or is it something else?

I don't recall exactly, but it was probably an ass-pulled number.

> > nbdcopy defaults to 4, which was derived from testing on a high end
> > (for 2024) AMD machine.  Above 4 performance doesn't increase any
> > further on that machine.  It's going to very much depend on how many
> > cores you have spare, how many TCP connections you want to open, and
> > how effectively the client and server handle parallelism.
> >
> > And imponderables like one we hit in virt-v2v: If accessing a VMware
> > server, the VMware server actually slows down as you add more
> > connections, even though it should theoretically support multi-conn.
> > We ended up forcing multi-conn to 1 in this case.  You can't know this
> > in advance from the client side.
> >
> >> >
> >> >  #define COOKIE_TO_INDEX(cookie) ((cookie) - 1)
> >> >  #define INDEX_TO_COOKIE(index)  ((index) + 1)
> >> > @@ -97,6 +98,7 @@ typedef struct BDRVNBDState {
> >> >      /* Connection parameters */
> >> >      uint32_t reconnect_delay;
> >> >      uint32_t open_timeout;
> >> > +    uint32_t multi_conn;
> >> >      SocketAddress *saddr;
> >> >      char *export;
> >> >      char *tlscredsid;
> >> > @@ -1840,6 +1842,15 @@ static QemuOptsList nbd_runtime_opts = {
> >> >                      "attempts until successful or until @open-timeout seconds "
> >> >                      "have elapsed. Default 0",
> >> >          },
> >> > +        {
> >> > +            .name = "multi-conn",
> >> > +            .type = QEMU_OPT_NUMBER,
> >> > +            .help = "If > 1 permit up to this number of connections to the "
> >> > +                    "server. The server must also advertise multi-conn "
> >> > +                    "support.  If <= 1, only a single connection is made "
> >> > +                    "to the server even if the server advertises multi-conn. "
> >> > +                    "Default 1",
> >> 
> >> This text implies the requested value is silently limited to the value
> >> provided by the server, unlike the doc comment above.  Although the
> >> "must" in "the sever must" could also be understood as "error when it
> >> doesn't".
> >
> > I'll just note that multi-conn is a boolean flag advertised by the
> > server.  Servers don't advertise any preferred number of connections.
> > I don't know how to improve the text.
> 
> Let's get the QAPI schema doc comment right before we worry about this
> one.
> 
> >> > +        },
> >> >          { /* end of list */ }
> >> >      },
> >> >  };
> >> > @@ -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;
> >> > +    }
> >> 
> >> We silently cap the user's requested number to 16.  Not clear from QAPI
> >> schema doc comment; the "up to 16" there suggests more is an error.
> >> Should we error out instead?
> >> 
> >> >
> >> >      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;
> >> > +    }
> >> > +
> >> >      return 0;
> >> >
> >> >  fail:
> >
> > Rich.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

"Richard W.M. Jones" <rjones@redhat.com> writes:

> On Tue, Apr 29, 2025 at 01:01:34PM +0200, Markus Armbruster wrote:
>> "Richard W.M. Jones" <rjones@redhat.com> writes:
>> 
>> > On Tue, Apr 29, 2025 at 07:49:00AM +0200, Markus Armbruster wrote:
>> >> Eric Blake <eblake@redhat.com> writes:
>> >> 
>> >> > From: "Richard W.M. Jones" <rjones@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 <rjones@redhat.com>
>> >> > [eblake: also expose it through QMP]
>> >> > Signed-off-by: Eric Blake <eblake@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 :)

[...]

On Tue, Apr 29, 2025 at 01:31:44PM +0200, Markus Armbruster wrote:
> > 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 :)

I've got a local patch that adds the ability for
query-named-block-nodes (and query-block) to output image-specific
information for NBD that includes how many connections are actually in
use.  But now I've got a QMP question:

My patch, as written, makes the output look like this:

"format-specific": {"mode": "extended", "type": "nbd", "connections": 1}},

by changing block-core.json like this (partial patch shown):

@@ -208,10 +223,12 @@
 #
 # @file: Since 8.0
 #
+# @nbd: Since 10.1
+#
 # Since: 1.7
 ##
 { 'enum': 'ImageInfoSpecificKind',
-  'data': [ 'qcow2', 'vmdk', 'luks', 'rbd', 'file' ] }
+  'data': [ 'qcow2', 'vmdk', 'luks', 'rbd', 'file', 'nbd' ] }

 ##
 # @ImageInfoSpecificQCow2Wrapper:
@@ -284,7 +301,8 @@
       'luks': 'ImageInfoSpecificLUKSWrapper',
       'rbd': 'ImageInfoSpecificRbdWrapper',
-      'file': 'ImageInfoSpecificFileWrapper'
+      'file': 'ImageInfoSpecificFileWrapper',
+      'nbd': 'ImageInfoSpecificNbd'
   } }

But that is different from all of the other image modes, where the
output looks more like:

"format-specific": {"type": "rbd", "data": {"encryption-format":"..."}}},

note the extra layer of nesting, due to historical reasons of being
added in a time when the QMP generator was not as nice on supporting
flatter union-style coding.

Must I create an ImageInfoSpecificNbdWrapper type, with the sole
purpose of having the same (pointless, IMO) "data":{} wrapper as all
the other branches of the union type, or am I okay with my addition
using the flatter style?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Eric Blake <eblake@redhat.com> writes:

> On Tue, Apr 29, 2025 at 01:31:44PM +0200, Markus Armbruster wrote:
>> > 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 :)
>
> I've got a local patch that adds the ability for
> query-named-block-nodes (and query-block) to output image-specific
> information for NBD that includes how many connections are actually in
> use.  But now I've got a QMP question:
>
> My patch, as written, makes the output look like this:
>
> "format-specific": {"mode": "extended", "type": "nbd", "connections": 1}},
>
> by changing block-core.json like this (partial patch shown):
>
> @@ -208,10 +223,12 @@
>  #
>  # @file: Since 8.0
>  #
> +# @nbd: Since 10.1
> +#
>  # Since: 1.7
>  ##
>  { 'enum': 'ImageInfoSpecificKind',
> -  'data': [ 'qcow2', 'vmdk', 'luks', 'rbd', 'file' ] }
> +  'data': [ 'qcow2', 'vmdk', 'luks', 'rbd', 'file', 'nbd' ] }
>
>  ##
>  # @ImageInfoSpecificQCow2Wrapper:
> @@ -284,7 +301,8 @@
>        'luks': 'ImageInfoSpecificLUKSWrapper',
>        'rbd': 'ImageInfoSpecificRbdWrapper',
> -      'file': 'ImageInfoSpecificFileWrapper'
> +      'file': 'ImageInfoSpecificFileWrapper',
> +      'nbd': 'ImageInfoSpecificNbd'
>    } }
>
> But that is different from all of the other image modes, where the
> output looks more like:
>
> "format-specific": {"type": "rbd", "data": {"encryption-format":"..."}}},
>
> note the extra layer of nesting, due to historical reasons of being
> added in a time when the QMP generator was not as nice on supporting
> flatter union-style coding.

Correct, this is an artifact of development history, specifically
"simple" unions and their elimination.

> Must I create an ImageInfoSpecificNbdWrapper type, with the sole
> purpose of having the same (pointless, IMO) "data":{} wrapper as all
> the other branches of the union type, or am I okay with my addition
> using the flatter style?

I dislike these wrappers, possibly more than is reasonable.  Still, I
think local consistency is more important.

Precedence: commit 7f36a50ab4e (block/file: Add file-specific image
info) added a new branch with a wrapper well after we eliminated
"simple" unions.

I think you should add the silly wrapper.