Re: [PATCH v3 18/33] nbd/client-connection: shutdown connection on release
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 > --- > nbd/client-connection.c | 36 ++-- > 1 file changed, 26 insertions(+), 10 deletions(-) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 18/33] nbd/client-connection: shutdown connection on release
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 --- 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 -- Best regards, Vladimir
Re: [PATCH v3 18/33] nbd/client-connection: shutdown connection on release
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 > --- > 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 >
[PATCH v3 18/33] nbd/client-connection: shutdown connection on release
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 --- 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); 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