subject:"\[PATCH v3 08\/33\] block\/nbd\: drop thr\->state"

Re: [PATCH v3 08/33] block/nbd: drop thr->state

2021-04-28 Thread Vladimir Sementsov-Ogievskiy


28.04.2021 01:23, Roman Kagan wrote:

On Fri, Apr 16, 2021 at 11:08:46AM +0300, Vladimir Sementsov-Ogievskiy wrote:

We don't need all these states. The code refactored to use two boolean
variables looks simpler.


Indeed.



Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/nbd.c | 125 ++--
  1 file changed, 34 insertions(+), 91 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index e1f39eda6c..2b26a033a4 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -66,24 +66,6 @@ typedef enum NBDClientState {
  NBD_CLIENT_QUIT
  } NBDClientState;
  
-typedef enum NBDConnectThreadState {

-/* No thread, no pending results */
-CONNECT_THREAD_NONE,
-
-/* Thread is running, no results for now */
-CONNECT_THREAD_RUNNING,
-
-/*
- * Thread is running, but requestor exited. Thread should close
- * the new socket and free the connect state on exit.
- */
-CONNECT_THREAD_RUNNING_DETACHED,
-
-/* Thread finished, results are stored in a state */
-CONNECT_THREAD_FAIL,
-CONNECT_THREAD_SUCCESS
-} NBDConnectThreadState;
-
  typedef struct NBDConnectThread {
  /* Initialization constants */
  SocketAddress *saddr; /* address to connect to */
@@ -97,7 +79,8 @@ typedef struct NBDConnectThread {
  
  QemuMutex mutex;

  /* All further fields are protected by mutex */
-NBDConnectThreadState state; /* current state of the thread */
+bool running; /* thread is running now */
+bool detached; /* thread is detached and should cleanup the state */
  Coroutine *wait_co; /* nbd_co_establish_connection() wait in yield() */
  } NBDConnectThread;
  
@@ -147,17 +130,17 @@ static void nbd_clear_bdrvstate(BlockDriverState *bs)

  {
  BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
  NBDConnectThread *thr = s->connect_thread;
-bool thr_running;
+bool do_free;
  
  qemu_mutex_lock(&thr->mutex);

-thr_running = thr->state == CONNECT_THREAD_RUNNING;
-if (thr_running) {
-thr->state = CONNECT_THREAD_RUNNING_DETACHED;
+if (thr->running) {
+thr->detached = true;
  }
+do_free = !thr->running && !thr->detached;


This is redundant.  You can unconditionally set ->detached and only
depend on ->running for the rest of this function.


Still, I don't want to set ->detached unconditionally, just to not break its 
meaning (even before freeing). And that fact that detached is set only once worth 
assertion. So, I think:

assert(!thr->detached);
if (thr->running) {
  thr->detached = true;
} else {
  do_free = true;
}




  qemu_mutex_unlock(&thr->mutex);
  
  /* the runaway thread will clean it up itself */

-if (!thr_running) {
+if (do_free) {
  nbd_free_connect_thread(thr);
  }
  
@@ -373,7 +356,6 @@ static void nbd_init_connect_thread(BDRVNBDState *s)
  
  *s->connect_thread = (NBDConnectThread) {

  .saddr = QAPI_CLONE(SocketAddress, s->saddr),
-.state = CONNECT_THREAD_NONE,
  };
  
  qemu_mutex_init(&s->connect_thread->mutex);

@@ -408,20 +390,13 @@ static void *connect_thread_func(void *opaque)
  
  qemu_mutex_lock(&thr->mutex);
  
-switch (thr->state) {

-case CONNECT_THREAD_RUNNING:
-thr->state = ret < 0 ? CONNECT_THREAD_FAIL : CONNECT_THREAD_SUCCESS;
-if (thr->wait_co) {
-aio_co_schedule(NULL, thr->wait_co);
-thr->wait_co = NULL;
-}
-break;
-case CONNECT_THREAD_RUNNING_DETACHED:
-do_free = true;
-break;
-default:
-abort();
+assert(thr->running);
+thr->running = false;
+if (thr->wait_co) {
+aio_co_schedule(NULL, thr->wait_co);
+thr->wait_co = NULL;
  }
+do_free = thr->detached;
  
  qemu_mutex_unlock(&thr->mutex);
  
@@ -435,36 +410,24 @@ static void *connect_thread_func(void *opaque)

  static int coroutine_fn
  nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
  {
-int ret;
  QemuThread thread;
  BDRVNBDState *s = bs->opaque;
  NBDConnectThread *thr = s->connect_thread;
  
+assert(!s->sioc);

+
  qemu_mutex_lock(&thr->mutex);
  
-switch (thr->state) {

-case CONNECT_THREAD_FAIL:
-case CONNECT_THREAD_NONE:
+if (!thr->running) {
+if (thr->sioc) {
+/* Previous attempt finally succeeded in background */
+goto out;
+}
+thr->running = true;
  error_free(thr->err);
  thr->err = NULL;
-thr->state = CONNECT_THREAD_RUNNING;
  qemu_thread_create(&thread, "nbd-connect",
 connect_thread_func, thr, QEMU_THREAD_DETACHED);
-break;
-case CONNECT_THREAD_SUCCESS:
-/* Previous attempt finally succeeded in background */
-thr->state = CONNECT_THREAD_NONE;
-s->sioc = thr->sioc;
-thr->sioc = NULL;
-yank_register_function(BLOCKDEV_YANK_INSTANCE(bs->node_name),
-

Re: [PATCH v3 08/33] block/nbd: drop thr->state

2021-04-27 Thread Roman Kagan

On Fri, Apr 16, 2021 at 11:08:46AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We don't need all these states. The code refactored to use two boolean
> variables looks simpler.

Indeed.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd.c | 125 ++--
>  1 file changed, 34 insertions(+), 91 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index e1f39eda6c..2b26a033a4 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -66,24 +66,6 @@ typedef enum NBDClientState {
>  NBD_CLIENT_QUIT
>  } NBDClientState;
>  
> -typedef enum NBDConnectThreadState {
> -/* No thread, no pending results */
> -CONNECT_THREAD_NONE,
> -
> -/* Thread is running, no results for now */
> -CONNECT_THREAD_RUNNING,
> -
> -/*
> - * Thread is running, but requestor exited. Thread should close
> - * the new socket and free the connect state on exit.
> - */
> -CONNECT_THREAD_RUNNING_DETACHED,
> -
> -/* Thread finished, results are stored in a state */
> -CONNECT_THREAD_FAIL,
> -CONNECT_THREAD_SUCCESS
> -} NBDConnectThreadState;
> -
>  typedef struct NBDConnectThread {
>  /* Initialization constants */
>  SocketAddress *saddr; /* address to connect to */
> @@ -97,7 +79,8 @@ typedef struct NBDConnectThread {
>  
>  QemuMutex mutex;
>  /* All further fields are protected by mutex */
> -NBDConnectThreadState state; /* current state of the thread */
> +bool running; /* thread is running now */
> +bool detached; /* thread is detached and should cleanup the state */
>  Coroutine *wait_co; /* nbd_co_establish_connection() wait in yield() */
>  } NBDConnectThread;
>  
> @@ -147,17 +130,17 @@ static void nbd_clear_bdrvstate(BlockDriverState *bs)
>  {
>  BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
>  NBDConnectThread *thr = s->connect_thread;
> -bool thr_running;
> +bool do_free;
>  
>  qemu_mutex_lock(&thr->mutex);
> -thr_running = thr->state == CONNECT_THREAD_RUNNING;
> -if (thr_running) {
> -thr->state = CONNECT_THREAD_RUNNING_DETACHED;
> +if (thr->running) {
> +thr->detached = true;
>  }
> +do_free = !thr->running && !thr->detached;

This is redundant.  You can unconditionally set ->detached and only
depend on ->running for the rest of this function.

>  qemu_mutex_unlock(&thr->mutex);
>  
>  /* the runaway thread will clean it up itself */
> -if (!thr_running) {
> +if (do_free) {
>  nbd_free_connect_thread(thr);
>  }
>  
> @@ -373,7 +356,6 @@ static void nbd_init_connect_thread(BDRVNBDState *s)
>  
>  *s->connect_thread = (NBDConnectThread) {
>  .saddr = QAPI_CLONE(SocketAddress, s->saddr),
> -.state = CONNECT_THREAD_NONE,
>  };
>  
>  qemu_mutex_init(&s->connect_thread->mutex);
> @@ -408,20 +390,13 @@ static void *connect_thread_func(void *opaque)
>  
>  qemu_mutex_lock(&thr->mutex);
>  
> -switch (thr->state) {
> -case CONNECT_THREAD_RUNNING:
> -thr->state = ret < 0 ? CONNECT_THREAD_FAIL : CONNECT_THREAD_SUCCESS;
> -if (thr->wait_co) {
> -aio_co_schedule(NULL, thr->wait_co);
> -thr->wait_co = NULL;
> -}
> -break;
> -case CONNECT_THREAD_RUNNING_DETACHED:
> -do_free = true;
> -break;
> -default:
> -abort();
> +assert(thr->running);
> +thr->running = false;
> +if (thr->wait_co) {
> +aio_co_schedule(NULL, thr->wait_co);
> +thr->wait_co = NULL;
>  }
> +do_free = thr->detached;
>  
>  qemu_mutex_unlock(&thr->mutex);
>  
> @@ -435,36 +410,24 @@ static void *connect_thread_func(void *opaque)
>  static int coroutine_fn
>  nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
>  {
> -int ret;
>  QemuThread thread;
>  BDRVNBDState *s = bs->opaque;
>  NBDConnectThread *thr = s->connect_thread;
>  
> +assert(!s->sioc);
> +
>  qemu_mutex_lock(&thr->mutex);
>  
> -switch (thr->state) {
> -case CONNECT_THREAD_FAIL:
> -case CONNECT_THREAD_NONE:
> +if (!thr->running) {
> +if (thr->sioc) {
> +/* Previous attempt finally succeeded in background */
> +goto out;
> +}
> +thr->running = true;
>  error_free(thr->err);
>  thr->err = NULL;
> -thr->state = CONNECT_THREAD_RUNNING;
>  qemu_thread_create(&thread, "nbd-connect",
> connect_thread_func, thr, QEMU_THREAD_DETACHED);
> -break;
> -case CONNECT_THREAD_SUCCESS:
> -/* Previous attempt finally succeeded in background */
> -thr->state = CONNECT_THREAD_NONE;
> -s->sioc = thr->sioc;
> -thr->sioc = NULL;
> -yank_register_function(BLOCKDEV_YANK_INSTANCE(bs->node_name),
> -   nbd_yank, bs);
> -qemu_mutex_unlock(&thr->mutex);
> -return 0;
> -case CONNECT_THREAD_RUNNIN

[PATCH v3 08/33] block/nbd: drop thr->state

2021-04-16 Thread Vladimir Sementsov-Ogievskiy

We don't need all these states. The code refactored to use two boolean
variables looks simpler.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd.c | 125 ++--
 1 file changed, 34 insertions(+), 91 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index e1f39eda6c..2b26a033a4 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -66,24 +66,6 @@ typedef enum NBDClientState {
 NBD_CLIENT_QUIT
 } NBDClientState;
 
-typedef enum NBDConnectThreadState {
-/* No thread, no pending results */
-CONNECT_THREAD_NONE,
-
-/* Thread is running, no results for now */
-CONNECT_THREAD_RUNNING,
-
-/*
- * Thread is running, but requestor exited. Thread should close
- * the new socket and free the connect state on exit.
- */
-CONNECT_THREAD_RUNNING_DETACHED,
-
-/* Thread finished, results are stored in a state */
-CONNECT_THREAD_FAIL,
-CONNECT_THREAD_SUCCESS
-} NBDConnectThreadState;
-
 typedef struct NBDConnectThread {
 /* Initialization constants */
 SocketAddress *saddr; /* address to connect to */
@@ -97,7 +79,8 @@ typedef struct NBDConnectThread {
 
 QemuMutex mutex;
 /* All further fields are protected by mutex */
-NBDConnectThreadState state; /* current state of the thread */
+bool running; /* thread is running now */
+bool detached; /* thread is detached and should cleanup the state */
 Coroutine *wait_co; /* nbd_co_establish_connection() wait in yield() */
 } NBDConnectThread;
 
@@ -147,17 +130,17 @@ static void nbd_clear_bdrvstate(BlockDriverState *bs)
 {
 BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
 NBDConnectThread *thr = s->connect_thread;
-bool thr_running;
+bool do_free;
 
 qemu_mutex_lock(&thr->mutex);
-thr_running = thr->state == CONNECT_THREAD_RUNNING;
-if (thr_running) {
-thr->state = CONNECT_THREAD_RUNNING_DETACHED;
+if (thr->running) {
+thr->detached = true;
 }
+do_free = !thr->running && !thr->detached;
 qemu_mutex_unlock(&thr->mutex);
 
 /* the runaway thread will clean it up itself */
-if (!thr_running) {
+if (do_free) {
 nbd_free_connect_thread(thr);
 }
 
@@ -373,7 +356,6 @@ static void nbd_init_connect_thread(BDRVNBDState *s)
 
 *s->connect_thread = (NBDConnectThread) {
 .saddr = QAPI_CLONE(SocketAddress, s->saddr),
-.state = CONNECT_THREAD_NONE,
 };
 
 qemu_mutex_init(&s->connect_thread->mutex);
@@ -408,20 +390,13 @@ static void *connect_thread_func(void *opaque)
 
 qemu_mutex_lock(&thr->mutex);
 
-switch (thr->state) {
-case CONNECT_THREAD_RUNNING:
-thr->state = ret < 0 ? CONNECT_THREAD_FAIL : CONNECT_THREAD_SUCCESS;
-if (thr->wait_co) {
-aio_co_schedule(NULL, thr->wait_co);
-thr->wait_co = NULL;
-}
-break;
-case CONNECT_THREAD_RUNNING_DETACHED:
-do_free = true;
-break;
-default:
-abort();
+assert(thr->running);
+thr->running = false;
+if (thr->wait_co) {
+aio_co_schedule(NULL, thr->wait_co);
+thr->wait_co = NULL;
 }
+do_free = thr->detached;
 
 qemu_mutex_unlock(&thr->mutex);
 
@@ -435,36 +410,24 @@ static void *connect_thread_func(void *opaque)
 static int coroutine_fn
 nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
 {
-int ret;
 QemuThread thread;
 BDRVNBDState *s = bs->opaque;
 NBDConnectThread *thr = s->connect_thread;
 
+assert(!s->sioc);
+
 qemu_mutex_lock(&thr->mutex);
 
-switch (thr->state) {
-case CONNECT_THREAD_FAIL:
-case CONNECT_THREAD_NONE:
+if (!thr->running) {
+if (thr->sioc) {
+/* Previous attempt finally succeeded in background */
+goto out;
+}
+thr->running = true;
 error_free(thr->err);
 thr->err = NULL;
-thr->state = CONNECT_THREAD_RUNNING;
 qemu_thread_create(&thread, "nbd-connect",
connect_thread_func, thr, QEMU_THREAD_DETACHED);
-break;
-case CONNECT_THREAD_SUCCESS:
-/* Previous attempt finally succeeded in background */
-thr->state = CONNECT_THREAD_NONE;
-s->sioc = thr->sioc;
-thr->sioc = NULL;
-yank_register_function(BLOCKDEV_YANK_INSTANCE(bs->node_name),
-   nbd_yank, bs);
-qemu_mutex_unlock(&thr->mutex);
-return 0;
-case CONNECT_THREAD_RUNNING:
-/* Already running, will wait */
-break;
-default:
-abort();
 }
 
 thr->wait_co = qemu_coroutine_self();
@@ -479,10 +442,15 @@ nbd_co_establish_connection(BlockDriverState *bs, Error 
**errp)
 
 qemu_mutex_lock(&thr->mutex);
 
-switch (thr->state) {
-case CONNECT_THREAD_SUCCESS:
-case CONNECT_THREAD_FAIL:
-thr->state = CONNECT_THREAD_NONE;
+out:
+if (thr->running) {
+/*
+ * Obviously, drained section wants t

Re: [PATCH v3 08/33] block/nbd: drop thr->state

Re: [PATCH v3 08/33] block/nbd: drop thr->state

[PATCH v3 08/33] block/nbd: drop thr->state

3 matches

Site Navigation

Mail list logo

Footer information