Message ID | 20210604100741.18966-2-eesposit@redhat.com |
---|---|
State | New |
Headers | show |
Series | blkdebug: fix racing condition when iterating on | expand |
On Fri, Jun 04, 2021 at 12:07:36PM +0200, Emanuele Giuseppe Esposito wrote: > Extract to a separate function. Do not rely on FOREACH_SAFE, which is > only "safe" if the *current* node is removed---not if another node is > removed. Instead, just walk the entire list from the beginning when > asked to resume all suspended requests with a given tag. > > Co-developed-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/blkdebug.c | 28 +++++++++++++++++----------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/block/blkdebug.c b/block/blkdebug.c > index 2c0b9b0ee8..8f19d991fa 100644 > --- a/block/blkdebug.c > -static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag) > +static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all) > { > - BDRVBlkdebugState *s = bs->opaque; > - BlkdebugSuspendedReq *r, *next; > + BlkdebugSuspendedReq *r; > > - QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, next) { > +retry: > + QLIST_FOREACH(r, &s->suspended_reqs, next) { > if (!strcmp(r->tag, tag)) { > + QLIST_REMOVE(r, next); Isn't the whole point of using QLIST_FOREACH_SAFE the ability to call QLIST_REMOVE on an element in that list while still iterating? > qemu_coroutine_enter(r->co); > + if (all) { > + goto retry; > + } > return 0; Oh, I see - you abandon the iteration in all control flow paths, so the simpler loop is still okay. Still, this confused me enough on first read that it may be worth a comment, maybe: /* No need for _SAFE, because iteration stops on first hit */ Reviewed-by: Eric Blake <eblake@redhat.com>
On 04/06/21 18:16, Eric Blake wrote: > On Fri, Jun 04, 2021 at 12:07:36PM +0200, Emanuele Giuseppe Esposito wrote: >> Extract to a separate function. Do not rely on FOREACH_SAFE, which is >> only "safe" if the *current* node is removed---not if another node is >> removed. Instead, just walk the entire list from the beginning when >> asked to resume all suspended requests with a given tag. >> >> - QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, next) { >> +retry: >> + QLIST_FOREACH(r, &s->suspended_reqs, next) { >> if (!strcmp(r->tag, tag)) { >> + QLIST_REMOVE(r, next); > > Isn't the whole point of using QLIST_FOREACH_SAFE the ability to call > QLIST_REMOVE on an element in that list while still iterating? > >> qemu_coroutine_enter(r->co); >> + if (all) { >> + goto retry; >> + } >> return 0; > > Oh, I see - you abandon the iteration in all control flow paths, so > the simpler loop is still okay. Still, this confused me enough on > first read that it may be worth a comment, maybe: > > /* No need for _SAFE, because iteration stops on first hit */ This is a bit confusing too because it sounds like not using _SAFE is an optimization, but it's actually wrong (see commit message). Paolo
On 07/06/2021 11:23, Paolo Bonzini wrote: > On 04/06/21 18:16, Eric Blake wrote: >> On Fri, Jun 04, 2021 at 12:07:36PM +0200, Emanuele Giuseppe Esposito >> wrote: >>> Extract to a separate function. Do not rely on FOREACH_SAFE, which is >>> only "safe" if the *current* node is removed---not if another node is >>> removed. Instead, just walk the entire list from the beginning when >>> asked to resume all suspended requests with a given tag. >>> - QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, next) { >>> +retry: >>> + QLIST_FOREACH(r, &s->suspended_reqs, next) { >>> if (!strcmp(r->tag, tag)) { >>> + QLIST_REMOVE(r, next); >> >> Isn't the whole point of using QLIST_FOREACH_SAFE the ability to call >> QLIST_REMOVE on an element in that list while still iterating? >> >>> qemu_coroutine_enter(r->co); >>> + if (all) { >>> + goto retry; >>> + } >>> return 0; >> >> Oh, I see - you abandon the iteration in all control flow paths, so >> the simpler loop is still okay. Still, this confused me enough on >> first read that it may be worth a comment, maybe: >> >> /* No need for _SAFE, because iteration stops on first hit */ > > This is a bit confusing too because it sounds like not using _SAFE is an > optimization, but it's actually wrong (see commit message). > What about: /* No need for _SAFE, since a different coroutine can remove another node (not the current one) in this list, and when the current one is removed the iteration starts back from beginning anyways. */ Alternatively, no comment at all. Thank you, Emanuele
On Tue, Jun 08, 2021 at 10:00:01AM +0200, Emanuele Giuseppe Esposito wrote: > > > Oh, I see - you abandon the iteration in all control flow paths, so > > > the simpler loop is still okay. Still, this confused me enough on > > > first read that it may be worth a comment, maybe: > > > > > > /* No need for _SAFE, because iteration stops on first hit */ > > > > This is a bit confusing too because it sounds like not using _SAFE is an > > optimization, but it's actually wrong (see commit message). > > > > What about: > > /* No need for _SAFE, since a different coroutine can remove another node > (not the current one) in this list, and when the current one is removed the > iteration starts back from beginning anyways. */ Works for me, although you'll have to reformat it to pass syntax check.
diff --git a/block/blkdebug.c b/block/blkdebug.c index 2c0b9b0ee8..8f19d991fa 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -793,7 +793,6 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule) printf("blkdebug: Resuming request '%s'\n", r.tag); } - QLIST_REMOVE(&r, next); g_free(r.tag); } @@ -869,25 +868,35 @@ static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event, return 0; } -static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag) +static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all) { - BDRVBlkdebugState *s = bs->opaque; - BlkdebugSuspendedReq *r, *next; + BlkdebugSuspendedReq *r; - QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, next) { +retry: + QLIST_FOREACH(r, &s->suspended_reqs, next) { if (!strcmp(r->tag, tag)) { + QLIST_REMOVE(r, next); qemu_coroutine_enter(r->co); + if (all) { + goto retry; + } return 0; } } return -ENOENT; } +static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag) +{ + BDRVBlkdebugState *s = bs->opaque; + + return resume_req_by_tag(s, tag, false); +} + static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs, const char *tag) { BDRVBlkdebugState *s = bs->opaque; - BlkdebugSuspendedReq *r, *r_next; BlkdebugRule *rule, *next; int i, ret = -ENOENT; @@ -900,11 +909,8 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs, } } } - QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, r_next) { - if (!strcmp(r->tag, tag)) { - qemu_coroutine_enter(r->co); - ret = 0; - } + if (resume_req_by_tag(s, tag, true) == 0) { + ret = 0; } return ret; }