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.
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.mandourr@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 <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir