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;
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;
--
2.30.2
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;
>
--
Best regards,
Vladimir
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;
>>>
>>
>>
>
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.