Re: [PATCH v6 06/18] jobs: protect jobs with job_lock/unlock
Am 03/06/2022 um 18:40 schrieb Kevin Wolf:
> Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben:
>> Introduce the job locking mechanism through the whole job API,
>> following the comments in job.h and requirements of job-monitor
>> (like the functions in job-qmp.c, assume lock is held) and
>> job-driver (like in mirror.c and all other JobDriver, lock is not held).
>>
>> Use the _locked helpers introduced before to differentiate
>> between functions called with and without job_mutex.
>> This only applies to function that are called under both
>> cases, all the others will be renamed later.
>>
>> job_{lock/unlock} is independent from real_job_{lock/unlock}.
>>
>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>> are *nop*.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> ---
>> block.c | 18 ---
>> block/replication.c | 8 ++-
>> blockdev.c | 17 --
>> blockjob.c | 56 +---
>> job-qmp.c | 2 +
>> job.c | 125 +++-
>> monitor/qmp-cmds.c | 6 ++-
>> qemu-img.c | 41 +--
>> 8 files changed, 187 insertions(+), 86 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 718e4cae8b..5dc46fde11 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4978,7 +4978,9 @@ static void bdrv_close(BlockDriverState *bs)
>>
>> void bdrv_close_all(void)
>> {
>> -assert(job_next(NULL) == NULL);
>> +WITH_JOB_LOCK_GUARD() {
>> +assert(job_next(NULL) == NULL);
>> +}
>> GLOBAL_STATE_CODE();
>
> This series seems really hard to review patch by patch, in this case
> because I would have to know whether you intended job_next() to be
> called with the lock held or not. Nothing in job.h indicates either way
> at this point in the series.
Well if it's under lock it means all its calls will be under lock. If
some cases will be under lock and some other not, I use the _locked
version, as described in the commit description.
>
> Patch 11 answers this by actually renaming it job_next_locked(), but
> always having to refer to the final state after the whole series is
> applied is really not how things should work. We're splitting the work
> into individual patches so that the state after each single patch makes
> sense on its own. Otherwise the whole series could as well be a single
> patch. :-(
The various function and ordering has changed pretty much in each of the
6 version I sent, because it is very difficult to understand what comes
first and what can go afterwards.
Anyways, I see what you mean but I would not move patch 11 before this
one, because otherwise we would have _locked functions used without
having even a fake lock around, and the next reviewer would complain. In
fact, I think I put it afterwards because someone initially suggested so.
Ideally we want both patches together, but then it will be a total mess
to read, so I would leave it as it is.
In addition, I don't think it would hurt to have "normal" (ie without
_locked) functions wrapped by a nop macro.
Emanuele
>
> So I'd argue that patch 11 should probably come before this one.
>
> Anyway, I guess I'll try to make my way to the end of the series quickly
> and then somehow try to verify whatever the state is then.
>
> Kevin
>
Re: [PATCH v6 06/18] jobs: protect jobs with job_lock/unlock
Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben:
> Introduce the job locking mechanism through the whole job API,
> following the comments in job.h and requirements of job-monitor
> (like the functions in job-qmp.c, assume lock is held) and
> job-driver (like in mirror.c and all other JobDriver, lock is not held).
>
> Use the _locked helpers introduced before to differentiate
> between functions called with and without job_mutex.
> This only applies to function that are called under both
> cases, all the others will be renamed later.
>
> job_{lock/unlock} is independent from real_job_{lock/unlock}.
>
> Note: at this stage, job_{lock/unlock} and job lock guard macros
> are *nop*.
>
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> block.c | 18 ---
> block/replication.c | 8 ++-
> blockdev.c | 17 --
> blockjob.c | 56 +---
> job-qmp.c | 2 +
> job.c | 125 +++-
> monitor/qmp-cmds.c | 6 ++-
> qemu-img.c | 41 +--
> 8 files changed, 187 insertions(+), 86 deletions(-)
>
> diff --git a/block.c b/block.c
> index 718e4cae8b..5dc46fde11 100644
> --- a/block.c
> +++ b/block.c
> @@ -4978,7 +4978,9 @@ static void bdrv_close(BlockDriverState *bs)
>
> void bdrv_close_all(void)
> {
> -assert(job_next(NULL) == NULL);
> +WITH_JOB_LOCK_GUARD() {
> +assert(job_next(NULL) == NULL);
> +}
> GLOBAL_STATE_CODE();
This series seems really hard to review patch by patch, in this case
because I would have to know whether you intended job_next() to be
called with the lock held or not. Nothing in job.h indicates either way
at this point in the series.
Patch 11 answers this by actually renaming it job_next_locked(), but
always having to refer to the final state after the whole series is
applied is really not how things should work. We're splitting the work
into individual patches so that the state after each single patch makes
sense on its own. Otherwise the whole series could as well be a single
patch. :-(
So I'd argue that patch 11 should probably come before this one.
Anyway, I guess I'll try to make my way to the end of the series quickly
and then somehow try to verify whatever the state is then.
Kevin
[PATCH v6 06/18] jobs: protect jobs with job_lock/unlock
Introduce the job locking mechanism through the whole job API,
following the comments in job.h and requirements of job-monitor
(like the functions in job-qmp.c, assume lock is held) and
job-driver (like in mirror.c and all other JobDriver, lock is not held).
Use the _locked helpers introduced before to differentiate
between functions called with and without job_mutex.
This only applies to function that are called under both
cases, all the others will be renamed later.
job_{lock/unlock} is independent from real_job_{lock/unlock}.
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.
Signed-off-by: Emanuele Giuseppe Esposito
---
block.c | 18 ---
block/replication.c | 8 ++-
blockdev.c | 17 --
blockjob.c | 56 +---
job-qmp.c | 2 +
job.c | 125 +++-
monitor/qmp-cmds.c | 6 ++-
qemu-img.c | 41 +--
8 files changed, 187 insertions(+), 86 deletions(-)
diff --git a/block.c b/block.c
index 718e4cae8b..5dc46fde11 100644
--- a/block.c
+++ b/block.c
@@ -4978,7 +4978,9 @@ static void bdrv_close(BlockDriverState *bs)
void bdrv_close_all(void)
{
-assert(job_next(NULL) == NULL);
+WITH_JOB_LOCK_GUARD() {
+assert(job_next(NULL) == NULL);
+}
GLOBAL_STATE_CODE();
/* Drop references from requests still in flight, such as canceled block
@@ -6165,13 +6167,15 @@ XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp)
}
}
-for (job = block_job_next(NULL); job; job = block_job_next(job)) {
-GSList *el;
+WITH_JOB_LOCK_GUARD() {
+for (job = block_job_next(NULL); job; job = block_job_next(job)) {
+GSList *el;
-xdbg_graph_add_node(gr, job, X_DBG_BLOCK_GRAPH_NODE_TYPE_BLOCK_JOB,
- job->job.id);
-for (el = job->nodes; el; el = el->next) {
-xdbg_graph_add_edge(gr, job, (BdrvChild *)el->data);
+xdbg_graph_add_node(gr, job, X_DBG_BLOCK_GRAPH_NODE_TYPE_BLOCK_JOB,
+job->job.id);
+for (el = job->nodes; el; el = el->next) {
+xdbg_graph_add_edge(gr, job, (BdrvChild *)el->data);
+}
}
}
diff --git a/block/replication.c b/block/replication.c
index 55c8f894aa..a03b28726e 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -149,7 +149,9 @@ static void replication_close(BlockDriverState *bs)
if (s->stage == BLOCK_REPLICATION_FAILOVER) {
commit_job = &s->commit_job->job;
assert(commit_job->aio_context == qemu_get_current_aio_context());
-job_cancel_sync(commit_job, false);
+WITH_JOB_LOCK_GUARD() {
+job_cancel_sync(commit_job, false);
+}
}
if (s->mode == REPLICATION_MODE_SECONDARY) {
@@ -726,7 +728,9 @@ static void replication_stop(ReplicationState *rs, bool
failover, Error **errp)
* disk, secondary disk in backup_job_completed().
*/
if (s->backup_job) {
-job_cancel_sync(&s->backup_job->job, true);
+WITH_JOB_LOCK_GUARD() {
+job_cancel_sync(&s->backup_job->job, true);
+}
}
if (!failover) {
diff --git a/blockdev.c b/blockdev.c
index e46e831212..8722e5d4b9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -150,6 +150,8 @@ void blockdev_mark_auto_del(BlockBackend *blk)
return;
}
+JOB_LOCK_GUARD();
+
for (job = block_job_next(NULL); job; job = block_job_next(job)) {
if (block_job_has_bdrv(job, blk_bs(blk))) {
AioContext *aio_context = job->job.aio_context;
@@ -1838,7 +1840,9 @@ static void drive_backup_abort(BlkActionState *common)
aio_context = bdrv_get_aio_context(state->bs);
aio_context_acquire(aio_context);
-job_cancel_sync(&state->job->job, true);
+WITH_JOB_LOCK_GUARD() {
+job_cancel_sync(&state->job->job, true);
+}
aio_context_release(aio_context);
}
@@ -1939,7 +1943,9 @@ static void blockdev_backup_abort(BlkActionState *common)
aio_context = bdrv_get_aio_context(state->bs);
aio_context_acquire(aio_context);
-job_cancel_sync(&state->job->job, true);
+WITH_JOB_LOCK_GUARD() {
+job_cancel_sync(&state->job->job, true);
+}
aio_context_release(aio_context);
}
@@ -2388,7 +2394,10 @@ exit:
if (!has_props) {
qapi_free_TransactionProperties(props);
}
-job_txn_unref(block_job_txn);
+
+WITH_JOB_LOCK_GUARD() {
+job_txn_unref(block_job_txn);
+}
}
BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
@@ -3720,6 +3729,8 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
BlockJobInfoList *head = NULL, **tail = &head;
BlockJob *job;
+JOB_LOCK_GUARD();
+
for (job = block_job_next(NULL); job; job =
