| Message ID | 20210517145049.55268-6-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: > Co-developed-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block/blkdebug.c | 53 ++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 40 insertions(+), 13 deletions(-) > > diff --git a/block/blkdebug.c b/block/blkdebug.c > index dffd869b32..cf8b088ce7 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -54,6 +54,7 @@ typedef struct BDRVBlkdebugState { > /* For blkdebug_refresh_filename() */ > char *config_file; > > + QemuMutex lock; we'll need a comments, which fields are protected by the lock, like in other your series. and also a comment which functions are thread-safe after the patch. remove_rule() lacks comment, like "Called with lock held or from .bdrv_close" > QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX]; > QSIMPLEQ_HEAD(, BlkdebugRule) active_rules; > QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs; > @@ -245,7 +246,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); > > return 0; > } > @@ -468,6 +471,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; > @@ -568,6 +572,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); > @@ -582,6 +587,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; > > @@ -595,6 +601,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; > } > > @@ -606,6 +613,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(); > @@ -771,8 +779,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; > @@ -791,6 +801,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) > { > @@ -829,9 +840,11 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event) > > assert((int)event >= 0 && event < BLKDBG__MAX); > > - s->new_state = s->state; > - QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) { > - process_rule(bs, rule, actions_count); > + WITH_QEMU_LOCK_GUARD(&s->lock) { > + s->new_state = s->state; > + QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) { > + process_rule(bs, rule, actions_count); > + } > } > > while (actions_count[ACTION_SUSPEND] > 0) { > @@ -839,7 +852,9 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event) > actions_count[ACTION_SUSPEND]--; > } > > + qemu_mutex_lock(&s->lock); > s->state = s->new_state; > + qemu_mutex_unlock(&s->lock); > } Preexising: honestly, I don't understand the logic around state and new_state.. (and therefore, I'm not sure, is it in good relation with patch 04) It come in the commit commit 8f96b5be92fbd74798b97b1dc1ff5fbbe249ed11 Author: Paolo Bonzini <pbonzini@redhat.com> Date: Fri Sep 28 17:23:00 2012 +0200 blkdebug: process all set_state rules in the old state Currently it is impossible to write a blkdebug script that ping-pongs between two states, because the second set-state rule will use the state that is set in the first. If you have [set-state] event = "..." state = "1" new_state = "2" [set-state] event = "..." state = "2" new_state = "1" for example the state will remain locked at 1. This can be fixed by first processing all rules, and then setting the state. But I don't understand, whey it should remain locked.. And this logic is not safe: another event may happen during the yield, and will operate on the same s->state and s->new_state, leading the the mess. > > static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event, > @@ -862,11 +877,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. */ I think, it would be a very good practice to include into convention: May temporarily release lock. > static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all) > { > BlkdebugSuspendedReq *r; > @@ -884,7 +902,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; > @@ -898,8 +918,12 @@ retry: > static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag) > { > BDRVBlkdebugState *s = bs->opaque; > + int rc; Hmm, you can simply use QEMU_LOCK_GUARD > > - return resume_req_by_tag(s, tag, false); > + qemu_mutex_lock(&s->lock); > + rc = resume_req_by_tag(s, tag, false); > + qemu_mutex_unlock(&s->lock); > + return rc; > } > > static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs, > @@ -909,17 +933,19 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs, > BlkdebugRule *rule, *next; > int i, ret = -ENOENT; > > - for (i = 0; i < BLKDBG__MAX; i++) { > - QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) { > - if (rule->action == ACTION_SUSPEND && > - !strcmp(rule->options.suspend.tag, tag)) { > - remove_rule(rule); > - ret = 0; > + WITH_QEMU_LOCK_GUARD(&s->lock) { And here, instead of increasing nesting, let's use QEMU_LOCK_GUARD() > + for (i = 0; i < BLKDBG__MAX; i++) { > + QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) { > + if (rule->action == ACTION_SUSPEND && > + !strcmp(rule->options.suspend.tag, tag)) { > + remove_rule(rule); > + ret = 0; > + } > } > } > - } > - if (resume_req_by_tag(s, tag, true) == 0) { > - ret = 0; > + if (resume_req_by_tag(s, tag, true) == 0) { > + ret = 0; > + } > } > return ret; > } > @@ -929,6 +955,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; >
On 01/06/2021 10:39, Vladimir Sementsov-Ogievskiy wrote: > 17.05.2021 17:50, Emanuele Giuseppe Esposito wrote: >> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> block/blkdebug.c | 53 ++++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 40 insertions(+), 13 deletions(-) >> >> diff --git a/block/blkdebug.c b/block/blkdebug.c >> index dffd869b32..cf8b088ce7 100644 >> --- a/block/blkdebug.c >> +++ b/block/blkdebug.c >> @@ -54,6 +54,7 @@ typedef struct BDRVBlkdebugState { >> /* For blkdebug_refresh_filename() */ >> char *config_file; >> + QemuMutex lock; > > we'll need a comments, which fields are protected by the lock, like in > other your series. > > and also a comment which functions are thread-safe after the patch. > > remove_rule() lacks comment, like "Called with lock held or from > .bdrv_close" Will apply these feedback in next version, thanks. [...] >> +/* Called with lock held. */ >> static void process_rule(BlockDriverState *bs, struct BlkdebugRule >> *rule, >> int *action_count) >> { >> @@ -829,9 +840,11 @@ static void blkdebug_debug_event(BlockDriverState >> *bs, BlkdebugEvent event) >> assert((int)event >= 0 && event < BLKDBG__MAX); >> - s->new_state = s->state; >> - QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) { >> - process_rule(bs, rule, actions_count); >> + WITH_QEMU_LOCK_GUARD(&s->lock) { >> + s->new_state = s->state; >> + QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) { >> + process_rule(bs, rule, actions_count); >> + } >> } >> while (actions_count[ACTION_SUSPEND] > 0) { >> @@ -839,7 +852,9 @@ static void blkdebug_debug_event(BlockDriverState >> *bs, BlkdebugEvent event) >> actions_count[ACTION_SUSPEND]--; >> } >> + qemu_mutex_lock(&s->lock); >> s->state = s->new_state; >> + qemu_mutex_unlock(&s->lock); >> } > > Preexising: honestly, I don't understand the logic around state and > new_state.. (and therefore, I'm not sure, is it in good relation with > patch 04) > > It come in the commit > > commit 8f96b5be92fbd74798b97b1dc1ff5fbbe249ed11 > Author: Paolo Bonzini <pbonzini@redhat.com> > Date: Fri Sep 28 17:23:00 2012 +0200 > > blkdebug: process all set_state rules in the old state > Currently it is impossible to write a blkdebug script that ping-pongs > between two states, because the second set-state rule will use the > state that is set in the first. If you have > [set-state] > event = "..." > state = "1" > new_state = "2" > [set-state] > event = "..." > state = "2" > new_state = "1" > for example the state will remain locked at 1. This can be fixed > by first processing all rules, and then setting the state. > > But I don't understand, whey it should remain locked.. From what I understand, when bdrv_debug_event is invoked the code before this patch will simply change the state in 1 - 2 - 1 in the same rule iteration. So following Paolo's example, having these two rules in the same rules["..."] list, will make only one bdrv_debug_event change the state from 1 to 2, and 2 to 1 (if they are ordered in this way), removing both rules from the list. This is not the expected behavior: we want two bdrv_debug_event to trigger the two state changes, so in the first bdrv_debug_event call we have 1-2 and next 2-1. > > And this logic is not safe: another event may happen during the yield, > and will operate on the same s->state and s->new_state, leading the the > mess. Yes, I think you are right. The state update should go in the same lock guard above, like this: WITH_QEMU_LOCK_GUARD(&s->lock) { s->new_state = s->state; QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) { process_rule(bs, rule, actions_count); } + s->state = s->new_state; } while (actions_count[ACTION_SUSPEND] > 0) { qemu_coroutine_yield(); actions_count[ACTION_SUSPEND]--; } - qemu_mutex_lock(&s->lock); - s->state = s->new_state; - qemu_mutex_unlock(&s->lock); The other comments below make sense to me, will also apply them. Thank you, Emanuele > >> static int blkdebug_debug_breakpoint(BlockDriverState *bs, const >> char *event, >> @@ -862,11 +877,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. */ > > I think, it would be a very good practice to include into convention: > > May temporarily release lock. > >> static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, >> bool all) >> { >> BlkdebugSuspendedReq *r; >> @@ -884,7 +902,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; >> @@ -898,8 +918,12 @@ retry: >> static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag) >> { >> BDRVBlkdebugState *s = bs->opaque; >> + int rc; > > Hmm, you can simply use QEMU_LOCK_GUARD > >> - return resume_req_by_tag(s, tag, false); >> + qemu_mutex_lock(&s->lock); >> + rc = resume_req_by_tag(s, tag, false); >> + qemu_mutex_unlock(&s->lock); >> + return rc; >> } >> static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs, >> @@ -909,17 +933,19 @@ static int >> blkdebug_debug_remove_breakpoint(BlockDriverState *bs, >> BlkdebugRule *rule, *next; >> int i, ret = -ENOENT; >> - for (i = 0; i < BLKDBG__MAX; i++) { >> - QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) { >> - if (rule->action == ACTION_SUSPEND && >> - !strcmp(rule->options.suspend.tag, tag)) { >> - remove_rule(rule); >> - ret = 0; >> + WITH_QEMU_LOCK_GUARD(&s->lock) { > > And here, instead of increasing nesting, let's use QEMU_LOCK_GUARD() > >> + for (i = 0; i < BLKDBG__MAX; i++) { >> + QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) { >> + if (rule->action == ACTION_SUSPEND && >> + !strcmp(rule->options.suspend.tag, tag)) { >> + remove_rule(rule); >> + ret = 0; >> + } >> } >> } >> - } >> - if (resume_req_by_tag(s, tag, true) == 0) { >> - ret = 0; >> + if (resume_req_by_tag(s, tag, true) == 0) { >> + ret = 0; >> + } >> } >> return ret; >> } >> @@ -929,6 +955,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; >> > >
02.06.2021 15:59, Emanuele Giuseppe Esposito wrote: > > > On 01/06/2021 10:39, Vladimir Sementsov-Ogievskiy wrote: >> 17.05.2021 17:50, Emanuele Giuseppe Esposito wrote: >>> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com> >>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>> --- >>> block/blkdebug.c | 53 ++++++++++++++++++++++++++++++++++++------------ >>> 1 file changed, 40 insertions(+), 13 deletions(-) >>> >>> diff --git a/block/blkdebug.c b/block/blkdebug.c >>> index dffd869b32..cf8b088ce7 100644 >>> --- a/block/blkdebug.c >>> +++ b/block/blkdebug.c >>> @@ -54,6 +54,7 @@ typedef struct BDRVBlkdebugState { >>> /* For blkdebug_refresh_filename() */ >>> char *config_file; >>> + QemuMutex lock; >> >> we'll need a comments, which fields are protected by the lock, like in other your series. >> >> and also a comment which functions are thread-safe after the patch. >> >> remove_rule() lacks comment, like "Called with lock held or from .bdrv_close" > > Will apply these feedback in next version, thanks. > > [...] > >>> +/* Called with lock held. */ >>> static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule, >>> int *action_count) >>> { >>> @@ -829,9 +840,11 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event) >>> assert((int)event >= 0 && event < BLKDBG__MAX); >>> - s->new_state = s->state; >>> - QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) { >>> - process_rule(bs, rule, actions_count); >>> + WITH_QEMU_LOCK_GUARD(&s->lock) { >>> + s->new_state = s->state; >>> + QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) { >>> + process_rule(bs, rule, actions_count); >>> + } >>> } >>> while (actions_count[ACTION_SUSPEND] > 0) { >>> @@ -839,7 +852,9 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event) >>> actions_count[ACTION_SUSPEND]--; >>> } >>> + qemu_mutex_lock(&s->lock); >>> s->state = s->new_state; >>> + qemu_mutex_unlock(&s->lock); >>> } >> >> Preexising: honestly, I don't understand the logic around state and new_state.. (and therefore, I'm not sure, is it in good relation with patch 04) >> >> It come in the commit >> >> commit 8f96b5be92fbd74798b97b1dc1ff5fbbe249ed11 >> Author: Paolo Bonzini <pbonzini@redhat.com> >> Date: Fri Sep 28 17:23:00 2012 +0200 >> >> blkdebug: process all set_state rules in the old state >> Currently it is impossible to write a blkdebug script that ping-pongs >> between two states, because the second set-state rule will use the >> state that is set in the first. If you have >> [set-state] >> event = "..." >> state = "1" >> new_state = "2" >> [set-state] >> event = "..." >> state = "2" >> new_state = "1" >> for example the state will remain locked at 1. This can be fixed >> by first processing all rules, and then setting the state. >> >> But I don't understand, whey it should remain locked.. > > From what I understand, when bdrv_debug_event is invoked the code before this patch will simply change the state in 1 - 2 - 1 in the same rule iteration. So following Paolo's example, having these two rules in the same rules["..."] list, will make only one bdrv_debug_event change the state from 1 to 2, and 2 to 1 (if they are ordered in this way), removing both rules from the list. > > This is not the expected behavior: we want two bdrv_debug_event to trigger the two state changes, so in the first bdrv_debug_event call we have 1-2 and next 2-1. Oh, understand now, thanks. > >> >> And this logic is not safe: another event may happen during the yield, and will operate on the same s->state and s->new_state, leading the the mess. > > Yes, I think you are right. The state update should go in the same lock guard above, like this: Agreed. Probably good refactoring would be return new_state from process_rule, this way we can drop extra state variable s->new_state and use local variable instead (like we do for actions_count) > > WITH_QEMU_LOCK_GUARD(&s->lock) { > s->new_state = s->state; > QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) { > process_rule(bs, rule, actions_count); > } > + s->state = s->new_state; > } > > while (actions_count[ACTION_SUSPEND] > 0) { > qemu_coroutine_yield(); > actions_count[ACTION_SUSPEND]--; > } > > - qemu_mutex_lock(&s->lock); > - s->state = s->new_state; > - qemu_mutex_unlock(&s->lock); > > The other comments below make sense to me, will also apply them. > > Thank you, > Emanuele > >> >>> static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event, >>> @@ -862,11 +877,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. */ >> >> I think, it would be a very good practice to include into convention: >> >> May temporarily release lock. > >>> static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all) >>> { >>> BlkdebugSuspendedReq *r; >>> @@ -884,7 +902,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; >>> @@ -898,8 +918,12 @@ retry: >>> static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag) >>> { >>> BDRVBlkdebugState *s = bs->opaque; >>> + int rc; >> >> Hmm, you can simply use QEMU_LOCK_GUARD > >> >>> - return resume_req_by_tag(s, tag, false); >>> + qemu_mutex_lock(&s->lock); >>> + rc = resume_req_by_tag(s, tag, false); >>> + qemu_mutex_unlock(&s->lock); >>> + return rc; >>> } >>> static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs, >>> @@ -909,17 +933,19 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs, >>> BlkdebugRule *rule, *next; >>> int i, ret = -ENOENT; >>> - for (i = 0; i < BLKDBG__MAX; i++) { >>> - QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) { >>> - if (rule->action == ACTION_SUSPEND && >>> - !strcmp(rule->options.suspend.tag, tag)) { >>> - remove_rule(rule); >>> - ret = 0; >>> + WITH_QEMU_LOCK_GUARD(&s->lock) { >> >> And here, instead of increasing nesting, let's use QEMU_LOCK_GUARD() >> >>> + for (i = 0; i < BLKDBG__MAX; i++) { >>> + QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) { >>> + if (rule->action == ACTION_SUSPEND && >>> + !strcmp(rule->options.suspend.tag, tag)) { >>> + remove_rule(rule); >>> + ret = 0; >>> + } >>> } >>> } >>> - } >>> - if (resume_req_by_tag(s, tag, true) == 0) { >>> - ret = 0; >>> + if (resume_req_by_tag(s, tag, true) == 0) { >>> + ret = 0; >>> + } >>> } >>> return ret; >>> } >>> @@ -929,6 +955,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; >>> >> >> >
diff --git a/block/blkdebug.c b/block/blkdebug.c index dffd869b32..cf8b088ce7 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -54,6 +54,7 @@ typedef struct BDRVBlkdebugState { /* For blkdebug_refresh_filename() */ char *config_file; + QemuMutex lock; QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX]; QSIMPLEQ_HEAD(, BlkdebugRule) active_rules; QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs; @@ -245,7 +246,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); return 0; } @@ -468,6 +471,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; @@ -568,6 +572,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); @@ -582,6 +587,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; @@ -595,6 +601,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; } @@ -606,6 +613,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(); @@ -771,8 +779,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; @@ -791,6 +801,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) { @@ -829,9 +840,11 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event) assert((int)event >= 0 && event < BLKDBG__MAX); - s->new_state = s->state; - QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) { - process_rule(bs, rule, actions_count); + WITH_QEMU_LOCK_GUARD(&s->lock) { + s->new_state = s->state; + QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) { + process_rule(bs, rule, actions_count); + } } while (actions_count[ACTION_SUSPEND] > 0) { @@ -839,7 +852,9 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event) actions_count[ACTION_SUSPEND]--; } + qemu_mutex_lock(&s->lock); s->state = s->new_state; + qemu_mutex_unlock(&s->lock); } static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event, @@ -862,11 +877,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. */ static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all) { BlkdebugSuspendedReq *r; @@ -884,7 +902,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; @@ -898,8 +918,12 @@ retry: static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag) { BDRVBlkdebugState *s = bs->opaque; + int rc; - return resume_req_by_tag(s, tag, false); + qemu_mutex_lock(&s->lock); + rc = resume_req_by_tag(s, tag, false); + qemu_mutex_unlock(&s->lock); + return rc; } static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs, @@ -909,17 +933,19 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs, BlkdebugRule *rule, *next; int i, ret = -ENOENT; - for (i = 0; i < BLKDBG__MAX; i++) { - QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) { - if (rule->action == ACTION_SUSPEND && - !strcmp(rule->options.suspend.tag, tag)) { - remove_rule(rule); - ret = 0; + WITH_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 && + !strcmp(rule->options.suspend.tag, tag)) { + remove_rule(rule); + ret = 0; + } } } - } - if (resume_req_by_tag(s, tag, true) == 0) { - ret = 0; + if (resume_req_by_tag(s, tag, true) == 0) { + ret = 0; + } } return ret; } @@ -929,6 +955,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;
Co-developed-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/blkdebug.c | 53 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 13 deletions(-)