| Message ID | 20210416080911.83197-9-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/nbd: rework client connection | expand |
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. > 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. */ > + 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 >
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 >>
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 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, 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; } qemu_mutex_unlock(&thr->mutex);
We don't need all these states. The code refactored to use two boolean variables looks simpler. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/nbd.c | 125 ++++++++++++++-------------------------------------- 1 file changed, 34 insertions(+), 91 deletions(-)