|
| From: | Vladimir Sementsov-Ogievskiy |
| Subject: | Re: [PATCH v3 08/33] block/nbd: drop thr->state |
| Date: | Wed, 28 Apr 2021 11:01:37 +0300 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 |
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 <vsementsov@virtuozzo.com> --- 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), - 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 to start. Report the attempt as + * failed. Still connect thread is executing in background, and its + * result may be used for next connection attempt. + */I must admit this wasn't quite obvious to me when I started looking at this code. While you're moving this comment, can you please consider rephrasing it? How about this: /* * The connection attempt was canceled and the coroutine resumed * before the connection thread finished its job. Report the * attempt as failed, but leave the connection thread running, * to reuse it for the next connection attempt. */
Yes agree. Moreover, when code is moved to separate file, it shouldn't care _why_ it is cancelled, and comment should not mention drained sections. It's cancelled because user cancel it.
+ error_setg(errp, "Connection attempt cancelled by other operation"); + } else { error_propagate(errp, thr->err); thr->err = NULL; s->sioc = thr->sioc; @@ -491,33 +459,11 @@ nbd_co_establish_connection(BlockDriverState *bs, Error **errp) yank_register_function(BLOCKDEV_YANK_INSTANCE(bs->node_name), nbd_yank, bs); } - ret = (s->sioc ? 0 : -1); - break; - case CONNECT_THREAD_RUNNING: - case CONNECT_THREAD_RUNNING_DETACHED: - /* - * 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. - */ - ret = -1; - error_setg(errp, "Connection attempt cancelled by other operation"); - break; - - case CONNECT_THREAD_NONE: - /* - * Impossible. We've seen this thread running. So it should be - * running or at least give some results. - */ - abort(); - - default: - abort(); }qemu_mutex_unlock(&thr->mutex); - return ret;+ return s->sioc ? 0 : -1; }/*@@ -532,12 +478,9 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs)qemu_mutex_lock(&thr->mutex); - if (thr->state == CONNECT_THREAD_RUNNING) {- /* We can cancel only in running state, when bh is not yet scheduled */ - if (thr->wait_co) { - aio_co_schedule(NULL, thr->wait_co); - thr->wait_co = NULL; - } + if (thr->wait_co) { + aio_co_schedule(NULL, thr->wait_co); + thr->wait_co = NULL;Ah, so you get rid of this redundant check in this patch. Fine by me; please disregard my comment on this matter to the previous patch, then. Roman.}qemu_mutex_unlock(&thr->mutex);-- 2.29.2
-- Best regards, Vladimir
| [Prev in Thread] | Current Thread | [Next in Thread] |