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