| Message ID | 20210416080911.83197-8-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/nbd: rework client connection | expand |
On Fri, Apr 16, 2021 at 11:08:45AM +0300, Vladimir Sementsov-Ogievskiy wrote: > Instead of connect_bh, bh_ctx and wait_connect fields we can live with > only one link to waiting coroutine, protected by mutex. > > So new logic is: > > nbd_co_establish_connection() sets wait_co under mutex, release the > mutex and do yield(). Note, that wait_co may be scheduled by thread > immediately after unlocking the mutex. Still, in main thread (or > iothread) we'll not reach the code for entering the coroutine until the > yield() so we are safe. > > Both connect_thread_func() and nbd_co_establish_connection_cancel() do > the following to handle wait_co: > > Under mutex, if thr->wait_co is not NULL, call aio_co_wake() (which > never tries to acquire aio context since previous commit, so we are > safe to do it under thr->mutex) and set thr->wait_co to NULL. > This way we protect ourselves of scheduling it twice. > > Also this commit make nbd_co_establish_connection() less connected to > bs (we have generic pointer to the coroutine, not use s->connection_co > directly). So, we are on the way of splitting connection API out of > nbd.c (which is overcomplicated now). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/nbd.c | 49 +++++++++---------------------------------------- > 1 file changed, 9 insertions(+), 40 deletions(-) > > diff --git a/block/nbd.c b/block/nbd.c > index d67556c7ee..e1f39eda6c 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -87,12 +87,6 @@ typedef enum NBDConnectThreadState { > typedef struct NBDConnectThread { > /* Initialization constants */ > SocketAddress *saddr; /* address to connect to */ > - /* > - * Bottom half to schedule on completion. Scheduled only if bh_ctx is not > - * NULL > - */ > - QEMUBHFunc *bh_func; > - void *bh_opaque; > > /* > * Result of last attempt. Valid in FAIL and SUCCESS states. > @@ -101,10 +95,10 @@ typedef struct NBDConnectThread { > QIOChannelSocket *sioc; > Error *err; > > - /* state and bh_ctx are protected by mutex */ > QemuMutex mutex; > + /* All further fields are protected by mutex */ > NBDConnectThreadState state; /* current state of the thread */ > - AioContext *bh_ctx; /* where to schedule bh (NULL means don't schedule) */ > + Coroutine *wait_co; /* nbd_co_establish_connection() wait in yield() */ > } NBDConnectThread; > > typedef struct BDRVNBDState { > @@ -138,7 +132,6 @@ typedef struct BDRVNBDState { > char *x_dirty_bitmap; > bool alloc_depth; > > - bool wait_connect; > NBDConnectThread *connect_thread; > } BDRVNBDState; > > @@ -374,15 +367,6 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s) > return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT; > } > > -static void connect_bh(void *opaque) > -{ > - BDRVNBDState *state = opaque; > - > - assert(state->wait_connect); > - state->wait_connect = false; > - aio_co_wake(state->connection_co); > -} > - > static void nbd_init_connect_thread(BDRVNBDState *s) > { > s->connect_thread = g_new(NBDConnectThread, 1); > @@ -390,8 +374,6 @@ static void nbd_init_connect_thread(BDRVNBDState *s) > *s->connect_thread = (NBDConnectThread) { > .saddr = QAPI_CLONE(SocketAddress, s->saddr), > .state = CONNECT_THREAD_NONE, > - .bh_func = connect_bh, > - .bh_opaque = s, > }; > > qemu_mutex_init(&s->connect_thread->mutex); > @@ -429,11 +411,9 @@ static void *connect_thread_func(void *opaque) > switch (thr->state) { > case CONNECT_THREAD_RUNNING: > thr->state = ret < 0 ? CONNECT_THREAD_FAIL : CONNECT_THREAD_SUCCESS; > - if (thr->bh_ctx) { > - aio_bh_schedule_oneshot(thr->bh_ctx, thr->bh_func, thr->bh_opaque); > - > - /* play safe, don't reuse bh_ctx on further connection attempts */ > - thr->bh_ctx = NULL; > + if (thr->wait_co) { > + aio_co_schedule(NULL, thr->wait_co); > + thr->wait_co = NULL; > } > break; > case CONNECT_THREAD_RUNNING_DETACHED: > @@ -487,20 +467,14 @@ nbd_co_establish_connection(BlockDriverState *bs, Error **errp) > abort(); > } > > - thr->bh_ctx = qemu_get_current_aio_context(); > + thr->wait_co = qemu_coroutine_self(); > > qemu_mutex_unlock(&thr->mutex); > > - > /* > * We are going to wait for connect-thread finish, but > * nbd_client_co_drain_begin() can interrupt. > - * > - * Note that wait_connect variable is not visible for connect-thread. It > - * doesn't need mutex protection, it used only inside home aio context of > - * bs. > */ > - s->wait_connect = true; > qemu_coroutine_yield(); > > qemu_mutex_lock(&thr->mutex); > @@ -555,24 +529,19 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs) > { > BDRVNBDState *s = bs->opaque; > NBDConnectThread *thr = s->connect_thread; > - bool wake = false; > > qemu_mutex_lock(&thr->mutex); > > if (thr->state == CONNECT_THREAD_RUNNING) { This check looks redundant and can probably go. This patch may be quite appropriate for this: the logic becomes even more straightforward. > /* We can cancel only in running state, when bh is not yet scheduled */ > - thr->bh_ctx = NULL; > - if (s->wait_connect) { > - s->wait_connect = false; > - wake = true; > + if (thr->wait_co) { > + aio_co_schedule(NULL, thr->wait_co); This will probably be replaced by a new function per our discussion of the previous patch. Note however that if that one doesn't fly for whatever reason you can retain the aio_context on NBDConnectThread and pass it explicitly into aio_co_schedule here. Roman. > + thr->wait_co = NULL; > } > } > > qemu_mutex_unlock(&thr->mutex); > > - if (wake) { > - aio_co_wake(s->connection_co); > - } > } > > static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
On Fri, Apr 16, 2021 at 11:08:45AM +0300, Vladimir Sementsov-Ogievskiy wrote: Grammar suggestions: > Instead of connect_bh, bh_ctx and wait_connect fields we can live with > only one link to waiting coroutine, protected by mutex. Instead of managing connect_bh, bh_ctx, and wait_connect fields, we can use a single link to the waiting coroutine with proper mutex protection. > > So new logic is: > > nbd_co_establish_connection() sets wait_co under mutex, release the > mutex and do yield(). Note, that wait_co may be scheduled by thread > immediately after unlocking the mutex. Still, in main thread (or > iothread) we'll not reach the code for entering the coroutine until the > yield() so we are safe. nbd_co_establish_connection() sets wait_co under the mutex, releases the mutex, then yield()s. Note that wait_co may be scheduled by the thread immediately after unlocking the mutex. Still, the main thread (or iothread) will not reach the code for entering the coroutine until the yield(), so we are safe. > > Both connect_thread_func() and nbd_co_establish_connection_cancel() do > the following to handle wait_co: > > Under mutex, if thr->wait_co is not NULL, call aio_co_wake() (which > never tries to acquire aio context since previous commit, so we are > safe to do it under thr->mutex) and set thr->wait_co to NULL. > This way we protect ourselves of scheduling it twice. Under the mutex, if thr_wait_co is not NULL, call aio_co_wake() (the previous commit ensures it never tries to acquire the aio context, so we are safe even while under thr->mutex), then sets thr->wait_co to NULL. This way, we avoid scheduling the coroutine twice. > > Also this commit make nbd_co_establish_connection() less connected to > bs (we have generic pointer to the coroutine, not use s->connection_co > directly). So, we are on the way of splitting connection API out of > nbd.c (which is overcomplicated now). Also, this commit reduces the dependence of nbd_co_establish_connection() on the internals of bs (we now use a generic pointer to the coroutine, instead of direct use of s->connection_co). This is a step towards splitting the connection API out of nbd.c. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/nbd.c | 49 +++++++++---------------------------------------- > 1 file changed, 9 insertions(+), 40 deletions(-) > > +++ b/block/nbd.c > @@ -101,10 +95,10 @@ typedef struct NBDConnectThread { > QIOChannelSocket *sioc; > Error *err; > > - /* state and bh_ctx are protected by mutex */ > QemuMutex mutex; > + /* All further fields are protected by mutex */ > NBDConnectThreadState state; /* current state of the thread */ > - AioContext *bh_ctx; /* where to schedule bh (NULL means don't schedule) */ > + Coroutine *wait_co; /* nbd_co_establish_connection() wait in yield() */ I'm not sure if that comment is the most legible, but I'm not coming up with an alternative. Maybe: /* * if non-NULL, which coroutine to wake in * nbd_co_establish_connection() after yield() */ But the simplification looks nice, and I didn't spot any obvious problems with the refactoring. Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/block/nbd.c b/block/nbd.c index d67556c7ee..e1f39eda6c 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -87,12 +87,6 @@ typedef enum NBDConnectThreadState { typedef struct NBDConnectThread { /* Initialization constants */ SocketAddress *saddr; /* address to connect to */ - /* - * Bottom half to schedule on completion. Scheduled only if bh_ctx is not - * NULL - */ - QEMUBHFunc *bh_func; - void *bh_opaque; /* * Result of last attempt. Valid in FAIL and SUCCESS states. @@ -101,10 +95,10 @@ typedef struct NBDConnectThread { QIOChannelSocket *sioc; Error *err; - /* state and bh_ctx are protected by mutex */ QemuMutex mutex; + /* All further fields are protected by mutex */ NBDConnectThreadState state; /* current state of the thread */ - AioContext *bh_ctx; /* where to schedule bh (NULL means don't schedule) */ + Coroutine *wait_co; /* nbd_co_establish_connection() wait in yield() */ } NBDConnectThread; typedef struct BDRVNBDState { @@ -138,7 +132,6 @@ typedef struct BDRVNBDState { char *x_dirty_bitmap; bool alloc_depth; - bool wait_connect; NBDConnectThread *connect_thread; } BDRVNBDState; @@ -374,15 +367,6 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s) return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT; } -static void connect_bh(void *opaque) -{ - BDRVNBDState *state = opaque; - - assert(state->wait_connect); - state->wait_connect = false; - aio_co_wake(state->connection_co); -} - static void nbd_init_connect_thread(BDRVNBDState *s) { s->connect_thread = g_new(NBDConnectThread, 1); @@ -390,8 +374,6 @@ static void nbd_init_connect_thread(BDRVNBDState *s) *s->connect_thread = (NBDConnectThread) { .saddr = QAPI_CLONE(SocketAddress, s->saddr), .state = CONNECT_THREAD_NONE, - .bh_func = connect_bh, - .bh_opaque = s, }; qemu_mutex_init(&s->connect_thread->mutex); @@ -429,11 +411,9 @@ static void *connect_thread_func(void *opaque) switch (thr->state) { case CONNECT_THREAD_RUNNING: thr->state = ret < 0 ? CONNECT_THREAD_FAIL : CONNECT_THREAD_SUCCESS; - if (thr->bh_ctx) { - aio_bh_schedule_oneshot(thr->bh_ctx, thr->bh_func, thr->bh_opaque); - - /* play safe, don't reuse bh_ctx on further connection attempts */ - thr->bh_ctx = NULL; + if (thr->wait_co) { + aio_co_schedule(NULL, thr->wait_co); + thr->wait_co = NULL; } break; case CONNECT_THREAD_RUNNING_DETACHED: @@ -487,20 +467,14 @@ nbd_co_establish_connection(BlockDriverState *bs, Error **errp) abort(); } - thr->bh_ctx = qemu_get_current_aio_context(); + thr->wait_co = qemu_coroutine_self(); qemu_mutex_unlock(&thr->mutex); - /* * We are going to wait for connect-thread finish, but * nbd_client_co_drain_begin() can interrupt. - * - * Note that wait_connect variable is not visible for connect-thread. It - * doesn't need mutex protection, it used only inside home aio context of - * bs. */ - s->wait_connect = true; qemu_coroutine_yield(); qemu_mutex_lock(&thr->mutex); @@ -555,24 +529,19 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs) { BDRVNBDState *s = bs->opaque; NBDConnectThread *thr = s->connect_thread; - bool wake = false; qemu_mutex_lock(&thr->mutex); if (thr->state == CONNECT_THREAD_RUNNING) { /* We can cancel only in running state, when bh is not yet scheduled */ - thr->bh_ctx = NULL; - if (s->wait_connect) { - s->wait_connect = false; - wake = true; + if (thr->wait_co) { + aio_co_schedule(NULL, thr->wait_co); + thr->wait_co = NULL; } } qemu_mutex_unlock(&thr->mutex); - if (wake) { - aio_co_wake(s->connection_co); - } } static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
Instead of connect_bh, bh_ctx and wait_connect fields we can live with only one link to waiting coroutine, protected by mutex. So new logic is: nbd_co_establish_connection() sets wait_co under mutex, release the mutex and do yield(). Note, that wait_co may be scheduled by thread immediately after unlocking the mutex. Still, in main thread (or iothread) we'll not reach the code for entering the coroutine until the yield() so we are safe. Both connect_thread_func() and nbd_co_establish_connection_cancel() do the following to handle wait_co: Under mutex, if thr->wait_co is not NULL, call aio_co_wake() (which never tries to acquire aio context since previous commit, so we are safe to do it under thr->mutex) and set thr->wait_co to NULL. This way we protect ourselves of scheduling it twice. Also this commit make nbd_co_establish_connection() less connected to bs (we have generic pointer to the coroutine, not use s->connection_co directly). So, we are on the way of splitting connection API out of nbd.c (which is overcomplicated now). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/nbd.c | 49 +++++++++---------------------------------------- 1 file changed, 9 insertions(+), 40 deletions(-)