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 <pbonz...@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eespo...@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
