Re: [PATCH nbd 2/4] nbd: Split out block device state from underlying NBD connections

[Eric Blake]

On Thu, Mar 09, 2023 at 11:39:44AM +0000, Richard W.M. Jones wrote:
> To implement multi-conn, we will put multiple underlying NBD
> connections (ie. NBDClientConnection) inside the NBD block device
> handle (BDRVNBDState).  This requires first breaking the one-to-one
> relationship between NBDClientConnection and BDRVNBDState.
> 
> To do this a new structure (NBDConnState) is implemented.
> NBDConnState takes all the per-connection state out of the block
> driver struct.  BDRVNBDState now contains a conns[] array of pointers
> to NBDConnState, one for each underlying multi-conn connection.
> 
> After this change there is still a one-to-one relationship because we
> only ever use the zeroth slot in the conns[] array.  Thus this does
> not implement multi-conn yet.
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  block/coroutines.h |   5 +-
>  block/nbd.c        | 674 ++++++++++++++++++++++++---------------------
>  2 files changed, 358 insertions(+), 321 deletions(-)
> 
> diff --git a/block/coroutines.h b/block/coroutines.h
> index dd9f3d449b..14b01d8591 100644
> --- a/block/coroutines.h
> +++ b/block/coroutines.h
> @@ -62,7 +62,7 @@ int coroutine_fn GRAPH_RDLOCK
>  bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t 
> pos);
>  
>  int coroutine_fn
> -nbd_co_do_establish_connection(BlockDriverState *bs, bool blocking,
> +nbd_co_do_establish_connection(BlockDriverState *bs, void *cs, bool blocking,
>                                 Error **errp);

I guess the void* here is because you couldn't find a way to expose
the new type through the coroutine wrapper generator?

>  
>  
> @@ -86,6 +86,7 @@ bdrv_common_block_status_above(BlockDriverState *bs,
>                                 BlockDriverState **file,
>                                 int *depth);
>  int co_wrapper_mixed
> -nbd_do_establish_connection(BlockDriverState *bs, bool blocking, Error 
> **errp);
> +nbd_do_establish_connection(BlockDriverState *bs, void *cs, bool blocking,
> +                            Error **errp);
>

same here

>  #endif /* BLOCK_COROUTINES_H */
> diff --git a/block/nbd.c b/block/nbd.c
> index 5ffae0b798..84e8a1add0 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -51,8 +51,8 @@
>  #define MAX_NBD_REQUESTS    16
>  #define MAX_MULTI_CONN      16
>  
> -#define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))
> -#define INDEX_TO_HANDLE(bs, index)  ((index)  ^ (uint64_t)(intptr_t)(bs))
> +#define HANDLE_TO_INDEX(cs, handle) ((handle) ^ (uint64_t)(intptr_t)(cs))
> +#define INDEX_TO_HANDLE(cs, index)  ((index)  ^ (uint64_t)(intptr_t)(cs))

Independently of your patches, these macros are odd.  I think we could
just as easily define

#define HANDLE_TO_INDEX(XX, handle) ((handle) - 1)
#define INDEX_TO_HANDLE(XX, index) ((index) + 1)

and then refactor to drop the unused parameter, since we never really
depend on it (I audited the code when you first asked me about it on
IRC, and we do a bounds check that whatever the server returns lies
within our expected index of 0-15 before dereferencing any memory with
it, so we are NOT relying on the server passing us a pointer that we
depend on).

Overall, your patch is mostly mechanical and looks nice to me.

>  
>  /*
>   * Update @bs with information learned during a completed negotiation 
> process.
>   * Return failure if the server's advertised options are incompatible with 
> the
>   * client's needs.
> + *
> + * Note that we are only called for the first connection (s->conns[0])
> + * because multi-conn assumes that all other connections are alike.
> + * We don't check that assumption but probably should (XXX).
>   */
> -static int nbd_handle_updated_info(BlockDriverState *bs, Error **errp)
> +static int nbd_handle_updated_info(BlockDriverState *bs,
> +                                   NBDConnState *cs, Error **errp)

But as you pointed out in your cover letter, we'll probably want to
address the XXX comments like this one prior to actually committing
the series.  We really should be making sure that the secondary
clients see the same server parameters as the first one, regardless of
whether they are open in parallel (once multi-conn is enabled) or in
series after a reopen (which is what we currently try to support).

> @@ -318,129 +332,132 @@ static int nbd_handle_updated_info(BlockDriverState 
> *bs, Error **errp)
>  }
>  
>  int coroutine_fn nbd_co_do_establish_connection(BlockDriverState *bs,
> +                                                void *csvp,
>                                                  bool blocking, Error **errp)
>  {
>      BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> +    NBDConnState *cs = csvp;

Is there a way to get the new type passed through the coroutine
generator without the use of void*?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org