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

[Markus Armbruster <armbru@redhat.com>]

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: