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

[Vladimir Sementsov-Ogievskiy]

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