diff mbox series

[v5,2/6] blkdebug: move post-resume handling to resume_req_by_tag

Message ID 20210614082931.24925-3-eesposit@redhat.com
State New
Headers show
Series blkdebug: fix racing condition when iterating on | expand

Commit Message

Emanuele Giuseppe Esposito June 14, 2021, 8:29 a.m. UTC 
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(-)

Comments

Max Reitz July 15, 2021, 9:59 a.m. UTC | #1 
On 14.06.21 10:29, Emanuele Giuseppe Esposito wrote:
> 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 5ccbfcab42..e8fdf7b056 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);

Not wrong, but just as a note: I personally would have done the 
initialization like

*r = (BlkdebugSuspendedReq) {
     .co = ...,
     .tag = ...,
};

The advantage is that this sets all fields that aren’t mentioned to zero 
(kind of important, because you don’t use g_new0(), and so now I have to 
manually verify that there are no other fields that would need to be 
initialized (which there aren’t)), and in this special case the diff 
stat also would have been smaller. (But that’s a rare coincidence.)

There are no other fields besides the list entry object (which is fully 
overwritten by QLIST_INSERT_HEAD()), though, so this patch is correct 
and I’m happy with it as-is.

Max
diff mbox series

Patch

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5ccbfcab42..e8fdf7b056 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,
@@ -880,8 +875,18 @@  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;
             }