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

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

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;
>>
> 
>