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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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