Thank you for the fast review and I'm sorry for the silly and obvious style
errors. Unfortunately I did not notice the section on using the checkpatch
script in the Contributing page on the wiki before committing. But I assure
you that such errors will not occur again.
On Fri, Mar 12, 2021 at 12:23 PM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:
> 11.03.2021 06:15, Mahmoud Mandour wrote:
> > Replaced various qemu_mutex_lock/qemu_mutex_unlock calls with
> > lock guard macros (QEMU_LOCK_GUARD() and WITH_QEMU_LOCK_GUARD).
> > This slightly simplifies the code by eliminating calls to
> > qemu_mutex_unlock and eliminates goto paths.
> >
> > Signed-off-by: Mahmoud Mandour <ma.mando...@gmail.com>
> > ---
> > block/curl.c | 13 ++--
> > block/nbd.c | 188 ++++++++++++++++++++++++---------------------------
>
> Better would be make two separate patches I think.
>
> > 2 files changed, 95 insertions(+), 106 deletions(-)
> >
> > diff --git a/block/curl.c b/block/curl.c
> > index 4ff895df8f..56a217951a 100644
> > --- a/block/curl.c
> > +++ b/block/curl.c
> > @@ -832,12 +832,12 @@ static void curl_setup_preadv(BlockDriverState
> *bs, CURLAIOCB *acb)
> > uint64_t start = acb->offset;
> > uint64_t end;
> >
> > - qemu_mutex_lock(&s->mutex);
> > + QEMU_LOCK_GUARD(&s->mutex);
> >
> > // In case we have the requested data already (e.g. read-ahead),
> > // we can just call the callback and be done.
> > if (curl_find_buf(s, start, acb->bytes, acb)) {
> > - goto out;
> > + return;
> > }
> >
> > // No cache found, so let's start a new request
> > @@ -852,7 +852,7 @@ static void curl_setup_preadv(BlockDriverState *bs,
> CURLAIOCB *acb)
> > if (curl_init_state(s, state) < 0) {
> > curl_clean_state(state);
> > acb->ret = -EIO;
> > - goto out;
> > + return;
> > }
> >
> > acb->start = 0;
> > @@ -867,7 +867,7 @@ static void curl_setup_preadv(BlockDriverState *bs,
> CURLAIOCB *acb)
> > if (state->buf_len && state->orig_buf == NULL) {
> > curl_clean_state(state);
> > acb->ret = -ENOMEM;
> > - goto out;
> > + return;
> > }
> > state->acb[0] = acb;
> >
> > @@ -880,14 +880,11 @@ static void curl_setup_preadv(BlockDriverState
> *bs, CURLAIOCB *acb)
> > acb->ret = -EIO;
> >
> > curl_clean_state(state);
> > - goto out;
> > + return;
> > }
> >
> > /* Tell curl it needs to kick things off */
> > curl_multi_socket_action(s->multi, CURL_SOCKET_TIMEOUT, 0,
> &running);
> > -
> > -out:
> > - qemu_mutex_unlock(&s->mutex);
> > }
>
> This change is obvious and good.
>
> >
> > static int coroutine_fn curl_co_preadv(BlockDriverState *bs,
> > diff --git a/block/nbd.c b/block/nbd.c
> > index c26dc5a54f..28ba7aad61 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -407,27 +407,25 @@ static void *connect_thread_func(void *opaque)
> > thr->sioc = NULL;
> > }
> >
> > - qemu_mutex_lock(&thr->mutex);
> > -
> > - switch (thr->state) {
> > - case CONNECT_THREAD_RUNNING:
> > - thr->state = ret < 0 ? CONNECT_THREAD_FAIL :
> CONNECT_THREAD_SUCCESS;
> > - if (thr->bh_ctx) {
> > - aio_bh_schedule_oneshot(thr->bh_ctx, thr->bh_func,
> thr->bh_opaque);
> > -
> > - /* play safe, don't reuse bh_ctx on further connection
> attempts */
> > - thr->bh_ctx = NULL;
> > + WITH_QEMU_LOCK_GUARD(&thr->mutex) {
> > + switch (thr->state) {
> > + case CONNECT_THREAD_RUNNING:
> > + thr->state = ret < 0 ? CONNECT_THREAD_FAIL :
> CONNECT_THREAD_SUCCESS;
> > + if (thr->bh_ctx) {
> > + aio_bh_schedule_oneshot(thr->bh_ctx, thr->bh_func,
> thr->bh_opaque);
>
> over-80 line
>
> > +
> > + /* play safe, don't reuse bh_ctx on further connection
> attempts */
>
> and here
>
> > + thr->bh_ctx = NULL;
> > + }
> > + break;
> > + case CONNECT_THREAD_RUNNING_DETACHED:
> > + do_free = true;
> > + break;
> > + default:
> > + abort();
> > }
> > - break;
> > - case CONNECT_THREAD_RUNNING_DETACHED:
> > - do_free = true;
> > - break;
> > - default:
> > - abort();
> > }
> >
> > - qemu_mutex_unlock(&thr->mutex);
> > -> if (do_free) {
> > nbd_free_connect_thread(thr);
> > }
> > @@ -443,36 +441,33 @@ nbd_co_establish_connection(BlockDriverState *bs,
> Error **errp)
> > BDRVNBDState *s = bs->opaque;
> > NBDConnectThread *thr = s->connect_thread;
> >
> > - qemu_mutex_lock(&thr->mutex);
> > -
> > - switch (thr->state) {
> > - case CONNECT_THREAD_FAIL:
> > - case CONNECT_THREAD_NONE:
> > - 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->bh_ctx = qemu_get_current_aio_context();
> > + WITH_QEMU_LOCK_GUARD(&thr->mutex) {
> > + switch (thr->state) {
> > + case CONNECT_THREAD_FAIL:
> > + case CONNECT_THREAD_NONE:
> > + 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);
> > + return 0;
> > + case CONNECT_THREAD_RUNNING:
> > + /* Already running, will wait */
> > + break;
> > + default:
> > + abort();
> > + }
> >
> > - qemu_mutex_unlock(&thr->mutex);
> > + thr->bh_ctx = qemu_get_current_aio_context();
> > + }
> >
> >
> > /*
> > @@ -486,46 +481,45 @@ nbd_co_establish_connection(BlockDriverState *bs,
> Error **errp)
> > s->wait_connect = true;
> > qemu_coroutine_yield();
> >
> > - qemu_mutex_lock(&thr->mutex);
> >
> > - switch (thr->state) {
> > - case CONNECT_THREAD_SUCCESS:
> > - case CONNECT_THREAD_FAIL:
> > - thr->state = CONNECT_THREAD_NONE;
> > - error_propagate(errp, thr->err);
> > - thr->err = NULL;
> > - s->sioc = thr->sioc;
> > - thr->sioc = NULL;
> > - if (s->sioc) {
> > -
> 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;
> > + WITH_QEMU_LOCK_GUARD(&thr->mutex) {
> > + switch (thr->state) {
> > + case CONNECT_THREAD_SUCCESS:
> > + case CONNECT_THREAD_FAIL:
> > + thr->state = CONNECT_THREAD_NONE;
> > + error_propagate(errp, thr->err);
> > + thr->err = NULL;
> > + s->sioc = thr->sioc;
> > + thr->sioc = NULL;
> > + if (s->sioc) {
> > +
> 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();
> > + 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();
> > + default:
> > + abort();
> > + }
> > }
> >
> > - qemu_mutex_unlock(&thr->mutex);
> > -
> > return ret;
> > }
> >
> > @@ -546,25 +540,23 @@ static void
> nbd_co_establish_connection_cancel(BlockDriverState *bs,
> > bool wake = false;
> > bool do_free = false;
> >
> > - qemu_mutex_lock(&thr->mutex);
> > -
> > - if (thr->state == CONNECT_THREAD_RUNNING) {
> > - /* We can cancel only in running state, when bh is not yet
> scheduled */
> > - thr->bh_ctx = NULL;
> > - if (s->wait_connect) {
> > - s->wait_connect = false;
> > - wake = true;
> > - }
> > - if (detach) {
> > - thr->state = CONNECT_THREAD_RUNNING_DETACHED;
> > - s->connect_thread = NULL;
> > + WITH_QEMU_LOCK_GUARD(&thr->mutex) {
> > + if (thr->state == CONNECT_THREAD_RUNNING) {
> > + /* We can cancel only in running state, when bh is not yet
> scheduled */
>
> over-80 line
>
> > + thr->bh_ctx = NULL;
> > + if (s->wait_connect) {
> > + s->wait_connect = false;
> > + wake = true;
> > + }
> > + if (detach) {
> > + thr->state = CONNECT_THREAD_RUNNING_DETACHED;
> > + s->connect_thread = NULL;
> > + }
> > + } else if (detach) {
> > + do_free = true;
> > }
> > - } else if (detach) {
> > - do_free = true;
> > }
> >
> > - qemu_mutex_unlock(&thr->mutex);
> > -
> > if (do_free) {
> > nbd_free_connect_thread(thr);
> > s->connect_thread = NULL;
> >
>
>
> For nbd.c we mostly change simple critical sections
>
> qemu_mutex_lock()
> ...
> qemu_mutex_unlock()
>
> into
>
> WITH_QEMU_LOCK_GUARD() {
> ...
> }
>
> And don't eliminate any gotos.. Hmm. On the first sight increasing nesting
> of the code doesn't make it more beautiful.
> But I understand that context-manager concept is safer than calling
> unlock() by hand, and with nested block the critical section becomes more
> obvious. So, with fixed over-80 lines:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>
> --
> Best regards,
> Vladimir
>
