diff mbox series

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

Message ID 20210416080911.83197-16-vsementsov@virtuozzo.com (mailing list archive)
State New
Headers show
Series block/nbd: rework client connection | expand

Commit Message

Vladimir Sementsov-Ogievskiy April 16, 2021, 8:08 a.m. UTC
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client-connection.c | 94 ++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 52 deletions(-)

Comments

Roman Kagan April 28, 2021, 6:08 a.m. UTC | #1
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
>
Vladimir Sementsov-Ogievskiy April 28, 2021, 8:17 a.m. UTC | #2
28.04.2021 09:08, Roman Kagan wrote:
> 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

Oops. Will fix.

> 
>>   
>> -    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
>>
diff mbox series

Patch

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;
 
-    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);
 }