[PATCH 0/4] Use lock guard macros in block

[PATCH 0/4] Use lock guard macros in block

[Gan Qixin]

Hi all,
  I saw some tasks to replace manual lock()/unlock() calls with lock guard macros in BiteSizedTasks.
I am very interested in this and modified some of the files under block. Could someone help me check the code?

Thanks,
Gan Qixin

Gan Qixin (4):
  block/accounting.c: Use lock guard macros
  block/curl.c: Use lock guard macros
  block/throttle-groups.c: Use lock guard macros
  block/iscsi.c: Use lock guard macros

 block/accounting.c      | 32 +++++++++++++--------------
 block/curl.c            | 28 ++++++++++++------------
 block/iscsi.c           | 28 +++++++++++-------------
 block/throttle-groups.c | 48 ++++++++++++++++++++---------------------
 4 files changed, 65 insertions(+), 71 deletions(-)

-- 
2.23.0

[PATCH 1/4] block/accounting.c: Use lock guard macros

[Gan Qixin]

Replace manual lock()/unlock() calls with lock guard macros
(QEMU_LOCK_GUARD/WITH_QEMU_LOCK_GUARD) in block/accounting.c.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
 block/accounting.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/block/accounting.c b/block/accounting.c
index 8d41c8a83a..2030851d79 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -199,29 +199,27 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
         return;
     }
 
-    qemu_mutex_lock(&stats->lock);
-
-    if (failed) {
-        stats->failed_ops[cookie->type]++;
-    } else {
-        stats->nr_bytes[cookie->type] += cookie->bytes;
-        stats->nr_ops[cookie->type]++;
-    }
+    WITH_QEMU_LOCK_GUARD(&stats->lock) {
+        if (failed) {
+            stats->failed_ops[cookie->type]++;
+        } else {
+            stats->nr_bytes[cookie->type] += cookie->bytes;
+            stats->nr_ops[cookie->type]++;
+        }
 
-    block_latency_histogram_account(&stats->latency_histogram[cookie->type],
-                                    latency_ns);
+        block_latency_histogram_account(&stats->latency_histogram[cookie->type],
+                                        latency_ns);
 
-    if (!failed || stats->account_failed) {
-        stats->total_time_ns[cookie->type] += latency_ns;
-        stats->last_access_time_ns = time_ns;
+        if (!failed || stats->account_failed) {
+            stats->total_time_ns[cookie->type] += latency_ns;
+            stats->last_access_time_ns = time_ns;
 
-        QSLIST_FOREACH(s, &stats->intervals, entries) {
-            timed_average_account(&s->latency[cookie->type], latency_ns);
+            QSLIST_FOREACH(s, &stats->intervals, entries) {
+                timed_average_account(&s->latency[cookie->type], latency_ns);
+            }
         }
     }
 
-    qemu_mutex_unlock(&stats->lock);
-
     cookie->type = BLOCK_ACCT_NONE;
 }
 
-- 
2.23.0

[PATCH 2/4] block/curl.c: Use lock guard macros

[Gan Qixin]

Replace manual lock()/unlock() calls with lock guard macros
(QEMU_LOCK_GUARD/WITH_QEMU_LOCK_GUARD) in block/curl.c.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
 block/curl.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 4f907c47be..d24a4c5897 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -564,23 +564,23 @@ static void curl_detach_aio_context(BlockDriverState *bs)
     BDRVCURLState *s = bs->opaque;
     int i;
 
-    qemu_mutex_lock(&s->mutex);
-    for (i = 0; i < CURL_NUM_STATES; i++) {
-        if (s->states[i].in_use) {
-            curl_clean_state(&s->states[i]);
+    WITH_QEMU_LOCK_GUARD(&s->mutex) {
+        for (i = 0; i < CURL_NUM_STATES; i++) {
+            if (s->states[i].in_use) {
+                curl_clean_state(&s->states[i]);
+            }
+            if (s->states[i].curl) {
+                curl_easy_cleanup(s->states[i].curl);
+                s->states[i].curl = NULL;
+            }
+            g_free(s->states[i].orig_buf);
+            s->states[i].orig_buf = NULL;
         }
-        if (s->states[i].curl) {
-            curl_easy_cleanup(s->states[i].curl);
-            s->states[i].curl = NULL;
+        if (s->multi) {
+            curl_multi_cleanup(s->multi);
+            s->multi = NULL;
         }
-        g_free(s->states[i].orig_buf);
-        s->states[i].orig_buf = NULL;
-    }
-    if (s->multi) {
-        curl_multi_cleanup(s->multi);
-        s->multi = NULL;
     }
-    qemu_mutex_unlock(&s->mutex);
 
     timer_del(&s->timer);
 }
-- 
2.23.0

[PATCH 3/4] block/throttle-groups.c: Use lock guard macros

[Gan Qixin]

Replace manual lock()/unlock() calls with lock guard macros
(QEMU_LOCK_GUARD/WITH_QEMU_LOCK_GUARD) in block/throttle-groups.c.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
 block/throttle-groups.c | 48 ++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index e2f2813c0f..badb93a3be 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -546,7 +546,7 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm,
     tgm->aio_context = ctx;
     qatomic_set(&tgm->restart_pending, 0);
 
-    qemu_mutex_lock(&tg->lock);
+    QEMU_LOCK_GUARD(&tg->lock);
     /* If the ThrottleGroup is new set this ThrottleGroupMember as the token */
     for (i = 0; i < 2; i++) {
         if (!tg->tokens[i]) {
@@ -565,8 +565,6 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm,
     qemu_co_mutex_init(&tgm->throttled_reqs_lock);
     qemu_co_queue_init(&tgm->throttled_reqs[0]);
     qemu_co_queue_init(&tgm->throttled_reqs[1]);
-
-    qemu_mutex_unlock(&tg->lock);
 }
 
 /* Unregister a ThrottleGroupMember from its group, removing it from the list,
@@ -594,25 +592,25 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)
     /* Wait for throttle_group_restart_queue_entry() coroutines to finish */
     AIO_WAIT_WHILE(tgm->aio_context, qatomic_read(&tgm->restart_pending) > 0);
 
-    qemu_mutex_lock(&tg->lock);
-    for (i = 0; i < 2; i++) {
-        assert(tgm->pending_reqs[i] == 0);
-        assert(qemu_co_queue_empty(&tgm->throttled_reqs[i]));
-        assert(!timer_pending(tgm->throttle_timers.timers[i]));
-        if (tg->tokens[i] == tgm) {
-            token = throttle_group_next_tgm(tgm);
-            /* Take care of the case where this is the last tgm in the group */
-            if (token == tgm) {
-                token = NULL;
+    WITH_QEMU_LOCK_GUARD(&tg->lock) {
+        for (i = 0; i < 2; i++) {
+            assert(tgm->pending_reqs[i] == 0);
+            assert(qemu_co_queue_empty(&tgm->throttled_reqs[i]));
+            assert(!timer_pending(tgm->throttle_timers.timers[i]));
+            if (tg->tokens[i] == tgm) {
+                token = throttle_group_next_tgm(tgm);
+                /* Take care of the case where this is the last tgm in the group */
+                if (token == tgm) {
+                    token = NULL;
+                }
+                tg->tokens[i] = token;
             }
-            tg->tokens[i] = token;
         }
-    }
 
-    /* remove the current tgm from the list */
-    QLIST_REMOVE(tgm, round_robin);
-    throttle_timers_destroy(&tgm->throttle_timers);
-    qemu_mutex_unlock(&tg->lock);
+        /* remove the current tgm from the list */
+        QLIST_REMOVE(tgm, round_robin);
+        throttle_timers_destroy(&tgm->throttle_timers);
+    }
 
     throttle_group_unref(&tg->ts);
     tgm->throttle_state = NULL;
@@ -638,14 +636,14 @@ void throttle_group_detach_aio_context(ThrottleGroupMember *tgm)
     assert(qemu_co_queue_empty(&tgm->throttled_reqs[1]));
 
     /* Kick off next ThrottleGroupMember, if necessary */
-    qemu_mutex_lock(&tg->lock);
-    for (i = 0; i < 2; i++) {
-        if (timer_pending(tt->timers[i])) {
-            tg->any_timer_armed[i] = false;
-            schedule_next_request(tgm, i);
+     WITH_QEMU_LOCK_GUARD(&tg->lock) {
+        for (i = 0; i < 2; i++) {
+            if (timer_pending(tt->timers[i])) {
+                tg->any_timer_armed[i] = false;
+                schedule_next_request(tgm, i);
+            }
         }
     }
-    qemu_mutex_unlock(&tg->lock);
 
     throttle_timers_detach_aio_context(tt);
     tgm->aio_context = NULL;
-- 
2.23.0

[PATCH 4/4] block/iscsi.c: Use lock guard macros

[Gan Qixin]

Replace manual lock()/unlock() calls with lock guard macros
(QEMU_LOCK_GUARD/WITH_QEMU_LOCK_GUARD) in block/iscsi.c.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
 block/iscsi.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index e30a7e3606..f5f657b582 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -322,7 +322,7 @@ iscsi_aio_cancel(BlockAIOCB *blockacb)
     IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
     IscsiLun *iscsilun = acb->iscsilun;
 
-    qemu_mutex_lock(&iscsilun->mutex);
+    QEMU_LOCK_GUARD(&iscsilun->mutex);
 
     /* If it was cancelled or completed already, our work is done here */
     if (acb->cancelled || acb->status != -EINPROGRESS) {
@@ -339,8 +339,6 @@ iscsi_aio_cancel(BlockAIOCB *blockacb)
                                          iscsi_abort_task_cb, acb) < 0) {
         qemu_aio_unref(acb); /* since iscsi_abort_task_cb() won't be called */
     }
-
-    qemu_mutex_unlock(&iscsilun->mutex);
 }
 
 static const AIOCBInfo iscsi_aiocb_info = {
@@ -375,22 +373,22 @@ static void iscsi_timed_check_events(void *opaque)
 {
     IscsiLun *iscsilun = opaque;
 
-    qemu_mutex_lock(&iscsilun->mutex);
+    WITH_QEMU_LOCK_GUARD(&iscsilun->mutex) {
+        /* check for timed out requests */
+        iscsi_service(iscsilun->iscsi, 0);
 
-    /* check for timed out requests */
-    iscsi_service(iscsilun->iscsi, 0);
+        if (iscsilun->request_timed_out) {
+            iscsilun->request_timed_out = false;
+            iscsi_reconnect(iscsilun->iscsi);
+        }
 
-    if (iscsilun->request_timed_out) {
-        iscsilun->request_timed_out = false;
-        iscsi_reconnect(iscsilun->iscsi);
+        /*
+         * newer versions of libiscsi may return zero events. Ensure we are
+         * able to return to service once this situation changes.
+         */
+        iscsi_set_events(iscsilun);
     }
 
-    /* newer versions of libiscsi may return zero events. Ensure we are able
-     * to return to service once this situation changes. */
-    iscsi_set_events(iscsilun);
-
-    qemu_mutex_unlock(&iscsilun->mutex);
-
     timer_mod(iscsilun->event_timer,
               qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
 }
-- 
2.23.0

Re: [PATCH 0/4] Use lock guard macros in block

[Markus Armbruster]

Did this fall through the cracks?

Gan Qixin <ganqixin@huawei.com> writes:

> Hi all,
>   I saw some tasks to replace manual lock()/unlock() calls with lock guard macros in BiteSizedTasks.
> I am very interested in this and modified some of the files under block. Could someone help me check the code?
>
> Thanks,
> Gan Qixin
>
> Gan Qixin (4):
>   block/accounting.c: Use lock guard macros
>   block/curl.c: Use lock guard macros
>   block/throttle-groups.c: Use lock guard macros
>   block/iscsi.c: Use lock guard macros
>
>  block/accounting.c      | 32 +++++++++++++--------------
>  block/curl.c            | 28 ++++++++++++------------
>  block/iscsi.c           | 28 +++++++++++-------------
>  block/throttle-groups.c | 48 ++++++++++++++++++++---------------------
>  4 files changed, 65 insertions(+), 71 deletions(-)

Re: [PATCH 0/4] Use lock guard macros in block

[Paolo Bonzini]

On 02/12/20 10:22, Markus Armbruster wrote:
> Did this fall through the cracks?
> 
> Gan Qixin <ganqixin@huawei.com> writes:
> 
>> Hi all,
>>    I saw some tasks to replace manual lock()/unlock() calls with lock guard macros in BiteSizedTasks.
>> I am very interested in this and modified some of the files under block. Could someone help me check the code?
>>
>> Thanks,
>> Gan Qixin
>>
>> Gan Qixin (4):
>>    block/accounting.c: Use lock guard macros
>>    block/curl.c: Use lock guard macros
>>    block/throttle-groups.c: Use lock guard macros
>>    block/iscsi.c: Use lock guard macros
>>
>>   block/accounting.c      | 32 +++++++++++++--------------
>>   block/curl.c            | 28 ++++++++++++------------
>>   block/iscsi.c           | 28 +++++++++++-------------
>>   block/throttle-groups.c | 48 ++++++++++++++++++++---------------------
>>   4 files changed, 65 insertions(+), 71 deletions(-)
> 

Yes, it did.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [PATCH 4/4] block/iscsi.c: Use lock guard macros

[Kevin Wolf]

Am 09.11.2020 um 16:43 hat Gan Qixin geschrieben:
> Replace manual lock()/unlock() calls with lock guard macros
> (QEMU_LOCK_GUARD/WITH_QEMU_LOCK_GUARD) in block/iscsi.c.
> 
> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
> ---
>  block/iscsi.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index e30a7e3606..f5f657b582 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -322,7 +322,7 @@ iscsi_aio_cancel(BlockAIOCB *blockacb)
>      IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
>      IscsiLun *iscsilun = acb->iscsilun;
>  
> -    qemu_mutex_lock(&iscsilun->mutex);
> +    QEMU_LOCK_GUARD(&iscsilun->mutex);
>  
>      /* If it was cancelled or completed already, our work is done here */
>      if (acb->cancelled || acb->status != -EINPROGRESS) {
           qemu_mutex_unlock(&iscsilun->mutex);
           return;
       }

I don't think this qemu_mutex_unlock() is right any more now.

Kevin

Re: [PATCH 3/4] block/throttle-groups.c: Use lock guard macros

[Kevin Wolf]

Am 09.11.2020 um 16:43 hat Gan Qixin geschrieben:
> Replace manual lock()/unlock() calls with lock guard macros
> (QEMU_LOCK_GUARD/WITH_QEMU_LOCK_GUARD) in block/throttle-groups.c.
> 
> Signed-off-by: Gan Qixin <ganqixin@huawei.com>

> @@ -638,14 +636,14 @@ void throttle_group_detach_aio_context(ThrottleGroupMember *tgm)
>      assert(qemu_co_queue_empty(&tgm->throttled_reqs[1]));
>  
>      /* Kick off next ThrottleGroupMember, if necessary */
> -    qemu_mutex_lock(&tg->lock);
> -    for (i = 0; i < 2; i++) {
> -        if (timer_pending(tt->timers[i])) {
> -            tg->any_timer_armed[i] = false;
> -            schedule_next_request(tgm, i);
> +     WITH_QEMU_LOCK_GUARD(&tg->lock) {

Indentation is off.

> +        for (i = 0; i < 2; i++) {
> +            if (timer_pending(tt->timers[i])) {
> +                tg->any_timer_armed[i] = false;
> +                schedule_next_request(tgm, i);
> +            }
>          }
>      }
> -    qemu_mutex_unlock(&tg->lock);
>  
>      throttle_timers_detach_aio_context(tt);
>      tgm->aio_context = NULL;

Kevin

RE: [PATCH 4/4] block/iscsi.c: Use lock guard macros

[ganqixin]

> -----Original Message-----
> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Sent: Wednesday, December 2, 2020 7:12 PM
> To: ganqixin <ganqixin@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org;
> pbonzini@redhat.com; mreitz@redhat.com; stefanha@redhat.com;
> dnbrdsky@gmail.com; Zhanghailiang <zhang.zhanghailiang@huawei.com>;
> Chenqun (kuhn) <kuhn.chenqun@huawei.com>
> Subject: Re: [PATCH 4/4] block/iscsi.c: Use lock guard macros
> 
> Am 09.11.2020 um 16:43 hat Gan Qixin geschrieben:
> > Replace manual lock()/unlock() calls with lock guard macros
> > (QEMU_LOCK_GUARD/WITH_QEMU_LOCK_GUARD) in block/iscsi.c.
> >
> > Signed-off-by: Gan Qixin <ganqixin@huawei.com>
> > ---
> >  block/iscsi.c | 28 +++++++++++++---------------
> >  1 file changed, 13 insertions(+), 15 deletions(-)
> >
> > diff --git a/block/iscsi.c b/block/iscsi.c index
> > e30a7e3606..f5f657b582 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -322,7 +322,7 @@ iscsi_aio_cancel(BlockAIOCB *blockacb)
> >      IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
> >      IscsiLun *iscsilun = acb->iscsilun;
> >
> > -    qemu_mutex_lock(&iscsilun->mutex);
> > +    QEMU_LOCK_GUARD(&iscsilun->mutex);
> >
> >      /* If it was cancelled or completed already, our work is done here */
> >      if (acb->cancelled || acb->status != -EINPROGRESS) {
>            qemu_mutex_unlock(&iscsilun->mutex);
>            return;
>        }
> 
> I don't think this qemu_mutex_unlock() is right any more now.

You are right, I ignored this qemu_mutex_unlock(). I will correct it and resubmit. :)

Thanks,
Gan Qixin

> 
> Kevin