| Message ID | 20210517145049.55268-5-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blkdebug: fix racing condition when iterating on | expand |
17.05.2021 17:50, Emanuele Giuseppe Esposito wrote: > 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> > --- > 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; > } > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> two thoughts: - suspend_request() should probably be renamed - actions_count logic is a bit overcomplicated, actually we need only suspend_count.
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; }
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> --- block/blkdebug.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)