Re: [PATCH v4 6/6] blkdebug: protect rules and suspended_reqs with a lock

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

05.06.2021 20:53, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 05/06/2021 17:15, Vladimir Sementsov-Ogievskiy wrote:
>> 04.06.2021 13:07, Emanuele Giuseppe Esposito wrote:
>>> First, categorize the structure fields to identify what needs
>>> to be protected and what doesn't.
>>>
>>> We essentially need to protect only .state, and the 3 lists in
>>> BDRVBlkdebugState.
>>>
>>> Then, add the lock and mark the functions accordingly.
>>>
>>> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>>   block/blkdebug.c | 46 +++++++++++++++++++++++++++++++++++-----------
>>>   1 file changed, 35 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>>> index d597753139..ac3799f739 100644
>>> --- a/block/blkdebug.c
>>> +++ b/block/blkdebug.c
>>> @@ -38,24 +38,27 @@
>>>   #include "qapi/qobject-input-visitor.h"
>>>   #include "sysemu/qtest.h"
>>> +/* All APIs are thread-safe */
>>> +
>>>   typedef struct BDRVBlkdebugState {
>>> -    int state;
>>> +    /* IN: initialized in blkdebug_open() and never changed */
>>>       uint64_t align;
>>>       uint64_t max_transfer;
>>>       uint64_t opt_write_zero;
>>>       uint64_t max_write_zero;
>>>       uint64_t opt_discard;
>>>       uint64_t max_discard;
>>> -
>>> +    char *config_file; /* For blkdebug_refresh_filename() */
>>> +    /* initialized in blkdebug_parse_perms() */
>>>       uint64_t take_child_perms;
>>>       uint64_t unshare_child_perms;
>>> -    /* For blkdebug_refresh_filename() */
>>> -    char *config_file;
>>> -
>>> +    /* State. Protected by lock */
>>> +    int state;
>>>       QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX];
>>>       QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
>>>       QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
>>> +    QemuMutex lock;
>>>   } BDRVBlkdebugState;
>>>   typedef struct BlkdebugAIOCB {
>>> @@ -64,6 +67,7 @@ typedef struct BlkdebugAIOCB {
>>>   } BlkdebugAIOCB;
>>>   typedef struct BlkdebugSuspendedReq {
>>> +    /* IN: initialized in suspend_request() */
>>>       Coroutine *co;
>>>       char *tag;
>>
>> @next is part of *suspended_reqs list (in a manner), so it should be protected by lock
>>
>>>       QLIST_ENTRY(BlkdebugSuspendedReq) next;
>>> @@ -77,6 +81,7 @@ enum {
>>>   };
>>>   typedef struct BlkdebugRule {
>>> +    /* IN: initialized in add_rule() or blkdebug_debug_breakpoint() */
>>>       BlkdebugEvent event;
>>>       int action;
>>>       int state;
>>
>> as well as @next and @active_next here.
> 
> They all are already protected by the lock, I will just update the comments in case I need to re-spin.
> 
> [...]
> 
>>
>> Optional suggestion for additional comments and more use of QEMU_LOCK_GUARD (it looks large because of indentation change):
> 
> Exactly, indentation change. If I recall correctly, you (rightly) complained about that in one of my patches (not sure if it was in this series).

Probably here, indentation doesn't become so big :)

Maximum is

WITH_ () {
   FOR {
      if {
         ***

And this if contains only one simple "break".

Of course, that's a kind of taste. I hope I was not too insistent that past time.

> 
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index ac3799f739..b4f8844e76 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -70,6 +70,8 @@ typedef struct BlkdebugSuspendedReq {
>>       /* IN: initialized in suspend_request() */
>>       Coroutine *co;
>>       char *tag;
>> +
>> +    /* List entry protected BDRVBlkdebugState::lock */
> 
> Is this C++ style ok to be used here? I don't think I have seen it used in QEMU. But I might be wrong, someone with better style taste and experience should comment here. Maybe it's better to have
> 
> /* List entry protected BDRVBlkdebugState's lock */

OK for me, I don't care)

Hmm, looking at git log, I see :: syntax in my commits. And not only my.

> 
>>       QLIST_ENTRY(BlkdebugSuspendedReq) next;
>>   } BlkdebugSuspendedReq;
>>
>> @@ -100,6 +102,8 @@ typedef struct BlkdebugRule {
>>               char *tag;
>>           } suspend;
>>       } options;
>> +
>> +    /* List entries protected BDRVBlkdebugState::lock */
>>       QLIST_ENTRY(BlkdebugRule) next;
>>       QSIMPLEQ_ENTRY(BlkdebugRule) active_next;
>>   } BlkdebugRule;
>> @@ -249,9 +253,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);
>> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
>> +        QLIST_INSERT_HEAD(&s->rules[event], rule, next);
>> +    }
> Same lines used, just additional indentation added. For one line it might be okay? not sure.

Same three lines, but don't need to call _unlock()..

I think, for last time I just get used to the thought that WITH_QEMU_LOCK_GUARD(){} is a good thing.

Here it's really doesn't make real sense. So, take the suggestion only if you like it, my r-b stands without it as well.

>>
>>       return 0;
>>   }
>> @@ -591,33 +595,32 @@ 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;
>> -
>> -        if ((inject_offset == -1 ||
>> -             (bytes && inject_offset >= offset &&
>> -              inject_offset < offset + bytes)) &&
>> -            (rule->options.inject.iotype_mask & (1ull << iotype)))
>> -        {
>> -            break;
>> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
>> +        QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
>> +            uint64_t inject_offset = rule->options.inject.offset;
>> +
>> +            if ((inject_offset == -1 ||
>> +                 (bytes && inject_offset >= offset &&
>> +                  inject_offset < offset + bytes)) &&
>> +                (rule->options.inject.iotype_mask & (1ull << iotype)))
>> +            {
>> +                break;
>> +            }
>>           }
>> -    }
>>
>> -    if (!rule || !rule->options.inject.error) {
>> -        qemu_mutex_unlock(&s->lock);
>> -        return 0;
>> -    }
>> +        if (!rule || !rule->options.inject.error) {
>> +            return 0;
>> +        }
>>
>> -    immediately = rule->options.inject.immediately;
>> -    error = rule->options.inject.error;
>> +        immediately = rule->options.inject.immediately;
>> +        error = rule->options.inject.error;
>>
>> -    if (rule->options.inject.once) {
>> -        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
>> -        remove_rule(rule);
>> +        if (rule->options.inject.once) {
>> +            QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
>> +            remove_rule(rule);
>> +        }
>>       }
>>
>> -    qemu_mutex_unlock(&s->lock);
> 
> Too much indentation added for a couple of lock/unlock IMO.
> 
>>       if (!immediately) {
>>           aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
>>           qemu_coroutine_yield();
>> @@ -880,9 +883,9 @@ 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);
>> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
>> +        QLIST_INSERT_HEAD(&s->rules[blkdebug_event], rule, next);
>> +    }
> 
> Same as above.
> 
> Thank you,
> Emanuele
> 


-- 
Best regards,
Vladimir