Re: [PATCH v2 1/4] nbd: Add multi-conn option

["Richard W.M. Jones" <rjones@redhat.com>]

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