diff mbox series

[v3,5/5] blkdebug: protect rules and suspended_reqs with a lock

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

Commit Message

Emanuele Giuseppe Esposito May 17, 2021, 2:50 p.m. UTC
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(-)

Comments

Vladimir Sementsov-Ogievskiy June 1, 2021, 8:39 a.m. UTC | #1
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;
>
Emanuele Giuseppe Esposito June 2, 2021, 12:59 p.m. UTC | #2
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;
>>
> 
>
Vladimir Sementsov-Ogievskiy June 2, 2021, 1:24 p.m. UTC | #3
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 mbox series

Patch

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;