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 <vsement...@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)
