Re: [PATCH v3 07/33] block/nbd: simplify waking of nbd_co_establish_connection()

[Roman Kagan]

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)