[PATCH v4 0/6] blkdebug: fix racing condition when iterating on

[PATCH v4 0/6] blkdebug: fix racing condition when iterating on

[Emanuele Giuseppe Esposito]

When qemu_coroutine_enter is executed in a loop
(even QEMU_FOREACH_SAFE), the new routine can modify the list,
for example removing an element, causing problem when control
is given back to the caller that continues iterating on the same list. 

Patch 1 solves the issue in blkdebug_debug_resume by restarting
the list walk after every coroutine_enter if list has to be fully iterated.
Patches 2,3,4 aim to fix blkdebug_debug_event by gathering
all actions that the rules make in a counter and invoking 
the respective coroutine_yeld only after processing all requests.

Patch 5-6 are somewhat independent of the others, patch 5 removes the need
of new_state field, and patch 6 adds a lock to
protect rules and suspended_reqs; right now everything works because
it's protected by the AioContext lock.
This is a preparation for the current proposal of removing the AioContext
lock and instead using smaller granularity locks to allow multiple
iothread execution in the same block device.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
v4:
* Patch 5 (new): get rid of new_state and instead use a local variable
* Patch 6: move the state update inside the same guard lock where the new
  one is decided, to have a single critical section and avoid use-before-update. 

Emanuele Giuseppe Esposito (6):
  blkdebug: refactor removal of a suspended request
  blkdebug: move post-resume handling to resume_req_by_tag
  blkdebug: track all actions
  blkdebug: do not suspend in the middle of QLIST_FOREACH_SAFE
  block/blkdebug: remove new_state field and instead use a local
    variable
  blkdebug: protect rules and suspended_reqs with a lock

 block/blkdebug.c | 128 +++++++++++++++++++++++++++++++----------------
 1 file changed, 84 insertions(+), 44 deletions(-)

-- 
2.30.2

[PATCH v4 1/6] blkdebug: refactor removal of a suspended request

[Emanuele Giuseppe Esposito]

Extract to a separate function.  Do not rely on FOREACH_SAFE, which is
only "safe" if the *current* node is removed---not if another node is
removed.  Instead, just walk the entire list from the beginning when
asked to resume all suspended requests with a given tag.

Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/blkdebug.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 2c0b9b0ee8..8f19d991fa 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -793,7 +793,6 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
         printf("blkdebug: Resuming request '%s'\n", r.tag);
     }
 
-    QLIST_REMOVE(&r, next);
     g_free(r.tag);
 }
 
@@ -869,25 +868,35 @@ static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
     return 0;
 }
 
-static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag)
+static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all)
 {
-    BDRVBlkdebugState *s = bs->opaque;
-    BlkdebugSuspendedReq *r, *next;
+    BlkdebugSuspendedReq *r;
 
-    QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, next) {
+retry:
+    QLIST_FOREACH(r, &s->suspended_reqs, next) {
         if (!strcmp(r->tag, tag)) {
+            QLIST_REMOVE(r, next);
             qemu_coroutine_enter(r->co);
+            if (all) {
+                goto retry;
+            }
             return 0;
         }
     }
     return -ENOENT;
 }
 
+static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+
+    return resume_req_by_tag(s, tag, false);
+}
+
 static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs,
                                             const char *tag)
 {
     BDRVBlkdebugState *s = bs->opaque;
-    BlkdebugSuspendedReq *r, *r_next;
     BlkdebugRule *rule, *next;
     int i, ret = -ENOENT;
 
@@ -900,11 +909,8 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs,
             }
         }
     }
-    QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, r_next) {
-        if (!strcmp(r->tag, tag)) {
-            qemu_coroutine_enter(r->co);
-            ret = 0;
-        }
+    if (resume_req_by_tag(s, tag, true) == 0) {
+        ret = 0;
     }
     return ret;
 }
-- 
2.30.2

[PATCH v4 2/6] blkdebug: move post-resume handling to resume_req_by_tag

[Emanuele Giuseppe Esposito]

We want to move qemu_coroutine_yield() after the loop on rules,
because QLIST_FOREACH_SAFE is wrong if the rule list is modified
while the coroutine has yielded.  Therefore move the suspended
request to the heap and clean it up from the remove side.
All that is left is for blkdebug_debug_event to handle the
yielding.

Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/blkdebug.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 8f19d991fa..e37f999254 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -775,25 +775,20 @@ static void blkdebug_close(BlockDriverState *bs)
 static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
 {
     BDRVBlkdebugState *s = bs->opaque;
-    BlkdebugSuspendedReq r;
+    BlkdebugSuspendedReq *r;
 
-    r = (BlkdebugSuspendedReq) {
-        .co         = qemu_coroutine_self(),
-        .tag        = g_strdup(rule->options.suspend.tag),
-    };
+    r = g_new(BlkdebugSuspendedReq, 1);
+
+    r->co         = qemu_coroutine_self();
+    r->tag        = g_strdup(rule->options.suspend.tag);
 
     remove_rule(rule);
-    QLIST_INSERT_HEAD(&s->suspended_reqs, &r, next);
+    QLIST_INSERT_HEAD(&s->suspended_reqs, r, next);
 
     if (!qtest_enabled()) {
-        printf("blkdebug: Suspended request '%s'\n", r.tag);
+        printf("blkdebug: Suspended request '%s'\n", r->tag);
     }
     qemu_coroutine_yield();
-    if (!qtest_enabled()) {
-        printf("blkdebug: Resuming request '%s'\n", r.tag);
-    }
-
-    g_free(r.tag);
 }
 
 static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
@@ -875,8 +870,18 @@ static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all)
 retry:
     QLIST_FOREACH(r, &s->suspended_reqs, next) {
         if (!strcmp(r->tag, tag)) {
+            Coroutine *co = r->co;
+
+            if (!qtest_enabled()) {
+                printf("blkdebug: Resuming request '%s'\n", r->tag);
+            }
+
             QLIST_REMOVE(r, next);
-            qemu_coroutine_enter(r->co);
+            g_free(r->tag);
+            g_free(r);
+
+            qemu_coroutine_enter(co);
+
             if (all) {
                 goto retry;
             }
-- 
2.30.2

[PATCH v4 3/6] blkdebug: track all actions

[Emanuele Giuseppe Esposito]

Add a counter for each action that a rule can trigger.
This is mainly used to keep track of how many coroutine_yield()
we need to perform after processing all rules in the list.

Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/blkdebug.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index e37f999254..388b5ed615 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -74,6 +74,7 @@ enum {
     ACTION_INJECT_ERROR,
     ACTION_SET_STATE,
     ACTION_SUSPEND,
+    ACTION__MAX,
 };
 
 typedef struct BlkdebugRule {
@@ -791,22 +792,22 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
     qemu_coroutine_yield();
 }
 
-static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
-    bool injected)
+static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
+                         int *action_count)
 {
     BDRVBlkdebugState *s = bs->opaque;
 
     /* Only process rules for the current state */
     if (rule->state && rule->state != s->state) {
-        return injected;
+        return;
     }
 
     /* Take the action */
+    action_count[rule->action]++;
     switch (rule->action) {
     case ACTION_INJECT_ERROR:
-        if (!injected) {
+        if (action_count[ACTION_INJECT_ERROR] == 1) {
             QSIMPLEQ_INIT(&s->active_rules);
-            injected = true;
         }
         QSIMPLEQ_INSERT_HEAD(&s->active_rules, rule, active_next);
         break;
@@ -819,21 +820,19 @@ static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
         suspend_request(bs, rule);
         break;
     }
-    return injected;
 }
 
 static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
 {
     BDRVBlkdebugState *s = bs->opaque;
     struct BlkdebugRule *rule, *next;
-    bool injected;
+    int actions_count[ACTION__MAX] = { 0 };
 
     assert((int)event >= 0 && event < BLKDBG__MAX);
 
-    injected = false;
     s->new_state = s->state;
     QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
-        injected = process_rule(bs, rule, injected);
+        process_rule(bs, rule, actions_count);
     }
     s->state = s->new_state;
 }
-- 
2.30.2

[PATCH v4 4/6] blkdebug: do not suspend in the middle of QLIST_FOREACH_SAFE

[Emanuele Giuseppe Esposito]

That would be unsafe in case a rule other than the current one
is removed while the coroutine has yielded.
Keep FOREACH_SAFE because suspend_request deletes the current rule.

After this patch, *all* matching rules are deleted before suspending
the coroutine, rather than just one.
This doesn't affect the existing testcases.

Use actions_count to see how many yield to issue.

Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/blkdebug.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 388b5ed615..dffd869b32 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -789,7 +789,6 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
     if (!qtest_enabled()) {
         printf("blkdebug: Suspended request '%s'\n", r->tag);
     }
-    qemu_coroutine_yield();
 }
 
 static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
@@ -834,6 +833,12 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
     QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
         process_rule(bs, rule, actions_count);
     }
+
+    while (actions_count[ACTION_SUSPEND] > 0) {
+        qemu_coroutine_yield();
+        actions_count[ACTION_SUSPEND]--;
+    }
+
     s->state = s->new_state;
 }
 
-- 
2.30.2

[PATCH v4 5/6] block/blkdebug: remove new_state field and instead use a local variable

[Emanuele Giuseppe Esposito]

There seems to be no benefit in using a field. Replace it with a local
variable.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/blkdebug.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index dffd869b32..d597753139 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -40,7 +40,6 @@
 
 typedef struct BDRVBlkdebugState {
     int state;
-    int new_state;
     uint64_t align;
     uint64_t max_transfer;
     uint64_t opt_write_zero;
@@ -792,7 +791,7 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
 }
 
 static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
-                         int *action_count)
+                         int *action_count, int *new_state)
 {
     BDRVBlkdebugState *s = bs->opaque;
 
@@ -812,7 +811,8 @@ static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
         break;
 
     case ACTION_SET_STATE:
-        s->new_state = rule->options.set_state.new_state;
+        assert(new_state != NULL);
+        *new_state = rule->options.set_state.new_state;
         break;
 
     case ACTION_SUSPEND:
@@ -825,13 +825,14 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
 {
     BDRVBlkdebugState *s = bs->opaque;
     struct BlkdebugRule *rule, *next;
+    int new_state;
     int actions_count[ACTION__MAX] = { 0 };
 
     assert((int)event >= 0 && event < BLKDBG__MAX);
 
-    s->new_state = s->state;
+    new_state = s->state;
     QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
-        process_rule(bs, rule, actions_count);
+        process_rule(bs, rule, actions_count, &new_state);
     }
 
     while (actions_count[ACTION_SUSPEND] > 0) {
@@ -839,7 +840,7 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
         actions_count[ACTION_SUSPEND]--;
     }
 
-    s->state = s->new_state;
+    s->state = new_state;
 }
 
 static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
-- 
2.30.2

[PATCH v4 6/6] blkdebug: protect rules and suspended_reqs with a lock

[Emanuele Giuseppe Esposito]

First, categorize the structure fields to identify what needs
to be protected and what doesn't.

We essentially need to protect only .state, and the 3 lists in
BDRVBlkdebugState.

Then, add the lock and mark the functions accordingly.

Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/blkdebug.c | 46 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index d597753139..ac3799f739 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -38,24 +38,27 @@
 #include "qapi/qobject-input-visitor.h"
 #include "sysemu/qtest.h"
 
+/* All APIs are thread-safe */
+
 typedef struct BDRVBlkdebugState {
-    int state;
+    /* IN: initialized in blkdebug_open() and never changed */
     uint64_t align;
     uint64_t max_transfer;
     uint64_t opt_write_zero;
     uint64_t max_write_zero;
     uint64_t opt_discard;
     uint64_t max_discard;
-
+    char *config_file; /* For blkdebug_refresh_filename() */
+    /* initialized in blkdebug_parse_perms() */
     uint64_t take_child_perms;
     uint64_t unshare_child_perms;
 
-    /* For blkdebug_refresh_filename() */
-    char *config_file;
-
+    /* State. Protected by lock */
+    int state;
     QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX];
     QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
     QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
+    QemuMutex lock;
 } BDRVBlkdebugState;
 
 typedef struct BlkdebugAIOCB {
@@ -64,6 +67,7 @@ typedef struct BlkdebugAIOCB {
 } BlkdebugAIOCB;
 
 typedef struct BlkdebugSuspendedReq {
+    /* IN: initialized in suspend_request() */
     Coroutine *co;
     char *tag;
     QLIST_ENTRY(BlkdebugSuspendedReq) next;
@@ -77,6 +81,7 @@ enum {
 };
 
 typedef struct BlkdebugRule {
+    /* IN: initialized in add_rule() or blkdebug_debug_breakpoint() */
     BlkdebugEvent event;
     int action;
     int state;
@@ -244,11 +249,14 @@ static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
     };
 
     /* Add the rule */
+    qemu_mutex_lock(&s->lock);
     QLIST_INSERT_HEAD(&s->rules[event], rule, next);
+    qemu_mutex_unlock(&s->lock);
 
     return 0;
 }
 
+/* Called with lock held or from .bdrv_close */
 static void remove_rule(BlkdebugRule *rule)
 {
     switch (rule->action) {
@@ -467,6 +475,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;
     uint64_t align;
 
+    qemu_mutex_init(&s->lock);
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     if (!qemu_opts_absorb_qdict(opts, options, errp)) {
         ret = -EINVAL;
@@ -567,6 +576,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     ret = 0;
 out:
     if (ret < 0) {
+        qemu_mutex_destroy(&s->lock);
         g_free(s->config_file);
     }
     qemu_opts_del(opts);
@@ -581,6 +591,7 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     int error;
     bool immediately;
 
+    qemu_mutex_lock(&s->lock);
     QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
         uint64_t inject_offset = rule->options.inject.offset;
 
@@ -594,6 +605,7 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     }
 
     if (!rule || !rule->options.inject.error) {
+        qemu_mutex_unlock(&s->lock);
         return 0;
     }
 
@@ -605,6 +617,7 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         remove_rule(rule);
     }
 
+    qemu_mutex_unlock(&s->lock);
     if (!immediately) {
         aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
         qemu_coroutine_yield();
@@ -770,8 +783,10 @@ static void blkdebug_close(BlockDriverState *bs)
     }
 
     g_free(s->config_file);
+    qemu_mutex_destroy(&s->lock);
 }
 
+/* Called with lock held.  */
 static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
 {
     BDRVBlkdebugState *s = bs->opaque;
@@ -790,6 +805,7 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
     }
 }
 
+/* Called with lock held.  */
 static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
                          int *action_count, int *new_state)
 {
@@ -830,17 +846,18 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
 
     assert((int)event >= 0 && event < BLKDBG__MAX);
 
-    new_state = s->state;
-    QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
-        process_rule(bs, rule, actions_count, &new_state);
+    WITH_QEMU_LOCK_GUARD(&s->lock) {
+        new_state = s->state;
+        QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
+            process_rule(bs, rule, actions_count, &new_state);
+        }
+        s->state = new_state;
     }
 
     while (actions_count[ACTION_SUSPEND] > 0) {
         qemu_coroutine_yield();
         actions_count[ACTION_SUSPEND]--;
     }
-
-    s->state = new_state;
 }
 
 static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
@@ -863,11 +880,14 @@ static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
         .options.suspend.tag = g_strdup(tag),
     };
 
+    qemu_mutex_lock(&s->lock);
     QLIST_INSERT_HEAD(&s->rules[blkdebug_event], rule, next);
+    qemu_mutex_unlock(&s->lock);
 
     return 0;
 }
 
+/* Called with lock held. May temporarily release lock. */
 static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all)
 {
     BlkdebugSuspendedReq *r;
@@ -885,7 +905,9 @@ retry:
             g_free(r->tag);
             g_free(r);
 
+            qemu_mutex_unlock(&s->lock);
             qemu_coroutine_enter(co);
+            qemu_mutex_lock(&s->lock);
 
             if (all) {
                 goto retry;
@@ -899,7 +921,7 @@ retry:
 static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag)
 {
     BDRVBlkdebugState *s = bs->opaque;
-
+    QEMU_LOCK_GUARD(&s->lock);
     return resume_req_by_tag(s, tag, false);
 }
 
@@ -910,6 +932,7 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs,
     BlkdebugRule *rule, *next;
     int i, ret = -ENOENT;
 
+    QEMU_LOCK_GUARD(&s->lock);
     for (i = 0; i < BLKDBG__MAX; i++) {
         QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) {
             if (rule->action == ACTION_SUSPEND &&
@@ -930,6 +953,7 @@ static bool blkdebug_debug_is_suspended(BlockDriverState *bs, const char *tag)
     BDRVBlkdebugState *s = bs->opaque;
     BlkdebugSuspendedReq *r;
 
+    QEMU_LOCK_GUARD(&s->lock);
     QLIST_FOREACH(r, &s->suspended_reqs, next) {
         if (!strcmp(r->tag, tag)) {
             return true;
-- 
2.30.2

Re: [PATCH v4 1/6] blkdebug: refactor removal of a suspended request

[Eric Blake]

On Fri, Jun 04, 2021 at 12:07:36PM +0200, Emanuele Giuseppe Esposito wrote:
> Extract to a separate function.  Do not rely on FOREACH_SAFE, which is
> only "safe" if the *current* node is removed---not if another node is
> removed.  Instead, just walk the entire list from the beginning when
> asked to resume all suspended requests with a given tag.
> 
> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/blkdebug.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 2c0b9b0ee8..8f19d991fa 100644
> --- a/block/blkdebug.c

> -static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag)
> +static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all)
>  {
> -    BDRVBlkdebugState *s = bs->opaque;
> -    BlkdebugSuspendedReq *r, *next;
> +    BlkdebugSuspendedReq *r;
>  
> -    QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, next) {
> +retry:
> +    QLIST_FOREACH(r, &s->suspended_reqs, next) {
>          if (!strcmp(r->tag, tag)) {
> +            QLIST_REMOVE(r, next);

Isn't the whole point of using QLIST_FOREACH_SAFE the ability to call
QLIST_REMOVE on an element in that list while still iterating?

>              qemu_coroutine_enter(r->co);
> +            if (all) {
> +                goto retry;
> +            }
>              return 0;

Oh, I see - you abandon the iteration in all control flow paths, so
the simpler loop is still okay.  Still, this confused me enough on
first read that it may be worth a comment, maybe:

/* No need for _SAFE, because iteration stops on first hit */

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v4 5/6] block/blkdebug: remove new_state field and instead use a local variable

[Vladimir Sementsov-Ogievskiy]

04.06.2021 13:07, Emanuele Giuseppe Esposito wrote:
> There seems to be no benefit in using a field. Replace it with a local
> variable.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/blkdebug.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index dffd869b32..d597753139 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -40,7 +40,6 @@
>   
>   typedef struct BDRVBlkdebugState {
>       int state;
> -    int new_state;
>       uint64_t align;
>       uint64_t max_transfer;
>       uint64_t opt_write_zero;
> @@ -792,7 +791,7 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
>   }
>   
>   static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
> -                         int *action_count)
> +                         int *action_count, int *new_state)
>   {
>       BDRVBlkdebugState *s = bs->opaque;
>   
> @@ -812,7 +811,8 @@ static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
>           break;
>   
>       case ACTION_SET_STATE:
> -        s->new_state = rule->options.set_state.new_state;
> +        assert(new_state != NULL);

you don't need additional assertion: crash on dereferencing NULL pointer is not less clear

anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> +        *new_state = rule->options.set_state.new_state;
>           break;
>   
>       case ACTION_SUSPEND:
> @@ -825,13 +825,14 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
>   {
>       BDRVBlkdebugState *s = bs->opaque;
>       struct BlkdebugRule *rule, *next;
> +    int new_state;
>       int actions_count[ACTION__MAX] = { 0 };
>   
>       assert((int)event >= 0 && event < BLKDBG__MAX);
>   
> -    s->new_state = s->state;
> +    new_state = s->state;
>       QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
> -        process_rule(bs, rule, actions_count);
> +        process_rule(bs, rule, actions_count, &new_state);
>       }
>   
>       while (actions_count[ACTION_SUSPEND] > 0) {
> @@ -839,7 +840,7 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
>           actions_count[ACTION_SUSPEND]--;
>       }
>   
> -    s->state = s->new_state;
> +    s->state = new_state;
>   }
>   
>   static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 6/6] blkdebug: protect rules and suspended_reqs with a lock

[Vladimir Sementsov-Ogievskiy]

04.06.2021 13:07, Emanuele Giuseppe Esposito wrote:
> First, categorize the structure fields to identify what needs
> to be protected and what doesn't.
> 
> We essentially need to protect only .state, and the 3 lists in
> BDRVBlkdebugState.
> 
> Then, add the lock and mark the functions accordingly.
> 
> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/blkdebug.c | 46 +++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index d597753139..ac3799f739 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -38,24 +38,27 @@
>   #include "qapi/qobject-input-visitor.h"
>   #include "sysemu/qtest.h"
>   
> +/* All APIs are thread-safe */
> +
>   typedef struct BDRVBlkdebugState {
> -    int state;
> +    /* IN: initialized in blkdebug_open() and never changed */
>       uint64_t align;
>       uint64_t max_transfer;
>       uint64_t opt_write_zero;
>       uint64_t max_write_zero;
>       uint64_t opt_discard;
>       uint64_t max_discard;
> -
> +    char *config_file; /* For blkdebug_refresh_filename() */
> +    /* initialized in blkdebug_parse_perms() */
>       uint64_t take_child_perms;
>       uint64_t unshare_child_perms;
>   
> -    /* For blkdebug_refresh_filename() */
> -    char *config_file;
> -
> +    /* State. Protected by lock */
> +    int state;
>       QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX];
>       QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
>       QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
> +    QemuMutex lock;
>   } BDRVBlkdebugState;
>   
>   typedef struct BlkdebugAIOCB {
> @@ -64,6 +67,7 @@ typedef struct BlkdebugAIOCB {
>   } BlkdebugAIOCB;
>   
>   typedef struct BlkdebugSuspendedReq {
> +    /* IN: initialized in suspend_request() */
>       Coroutine *co;
>       char *tag;

@next is part of *suspended_reqs list (in a manner), so it should be protected by lock

>       QLIST_ENTRY(BlkdebugSuspendedReq) next;
> @@ -77,6 +81,7 @@ enum {
>   };
>   
>   typedef struct BlkdebugRule {
> +    /* IN: initialized in add_rule() or blkdebug_debug_breakpoint() */
>       BlkdebugEvent event;
>       int action;
>       int state;

as well as @next and @active_next here.

> @@ -244,11 +249,14 @@ static int add_rule(void *opaque, QemuOpts *opts, Error **errp)


>       };
>   
>       /* Add the rule */
> +    qemu_mutex_lock(&s->lock);
>       QLIST_INSERT_HEAD(&s->rules[event], rule, next);
> +    qemu_mutex_unlock(&s->lock);
>   

actually, add_rule is called only from .open(), so doesn't need a lock.. But it doesn't hurt.

>       return 0;
>   }
>   
> +/* Called with lock held or from .bdrv_close */
>   static void remove_rule(BlkdebugRule *rule)
>   {
>       switch (rule->action) {
> @@ -467,6 +475,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>       int ret;
>       uint64_t align;
>   
> +    qemu_mutex_init(&s->lock);
>       opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>       if (!qemu_opts_absorb_qdict(opts, options, errp)) {
>           ret = -EINVAL;
> @@ -567,6 +576,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>       ret = 0;
>   out:
>       if (ret < 0) {
> +        qemu_mutex_destroy(&s->lock);
>           g_free(s->config_file);
>       }
>       qemu_opts_del(opts);
> @@ -581,6 +591,7 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
>       int error;
>       bool immediately;
>   
> +    qemu_mutex_lock(&s->lock);
>       QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
>           uint64_t inject_offset = rule->options.inject.offset;
>   
> @@ -594,6 +605,7 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
>       }
>   
>       if (!rule || !rule->options.inject.error) {
> +        qemu_mutex_unlock(&s->lock);
>           return 0;
>       }
>   
> @@ -605,6 +617,7 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
>           remove_rule(rule);
>       }
>   
> +    qemu_mutex_unlock(&s->lock);
>       if (!immediately) {
>           aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
>           qemu_coroutine_yield();
> @@ -770,8 +783,10 @@ static void blkdebug_close(BlockDriverState *bs)
>       }
>   
>       g_free(s->config_file);
> +    qemu_mutex_destroy(&s->lock);
>   }
>   
> +/* Called with lock held.  */
>   static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
>   {
>       BDRVBlkdebugState *s = bs->opaque;
> @@ -790,6 +805,7 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
>       }
>   }
>   
> +/* Called with lock held.  */
>   static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
>                            int *action_count, int *new_state)
>   {
> @@ -830,17 +846,18 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
>   
>       assert((int)event >= 0 && event < BLKDBG__MAX);
>   
> -    new_state = s->state;
> -    QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
> -        process_rule(bs, rule, actions_count, &new_state);
> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
> +        new_state = s->state;
> +        QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
> +            process_rule(bs, rule, actions_count, &new_state);
> +        }
> +        s->state = new_state;
>       }
>   
>       while (actions_count[ACTION_SUSPEND] > 0) {
>           qemu_coroutine_yield();
>           actions_count[ACTION_SUSPEND]--;
>       }
> -
> -    s->state = new_state;

Not sure, are all existing users prepared to state update moved to be before actual suspend. But that looks better and as we discussed is safer. So, if all iotests pass, it's OK.

>   }
>   
>   static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
> @@ -863,11 +880,14 @@ static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
>           .options.suspend.tag = g_strdup(tag),
>       };
>   
> +    qemu_mutex_lock(&s->lock);
>       QLIST_INSERT_HEAD(&s->rules[blkdebug_event], rule, next);
> +    qemu_mutex_unlock(&s->lock);
>   
>       return 0;
>   }
>   
> +/* Called with lock held. May temporarily release lock. */
>   static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all)
>   {
>       BlkdebugSuspendedReq *r;
> @@ -885,7 +905,9 @@ retry:
>               g_free(r->tag);
>               g_free(r);
>   
> +            qemu_mutex_unlock(&s->lock);
>               qemu_coroutine_enter(co);
> +            qemu_mutex_lock(&s->lock);
>   
>               if (all) {
>                   goto retry;
> @@ -899,7 +921,7 @@ retry:
>   static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag)
>   {
>       BDRVBlkdebugState *s = bs->opaque;
> -
> +    QEMU_LOCK_GUARD(&s->lock);
>       return resume_req_by_tag(s, tag, false);
>   }
>   
> @@ -910,6 +932,7 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs,
>       BlkdebugRule *rule, *next;
>       int i, ret = -ENOENT;
>   
> +    QEMU_LOCK_GUARD(&s->lock);
>       for (i = 0; i < BLKDBG__MAX; i++) {
>           QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) {
>               if (rule->action == ACTION_SUSPEND &&
> @@ -930,6 +953,7 @@ static bool blkdebug_debug_is_suspended(BlockDriverState *bs, const char *tag)
>       BDRVBlkdebugState *s = bs->opaque;
>       BlkdebugSuspendedReq *r;
>   
> +    QEMU_LOCK_GUARD(&s->lock);
>       QLIST_FOREACH(r, &s->suspended_reqs, next) {
>           if (!strcmp(r->tag, tag)) {
>               return true;
> 


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


Optional suggestion for additional comments and more use of QEMU_LOCK_GUARD (it looks large because of indentation change):

diff --git a/block/blkdebug.c b/block/blkdebug.c
index ac3799f739..b4f8844e76 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -70,6 +70,8 @@ typedef struct BlkdebugSuspendedReq {
      /* IN: initialized in suspend_request() */
      Coroutine *co;
      char *tag;
+
+    /* List entry protected BDRVBlkdebugState::lock */
      QLIST_ENTRY(BlkdebugSuspendedReq) next;
  } BlkdebugSuspendedReq;
  
@@ -100,6 +102,8 @@ typedef struct BlkdebugRule {
              char *tag;
          } suspend;
      } options;
+
+    /* List entries protected BDRVBlkdebugState::lock */
      QLIST_ENTRY(BlkdebugRule) next;
      QSIMPLEQ_ENTRY(BlkdebugRule) active_next;
  } BlkdebugRule;
@@ -249,9 +253,9 @@ static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
      };
  
      /* Add the rule */
-    qemu_mutex_lock(&s->lock);
-    QLIST_INSERT_HEAD(&s->rules[event], rule, next);
-    qemu_mutex_unlock(&s->lock);
+    WITH_QEMU_LOCK_GUARD(&s->lock) {
+        QLIST_INSERT_HEAD(&s->rules[event], rule, next);
+    }
  
      return 0;
  }
@@ -591,33 +595,32 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
      int error;
      bool immediately;
  
-    qemu_mutex_lock(&s->lock);
-    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
-        uint64_t inject_offset = rule->options.inject.offset;
-
-        if ((inject_offset == -1 ||
-             (bytes && inject_offset >= offset &&
-              inject_offset < offset + bytes)) &&
-            (rule->options.inject.iotype_mask & (1ull << iotype)))
-        {
-            break;
+    WITH_QEMU_LOCK_GUARD(&s->lock) {
+        QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
+            uint64_t inject_offset = rule->options.inject.offset;
+
+            if ((inject_offset == -1 ||
+                 (bytes && inject_offset >= offset &&
+                  inject_offset < offset + bytes)) &&
+                (rule->options.inject.iotype_mask & (1ull << iotype)))
+            {
+                break;
+            }
          }
-    }
  
-    if (!rule || !rule->options.inject.error) {
-        qemu_mutex_unlock(&s->lock);
-        return 0;
-    }
+        if (!rule || !rule->options.inject.error) {
+            return 0;
+        }
  
-    immediately = rule->options.inject.immediately;
-    error = rule->options.inject.error;
+        immediately = rule->options.inject.immediately;
+        error = rule->options.inject.error;
  
-    if (rule->options.inject.once) {
-        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
-        remove_rule(rule);
+        if (rule->options.inject.once) {
+            QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
+            remove_rule(rule);
+        }
      }
  
-    qemu_mutex_unlock(&s->lock);
      if (!immediately) {
          aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
          qemu_coroutine_yield();
@@ -880,9 +883,9 @@ static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
          .options.suspend.tag = g_strdup(tag),
      };
  
-    qemu_mutex_lock(&s->lock);
-    QLIST_INSERT_HEAD(&s->rules[blkdebug_event], rule, next);
-    qemu_mutex_unlock(&s->lock);
+    WITH_QEMU_LOCK_GUARD(&s->lock) {
+        QLIST_INSERT_HEAD(&s->rules[blkdebug_event], rule, next);
+    }
  
      return 0;
  }




-- 
Best regards,
Vladimir

Re: [PATCH v4 6/6] blkdebug: protect rules and suspended_reqs with a lock

[Emanuele Giuseppe Esposito]

On 05/06/2021 17:15, Vladimir Sementsov-Ogievskiy wrote:
> 04.06.2021 13:07, Emanuele Giuseppe Esposito wrote:
>> First, categorize the structure fields to identify what needs
>> to be protected and what doesn't.
>>
>> We essentially need to protect only .state, and the 3 lists in
>> BDRVBlkdebugState.
>>
>> Then, add the lock and mark the functions accordingly.
>>
>> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   block/blkdebug.c | 46 +++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index d597753139..ac3799f739 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -38,24 +38,27 @@
>>   #include "qapi/qobject-input-visitor.h"
>>   #include "sysemu/qtest.h"
>> +/* All APIs are thread-safe */
>> +
>>   typedef struct BDRVBlkdebugState {
>> -    int state;
>> +    /* IN: initialized in blkdebug_open() and never changed */
>>       uint64_t align;
>>       uint64_t max_transfer;
>>       uint64_t opt_write_zero;
>>       uint64_t max_write_zero;
>>       uint64_t opt_discard;
>>       uint64_t max_discard;
>> -
>> +    char *config_file; /* For blkdebug_refresh_filename() */
>> +    /* initialized in blkdebug_parse_perms() */
>>       uint64_t take_child_perms;
>>       uint64_t unshare_child_perms;
>> -    /* For blkdebug_refresh_filename() */
>> -    char *config_file;
>> -
>> +    /* State. Protected by lock */
>> +    int state;
>>       QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX];
>>       QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
>>       QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
>> +    QemuMutex lock;
>>   } BDRVBlkdebugState;
>>   typedef struct BlkdebugAIOCB {
>> @@ -64,6 +67,7 @@ typedef struct BlkdebugAIOCB {
>>   } BlkdebugAIOCB;
>>   typedef struct BlkdebugSuspendedReq {
>> +    /* IN: initialized in suspend_request() */
>>       Coroutine *co;
>>       char *tag;
> 
> @next is part of *suspended_reqs list (in a manner), so it should be 
> protected by lock
> 
>>       QLIST_ENTRY(BlkdebugSuspendedReq) next;
>> @@ -77,6 +81,7 @@ enum {
>>   };
>>   typedef struct BlkdebugRule {
>> +    /* IN: initialized in add_rule() or blkdebug_debug_breakpoint() */
>>       BlkdebugEvent event;
>>       int action;
>>       int state;
> 
> as well as @next and @active_next here.

They all are already protected by the lock, I will just update the 
comments in case I need to re-spin.

[...]

> 
> Optional suggestion for additional comments and more use of 
> QEMU_LOCK_GUARD (it looks large because of indentation change):

Exactly, indentation change. If I recall correctly, you (rightly) 
complained about that in one of my patches (not sure if it was in this 
series).

> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index ac3799f739..b4f8844e76 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -70,6 +70,8 @@ typedef struct BlkdebugSuspendedReq {
>       /* IN: initialized in suspend_request() */
>       Coroutine *co;
>       char *tag;
> +
> +    /* List entry protected BDRVBlkdebugState::lock */

Is this C++ style ok to be used here? I don't think I have seen it used 
in QEMU. But I might be wrong, someone with better style taste and 
experience should comment here. Maybe it's better to have

/* List entry protected BDRVBlkdebugState's lock */

>       QLIST_ENTRY(BlkdebugSuspendedReq) next;
>   } BlkdebugSuspendedReq;
> 
> @@ -100,6 +102,8 @@ typedef struct BlkdebugRule {
>               char *tag;
>           } suspend;
>       } options;
> +
> +    /* List entries protected BDRVBlkdebugState::lock */
>       QLIST_ENTRY(BlkdebugRule) next;
>       QSIMPLEQ_ENTRY(BlkdebugRule) active_next;
>   } BlkdebugRule;
> @@ -249,9 +253,9 @@ static int add_rule(void *opaque, QemuOpts *opts, 
> Error **errp)
>       };
> 
>       /* Add the rule */
> -    qemu_mutex_lock(&s->lock);
> -    QLIST_INSERT_HEAD(&s->rules[event], rule, next);
> -    qemu_mutex_unlock(&s->lock);
> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
> +        QLIST_INSERT_HEAD(&s->rules[event], rule, next);
> +    }
Same lines used, just additional indentation added. For one line it 
might be okay? not sure.
> 
>       return 0;
>   }
> @@ -591,33 +595,32 @@ static int rule_check(BlockDriverState *bs, 
> uint64_t offset, uint64_t bytes,
>       int error;
>       bool immediately;
> 
> -    qemu_mutex_lock(&s->lock);
> -    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
> -        uint64_t inject_offset = rule->options.inject.offset;
> -
> -        if ((inject_offset == -1 ||
> -             (bytes && inject_offset >= offset &&
> -              inject_offset < offset + bytes)) &&
> -            (rule->options.inject.iotype_mask & (1ull << iotype)))
> -        {
> -            break;
> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
> +        QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
> +            uint64_t inject_offset = rule->options.inject.offset;
> +
> +            if ((inject_offset == -1 ||
> +                 (bytes && inject_offset >= offset &&
> +                  inject_offset < offset + bytes)) &&
> +                (rule->options.inject.iotype_mask & (1ull << iotype)))
> +            {
> +                break;
> +            }
>           }
> -    }
> 
> -    if (!rule || !rule->options.inject.error) {
> -        qemu_mutex_unlock(&s->lock);
> -        return 0;
> -    }
> +        if (!rule || !rule->options.inject.error) {
> +            return 0;
> +        }
> 
> -    immediately = rule->options.inject.immediately;
> -    error = rule->options.inject.error;
> +        immediately = rule->options.inject.immediately;
> +        error = rule->options.inject.error;
> 
> -    if (rule->options.inject.once) {
> -        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, 
> active_next);
> -        remove_rule(rule);
> +        if (rule->options.inject.once) {
> +            QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, 
> active_next);
> +            remove_rule(rule);
> +        }
>       }
> 
> -    qemu_mutex_unlock(&s->lock);

Too much indentation added for a couple of lock/unlock IMO.

>       if (!immediately) {
>           aio_co_schedule(qemu_get_current_aio_context(), 
> qemu_coroutine_self());
>           qemu_coroutine_yield();
> @@ -880,9 +883,9 @@ static int 
> blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
>           .options.suspend.tag = g_strdup(tag),
>       };
> 
> -    qemu_mutex_lock(&s->lock);
> -    QLIST_INSERT_HEAD(&s->rules[blkdebug_event], rule, next);
> -    qemu_mutex_unlock(&s->lock);
> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
> +        QLIST_INSERT_HEAD(&s->rules[blkdebug_event], rule, next);
> +    }

Same as above.

Thank you,
Emanuele

Re: [PATCH v4 6/6] blkdebug: protect rules and suspended_reqs with a lock

[Vladimir Sementsov-Ogievskiy]

05.06.2021 20:53, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 05/06/2021 17:15, Vladimir Sementsov-Ogievskiy wrote:
>> 04.06.2021 13:07, Emanuele Giuseppe Esposito wrote:
>>> First, categorize the structure fields to identify what needs
>>> to be protected and what doesn't.
>>>
>>> We essentially need to protect only .state, and the 3 lists in
>>> BDRVBlkdebugState.
>>>
>>> Then, add the lock and mark the functions accordingly.
>>>
>>> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>>   block/blkdebug.c | 46 +++++++++++++++++++++++++++++++++++-----------
>>>   1 file changed, 35 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>>> index d597753139..ac3799f739 100644
>>> --- a/block/blkdebug.c
>>> +++ b/block/blkdebug.c
>>> @@ -38,24 +38,27 @@
>>>   #include "qapi/qobject-input-visitor.h"
>>>   #include "sysemu/qtest.h"
>>> +/* All APIs are thread-safe */
>>> +
>>>   typedef struct BDRVBlkdebugState {
>>> -    int state;
>>> +    /* IN: initialized in blkdebug_open() and never changed */
>>>       uint64_t align;
>>>       uint64_t max_transfer;
>>>       uint64_t opt_write_zero;
>>>       uint64_t max_write_zero;
>>>       uint64_t opt_discard;
>>>       uint64_t max_discard;
>>> -
>>> +    char *config_file; /* For blkdebug_refresh_filename() */
>>> +    /* initialized in blkdebug_parse_perms() */
>>>       uint64_t take_child_perms;
>>>       uint64_t unshare_child_perms;
>>> -    /* For blkdebug_refresh_filename() */
>>> -    char *config_file;
>>> -
>>> +    /* State. Protected by lock */
>>> +    int state;
>>>       QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX];
>>>       QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
>>>       QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
>>> +    QemuMutex lock;
>>>   } BDRVBlkdebugState;
>>>   typedef struct BlkdebugAIOCB {
>>> @@ -64,6 +67,7 @@ typedef struct BlkdebugAIOCB {
>>>   } BlkdebugAIOCB;
>>>   typedef struct BlkdebugSuspendedReq {
>>> +    /* IN: initialized in suspend_request() */
>>>       Coroutine *co;
>>>       char *tag;
>>
>> @next is part of *suspended_reqs list (in a manner), so it should be protected by lock
>>
>>>       QLIST_ENTRY(BlkdebugSuspendedReq) next;
>>> @@ -77,6 +81,7 @@ enum {
>>>   };
>>>   typedef struct BlkdebugRule {
>>> +    /* IN: initialized in add_rule() or blkdebug_debug_breakpoint() */
>>>       BlkdebugEvent event;
>>>       int action;
>>>       int state;
>>
>> as well as @next and @active_next here.
> 
> They all are already protected by the lock, I will just update the comments in case I need to re-spin.
> 
> [...]
> 
>>
>> Optional suggestion for additional comments and more use of QEMU_LOCK_GUARD (it looks large because of indentation change):
> 
> Exactly, indentation change. If I recall correctly, you (rightly) complained about that in one of my patches (not sure if it was in this series).

Probably here, indentation doesn't become so big :)

Maximum is

WITH_ () {
   FOR {
      if {
         ***

And this if contains only one simple "break".

Of course, that's a kind of taste. I hope I was not too insistent that past time.

> 
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index ac3799f739..b4f8844e76 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -70,6 +70,8 @@ typedef struct BlkdebugSuspendedReq {
>>       /* IN: initialized in suspend_request() */
>>       Coroutine *co;
>>       char *tag;
>> +
>> +    /* List entry protected BDRVBlkdebugState::lock */
> 
> Is this C++ style ok to be used here? I don't think I have seen it used in QEMU. But I might be wrong, someone with better style taste and experience should comment here. Maybe it's better to have
> 
> /* List entry protected BDRVBlkdebugState's lock */

OK for me, I don't care)

Hmm, looking at git log, I see :: syntax in my commits. And not only my.

> 
>>       QLIST_ENTRY(BlkdebugSuspendedReq) next;
>>   } BlkdebugSuspendedReq;
>>
>> @@ -100,6 +102,8 @@ typedef struct BlkdebugRule {
>>               char *tag;
>>           } suspend;
>>       } options;
>> +
>> +    /* List entries protected BDRVBlkdebugState::lock */
>>       QLIST_ENTRY(BlkdebugRule) next;
>>       QSIMPLEQ_ENTRY(BlkdebugRule) active_next;
>>   } BlkdebugRule;
>> @@ -249,9 +253,9 @@ static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
>>       };
>>
>>       /* Add the rule */
>> -    qemu_mutex_lock(&s->lock);
>> -    QLIST_INSERT_HEAD(&s->rules[event], rule, next);
>> -    qemu_mutex_unlock(&s->lock);
>> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
>> +        QLIST_INSERT_HEAD(&s->rules[event], rule, next);
>> +    }
> Same lines used, just additional indentation added. For one line it might be okay? not sure.

Same three lines, but don't need to call _unlock()..

I think, for last time I just get used to the thought that WITH_QEMU_LOCK_GUARD(){} is a good thing.

Here it's really doesn't make real sense. So, take the suggestion only if you like it, my r-b stands without it as well.

>>
>>       return 0;
>>   }
>> @@ -591,33 +595,32 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
>>       int error;
>>       bool immediately;
>>
>> -    qemu_mutex_lock(&s->lock);
>> -    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
>> -        uint64_t inject_offset = rule->options.inject.offset;
>> -
>> -        if ((inject_offset == -1 ||
>> -             (bytes && inject_offset >= offset &&
>> -              inject_offset < offset + bytes)) &&
>> -            (rule->options.inject.iotype_mask & (1ull << iotype)))
>> -        {
>> -            break;
>> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
>> +        QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
>> +            uint64_t inject_offset = rule->options.inject.offset;
>> +
>> +            if ((inject_offset == -1 ||
>> +                 (bytes && inject_offset >= offset &&
>> +                  inject_offset < offset + bytes)) &&
>> +                (rule->options.inject.iotype_mask & (1ull << iotype)))
>> +            {
>> +                break;
>> +            }
>>           }
>> -    }
>>
>> -    if (!rule || !rule->options.inject.error) {
>> -        qemu_mutex_unlock(&s->lock);
>> -        return 0;
>> -    }
>> +        if (!rule || !rule->options.inject.error) {
>> +            return 0;
>> +        }
>>
>> -    immediately = rule->options.inject.immediately;
>> -    error = rule->options.inject.error;
>> +        immediately = rule->options.inject.immediately;
>> +        error = rule->options.inject.error;
>>
>> -    if (rule->options.inject.once) {
>> -        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
>> -        remove_rule(rule);
>> +        if (rule->options.inject.once) {
>> +            QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
>> +            remove_rule(rule);
>> +        }
>>       }
>>
>> -    qemu_mutex_unlock(&s->lock);
> 
> Too much indentation added for a couple of lock/unlock IMO.
> 
>>       if (!immediately) {
>>           aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
>>           qemu_coroutine_yield();
>> @@ -880,9 +883,9 @@ static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
>>           .options.suspend.tag = g_strdup(tag),
>>       };
>>
>> -    qemu_mutex_lock(&s->lock);
>> -    QLIST_INSERT_HEAD(&s->rules[blkdebug_event], rule, next);
>> -    qemu_mutex_unlock(&s->lock);
>> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
>> +        QLIST_INSERT_HEAD(&s->rules[blkdebug_event], rule, next);
>> +    }
> 
> Same as above.
> 
> Thank you,
> Emanuele
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 1/6] blkdebug: refactor removal of a suspended request

[Paolo Bonzini]

On 04/06/21 18:16, Eric Blake wrote:
> On Fri, Jun 04, 2021 at 12:07:36PM +0200, Emanuele Giuseppe Esposito wrote:
>> Extract to a separate function.  Do not rely on FOREACH_SAFE, which is
>> only "safe" if the *current* node is removed---not if another node is
>> removed.  Instead, just walk the entire list from the beginning when
>> asked to resume all suspended requests with a given tag.
>>   
>> -    QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, next) {
>> +retry:
>> +    QLIST_FOREACH(r, &s->suspended_reqs, next) {
>>           if (!strcmp(r->tag, tag)) {
>> +            QLIST_REMOVE(r, next);
> 
> Isn't the whole point of using QLIST_FOREACH_SAFE the ability to call
> QLIST_REMOVE on an element in that list while still iterating?
> 
>>               qemu_coroutine_enter(r->co);
>> +            if (all) {
>> +                goto retry;
>> +            }
>>               return 0;
> 
> Oh, I see - you abandon the iteration in all control flow paths, so
> the simpler loop is still okay.  Still, this confused me enough on
> first read that it may be worth a comment, maybe:
> 
> /* No need for _SAFE, because iteration stops on first hit */

This is a bit confusing too because it sounds like not using _SAFE is an 
optimization, but it's actually wrong (see commit message).

Paolo

Re: [PATCH v4 6/6] blkdebug: protect rules and suspended_reqs with a lock

[Paolo Bonzini]

On 04/06/21 12:07, Emanuele Giuseppe Esposito wrote:
> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
> +        new_state = s->state;
> +        QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
> +            process_rule(bs, rule, actions_count, &new_state);
> +        }
> +        s->state = new_state;
>       }
>   
>       while (actions_count[ACTION_SUSPEND] > 0) {
>           qemu_coroutine_yield();
>           actions_count[ACTION_SUSPEND]--;
>       }
> -
> -    s->state = new_state;

This changes the semantics by moving the state change *before* the yield 
instead of after:

- before the series, the new state was assigned after all yields (and 
could be overwritten by other coroutines during the yield).  Until patch 
4, the situation is more or less the same even though the ordering 
changed in the processing of actions (suspend actions are processed last).

- with patch 5 new_state became a local variable, so it couldn't be 
overwritten by the yields

- now it is a local variable and is assigned before the yields.  The 
yields can write s->state just like before.

So it's a bit messy.  Moving s->state = new_state before the yields 
makes sense, but I'd do that in patch 5 to avoid the temporary change in 
semantics.

Paolo

Re: [PATCH v4 1/6] blkdebug: refactor removal of a suspended request

[Emanuele Giuseppe Esposito]

On 07/06/2021 11:23, Paolo Bonzini wrote:
> On 04/06/21 18:16, Eric Blake wrote:
>> On Fri, Jun 04, 2021 at 12:07:36PM +0200, Emanuele Giuseppe Esposito 
>> wrote:
>>> Extract to a separate function.  Do not rely on FOREACH_SAFE, which is
>>> only "safe" if the *current* node is removed---not if another node is
>>> removed.  Instead, just walk the entire list from the beginning when
>>> asked to resume all suspended requests with a given tag.
>>> -    QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, next) {
>>> +retry:
>>> +    QLIST_FOREACH(r, &s->suspended_reqs, next) {
>>>           if (!strcmp(r->tag, tag)) {
>>> +            QLIST_REMOVE(r, next);
>>
>> Isn't the whole point of using QLIST_FOREACH_SAFE the ability to call
>> QLIST_REMOVE on an element in that list while still iterating?
>>
>>>               qemu_coroutine_enter(r->co);
>>> +            if (all) {
>>> +                goto retry;
>>> +            }
>>>               return 0;
>>
>> Oh, I see - you abandon the iteration in all control flow paths, so
>> the simpler loop is still okay.  Still, this confused me enough on
>> first read that it may be worth a comment, maybe:
>>
>> /* No need for _SAFE, because iteration stops on first hit */
> 
> This is a bit confusing too because it sounds like not using _SAFE is an 
> optimization, but it's actually wrong (see commit message).
> 

What about:

/* No need for _SAFE, since a different coroutine can remove another 
node (not the current one) in this list, and when the current one is 
removed the iteration starts back from beginning anyways. */

Alternatively, no comment at all.

Thank you,
Emanuele

Re: [PATCH v4 1/6] blkdebug: refactor removal of a suspended request

[Eric Blake]

On Tue, Jun 08, 2021 at 10:00:01AM +0200, Emanuele Giuseppe Esposito wrote:
> > > Oh, I see - you abandon the iteration in all control flow paths, so
> > > the simpler loop is still okay.  Still, this confused me enough on
> > > first read that it may be worth a comment, maybe:
> > > 
> > > /* No need for _SAFE, because iteration stops on first hit */
> > 
> > This is a bit confusing too because it sounds like not using _SAFE is an
> > optimization, but it's actually wrong (see commit message).
> > 
> 
> What about:
> 
> /* No need for _SAFE, since a different coroutine can remove another node
> (not the current one) in this list, and when the current one is removed the
> iteration starts back from beginning anyways. */

Works for me, although you'll have to reformat it to pass syntax check.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org