Re: [PATCH v3 15/33] nbd/client-connection: use QEMU_LOCK_GUARD

[Roman Kagan]

On Fri, Apr 16, 2021 at 11:08:53AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/client-connection.c | 94 ++++++++++++++++++-----------------------
>  1 file changed, 42 insertions(+), 52 deletions(-)
> 
> diff --git a/nbd/client-connection.c b/nbd/client-connection.c
> index 4e39a5b1af..b45a0bd5f6 100644
> --- a/nbd/client-connection.c
> +++ b/nbd/client-connection.c
> @@ -87,17 +87,16 @@ static void *connect_thread_func(void *opaque)
>          conn->sioc = NULL;
>      }
>  
> -    qemu_mutex_lock(&conn->mutex);
> -
> -    assert(conn->running);
> -    conn->running = false;
> -    if (conn->wait_co) {
> -        aio_co_schedule(NULL, conn->wait_co);
> -        conn->wait_co = NULL;
> +    WITH_QEMU_LOCK_GUARD(&conn->mutex) {
> +        assert(conn->running);
> +        conn->running = false;
> +        if (conn->wait_co) {
> +            aio_co_schedule(NULL, conn->wait_co);
> +            conn->wait_co = NULL;
> +        }
>      }
>      do_free = conn->detached;

->detached is now accessed outside the mutex

>  
> -    qemu_mutex_unlock(&conn->mutex);
>  
>      if (do_free) {
>          nbd_client_connection_do_free(conn);
> @@ -136,61 +135,54 @@ void nbd_client_connection_release(NBDClientConnection 
> *conn)
>  QIOChannelSocket *coroutine_fn
>  nbd_co_establish_connection(NBDClientConnection *conn, Error **errp)
>  {
> -    QIOChannelSocket *sioc = NULL;
>      QemuThread thread;
>  
> -    qemu_mutex_lock(&conn->mutex);
> -
> -    /*
> -     * Don't call nbd_co_establish_connection() in several coroutines in
> -     * parallel. Only one call at once is supported.
> -     */
> -    assert(!conn->wait_co);
> -
> -    if (!conn->running) {
> -        if (conn->sioc) {
> -            /* Previous attempt finally succeeded in background */
> -            sioc = g_steal_pointer(&conn->sioc);
> -            qemu_mutex_unlock(&conn->mutex);
> -
> -            return sioc;
> +    WITH_QEMU_LOCK_GUARD(&conn->mutex) {
> +        /*
> +         * Don't call nbd_co_establish_connection() in several coroutines in
> +         * parallel. Only one call at once is supported.
> +         */
> +        assert(!conn->wait_co);
> +
> +        if (!conn->running) {
> +            if (conn->sioc) {
> +                /* Previous attempt finally succeeded in background */
> +                return g_steal_pointer(&conn->sioc);
> +            }
> +
> +            conn->running = true;
> +            error_free(conn->err);
> +            conn->err = NULL;
> +            qemu_thread_create(&thread, "nbd-connect",
> +                               connect_thread_func, conn, 
> QEMU_THREAD_DETACHED);
>          }
>  
> -        conn->running = true;
> -        error_free(conn->err);
> -        conn->err = NULL;
> -        qemu_thread_create(&thread, "nbd-connect",
> -                           connect_thread_func, conn, QEMU_THREAD_DETACHED);
> +        conn->wait_co = qemu_coroutine_self();
>      }
>  
> -    conn->wait_co = qemu_coroutine_self();
> -
> -    qemu_mutex_unlock(&conn->mutex);
> -
>      /*
>       * We are going to wait for connect-thread finish, but
>       * nbd_co_establish_connection_cancel() can interrupt.
>       */
>      qemu_coroutine_yield();
>  
> -    qemu_mutex_lock(&conn->mutex);
> -
> -    if (conn->running) {
> -        /*
> -         * Obviously, drained section wants to start. Report the attempt as
> -         * failed. Still connect thread is executing in background, and its
> -         * result may be used for next connection attempt.
> -         */
> -        error_setg(errp, "Connection attempt cancelled by other operation");
> -    } else {
> -        error_propagate(errp, conn->err);
> -        conn->err = NULL;
> -        sioc = g_steal_pointer(&conn->sioc);
> +    WITH_QEMU_LOCK_GUARD(&conn->mutex) {
> +        if (conn->running) {
> +            /*
> +             * Obviously, drained section wants to start. Report the attempt 
> as
> +             * failed. Still connect thread is executing in background, and 
> its
> +             * result may be used for next connection attempt.
> +             */
> +            error_setg(errp, "Connection attempt cancelled by other 
> operation");
> +            return NULL;
> +        } else {
> +            error_propagate(errp, conn->err);
> +            conn->err = NULL;
> +            return g_steal_pointer(&conn->sioc);
> +        }
>      }
>  
> -    qemu_mutex_unlock(&conn->mutex);
> -
> -    return sioc;
> +    abort(); /* unreachable */
>  }
>  
>  /*
> @@ -201,12 +193,10 @@ nbd_co_establish_connection(NBDClientConnection *conn, 
> Error **errp)
>   */
>  void coroutine_fn nbd_co_establish_connection_cancel(NBDClientConnection 
> *conn)
>  {
> -    qemu_mutex_lock(&conn->mutex);
> +    QEMU_LOCK_GUARD(&conn->mutex);
>  
>      if (conn->wait_co) {
>          aio_co_schedule(NULL, conn->wait_co);
>          conn->wait_co = NULL;
>      }
> -
> -    qemu_mutex_unlock(&conn->mutex);
>  }
> -- 
> 2.29.2
>