diff mbox series

[v3,18/33] nbd/client-connection: shutdown connection on release

Message ID 20210416080911.83197-19-vsementsov@virtuozzo.com
State New
Headers show
Series block/nbd: rework client connection | expand

Commit Message

Vladimir Sementsov-Ogievskiy April 16, 2021, 8:08 a.m. UTC 
Now, when thread can do negotiation and retry, it may run relatively
long. We need a mechanism to stop it, when user is not interested in
result anymore. So, on nbd_client_connection_release() let's shutdown
the socked, and do not retry connection if thread is detached.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client-connection.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

Comments

Roman Kagan May 11, 2021, 9:08 p.m. UTC | #1 
On Fri, Apr 16, 2021 at 11:08:56AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Now, when thread can do negotiation and retry, it may run relatively
> long. We need a mechanism to stop it, when user is not interested in
> result anymore. So, on nbd_client_connection_release() let's shutdown
> the socked, and do not retry connection if thread is detached.

This kinda answers my question to the previous patch about reconnect
cancellation.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/client-connection.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/nbd/client-connection.c b/nbd/client-connection.c
> index 002bd91f42..54f73c6c24 100644
> --- a/nbd/client-connection.c
> +++ b/nbd/client-connection.c
> @@ -158,9 +158,13 @@ static void *connect_thread_func(void *opaque)
>      uint64_t timeout = 1;
>      uint64_t max_timeout = 16;
>  
> -    while (true) {
> +    qemu_mutex_lock(&conn->mutex);
> +    while (!conn->detached) {
> +        assert(!conn->sioc);
>          conn->sioc = qio_channel_socket_new();
>  
> +        qemu_mutex_unlock(&conn->mutex);
> +
>          error_free(conn->err);
>          conn->err = NULL;
>          conn->updated_info = conn->initial_info;
> @@ -171,14 +175,20 @@ static void *connect_thread_func(void *opaque)
>          conn->updated_info.x_dirty_bitmap = NULL;
>          conn->updated_info.name = NULL;
>  
> +        qemu_mutex_lock(&conn->mutex);
> +
>          if (ret < 0) {
>              object_unref(OBJECT(conn->sioc));
>              conn->sioc = NULL;
> -            if (conn->do_retry) {
> +            if (conn->do_retry && !conn->detached) {
> +                qemu_mutex_unlock(&conn->mutex);
> +
>                  sleep(timeout);
>                  if (timeout < max_timeout) {
>                      timeout *= 2;
>                  }
> +
> +                qemu_mutex_lock(&conn->mutex);
>                  continue;
>              }
>          }
> @@ -186,15 +196,17 @@ static void *connect_thread_func(void *opaque)
>          break;
>      }
>  
> -    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;
> +    /* mutex is locked */
> +
> +    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);

This basically reverts some hunks from patch 15 "nbd/client-connection:
use QEMU_LOCK_GUARD".  How about dropping them there (they weren't an
obvious improvement even then).

Roman.

>  
>      if (do_free) {
>          nbd_client_connection_do_free(conn);
> @@ -215,6 +227,10 @@ void nbd_client_connection_release(NBDClientConnection *conn)
>      if (conn->running) {
>          conn->detached = true;
>      }
> +    if (conn->sioc) {
> +        qio_channel_shutdown(QIO_CHANNEL(conn->sioc),
> +                             QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> +    }
>      do_free = !conn->running && !conn->detached;
>      qemu_mutex_unlock(&conn->mutex);
>  
> -- 
> 2.29.2
>
Vladimir Sementsov-Ogievskiy May 12, 2021, 6:39 a.m. UTC | #2 
12.05.2021 00:08, Roman Kagan wrote:
> On Fri, Apr 16, 2021 at 11:08:56AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Now, when thread can do negotiation and retry, it may run relatively
>> long. We need a mechanism to stop it, when user is not interested in
>> result anymore. So, on nbd_client_connection_release() let's shutdown
>> the socked, and do not retry connection if thread is detached.
> 
> This kinda answers my question to the previous patch about reconnect
> cancellation.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/client-connection.c | 36 ++++++++++++++++++++++++++----------
>>   1 file changed, 26 insertions(+), 10 deletions(-)
>>
>> diff --git a/nbd/client-connection.c b/nbd/client-connection.c
>> index 002bd91f42..54f73c6c24 100644
>> --- a/nbd/client-connection.c
>> +++ b/nbd/client-connection.c
>> @@ -158,9 +158,13 @@ static void *connect_thread_func(void *opaque)
>>       uint64_t timeout = 1;
>>       uint64_t max_timeout = 16;
>>   
>> -    while (true) {
>> +    qemu_mutex_lock(&conn->mutex);
>> +    while (!conn->detached) {
>> +        assert(!conn->sioc);
>>           conn->sioc = qio_channel_socket_new();
>>   
>> +        qemu_mutex_unlock(&conn->mutex);
>> +
>>           error_free(conn->err);
>>           conn->err = NULL;
>>           conn->updated_info = conn->initial_info;
>> @@ -171,14 +175,20 @@ static void *connect_thread_func(void *opaque)
>>           conn->updated_info.x_dirty_bitmap = NULL;
>>           conn->updated_info.name = NULL;
>>   
>> +        qemu_mutex_lock(&conn->mutex);
>> +
>>           if (ret < 0) {
>>               object_unref(OBJECT(conn->sioc));
>>               conn->sioc = NULL;
>> -            if (conn->do_retry) {
>> +            if (conn->do_retry && !conn->detached) {
>> +                qemu_mutex_unlock(&conn->mutex);
>> +
>>                   sleep(timeout);
>>                   if (timeout < max_timeout) {
>>                       timeout *= 2;
>>                   }
>> +
>> +                qemu_mutex_lock(&conn->mutex);
>>                   continue;
>>               }
>>           }
>> @@ -186,15 +196,17 @@ static void *connect_thread_func(void *opaque)
>>           break;
>>       }
>>   
>> -    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;
>> +    /* mutex is locked */
>> +
>> +    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);
> 
> This basically reverts some hunks from patch 15 "nbd/client-connection:
> use QEMU_LOCK_GUARD".  How about dropping them there (they weren't an
> obvious improvement even then).
> 

OK, will do

> 
>>   
>>       if (do_free) {
>>           nbd_client_connection_do_free(conn);
>> @@ -215,6 +227,10 @@ void nbd_client_connection_release(NBDClientConnection *conn)
>>       if (conn->running) {
>>           conn->detached = true;
>>       }
>> +    if (conn->sioc) {
>> +        qio_channel_shutdown(QIO_CHANNEL(conn->sioc),
>> +                             QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
>> +    }
>>       do_free = !conn->running && !conn->detached;
>>       qemu_mutex_unlock(&conn->mutex);
>>   
>> -- 
>> 2.29.2
>>
Eric Blake June 3, 2021, 4:20 p.m. UTC | #3 
On Fri, Apr 16, 2021 at 11:08:56AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Now, when thread can do negotiation and retry, it may run relatively

when a thread

> long. We need a mechanism to stop it, when user is not interested in

when the user

> result anymore. So, on nbd_client_connection_release() let's shutdown

in a result any more.

> the socked, and do not retry connection if thread is detached.

socket

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/client-connection.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
>
diff mbox series

Patch

diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index 002bd91f42..54f73c6c24 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -158,9 +158,13 @@  static void *connect_thread_func(void *opaque)
     uint64_t timeout = 1;
     uint64_t max_timeout = 16;
 
-    while (true) {
+    qemu_mutex_lock(&conn->mutex);
+    while (!conn->detached) {
+        assert(!conn->sioc);
         conn->sioc = qio_channel_socket_new();
 
+        qemu_mutex_unlock(&conn->mutex);
+
         error_free(conn->err);
         conn->err = NULL;
         conn->updated_info = conn->initial_info;
@@ -171,14 +175,20 @@  static void *connect_thread_func(void *opaque)
         conn->updated_info.x_dirty_bitmap = NULL;
         conn->updated_info.name = NULL;
 
+        qemu_mutex_lock(&conn->mutex);
+
         if (ret < 0) {
             object_unref(OBJECT(conn->sioc));
             conn->sioc = NULL;
-            if (conn->do_retry) {
+            if (conn->do_retry && !conn->detached) {
+                qemu_mutex_unlock(&conn->mutex);
+
                 sleep(timeout);
                 if (timeout < max_timeout) {
                     timeout *= 2;
                 }
+
+                qemu_mutex_lock(&conn->mutex);
                 continue;
             }
         }
@@ -186,15 +196,17 @@  static void *connect_thread_func(void *opaque)
         break;
     }
 
-    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;
+    /* mutex is locked */
+
+    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);
@@ -215,6 +227,10 @@  void nbd_client_connection_release(NBDClientConnection *conn)
     if (conn->running) {
         conn->detached = true;
     }
+    if (conn->sioc) {
+        qio_channel_shutdown(QIO_CHANNEL(conn->sioc),
+                             QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+    }
     do_free = !conn->running && !conn->detached;
     qemu_mutex_unlock(&conn->mutex);