[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 08/20] jobs: protect jobs with job_lock/unlock
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH v5 08/20] jobs: protect jobs with job_lock/unlock |
Date: |
Thu, 17 Feb 2022 14:48:45 +0000 |
On Tue, Feb 08, 2022 at 09:35:01AM -0500, Emanuele Giuseppe Esposito wrote:
> 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());
Is it safe to access commit_job->aio_context outside job_mutex?
> @@ -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);
> + }
Maybe job_cancel_sync() should take the lock internally since all
callers in this patch seem to need the lock?
I noticed this patch does not add WITH_JOB_LOCK_GUARD() to
tests/unit/test-blockjob.c:cancel_common(). Was that an oversight or is
there a reason why job_mutex is not needed around the job_cancel_sync()
call there?
> @@ -252,7 +258,13 @@ int block_job_add_bdrv(BlockJob *job, const char *name,
> BlockDriverState *bs,
>
> static void block_job_on_idle(Notifier *n, void *opaque)
> {
> + /*
> + * we can't kick with job_mutex held, but we also want
> + * to protect the notifier list.
> + */
> + job_unlock();
> aio_wait_kick();
> + job_lock();
I don't understand this. aio_wait_kick() looks safe to call with a mutex
held?
> @@ -292,7 +304,9 @@ bool block_job_set_speed(BlockJob *job, int64_t speed,
> Error **errp)
> job->speed = speed;
>
> if (drv->set_speed) {
> + job_unlock();
> drv->set_speed(job, speed);
> + job_lock();
What guarantees that job stays alive during drv->set_speed(job)? We
don't hold a ref here. Maybe the assumption is that
block_job_set_speed() only gets called from the main loop thread and
nothing else will modify the jobs list while we're in drv->set_speed()?
> @@ -545,10 +566,15 @@ BlockErrorAction block_job_error_action(BlockJob *job,
> BlockdevOnError on_err,
> action);
> }
> if (action == BLOCK_ERROR_ACTION_STOP) {
> - if (!job->job.user_paused) {
> - job_pause(&job->job);
> - /* make the pause user visible, which will be resumed from QMP.
> */
> - job->job.user_paused = true;
> + WITH_JOB_LOCK_GUARD() {
> + if (!job->job.user_paused) {
> + job_pause(&job->job);
> + /*
> + * make the pause user visible, which will be
> + * resumed from QMP.
> + */
> + job->job.user_paused = true;
> + }
> }
> block_job_iostatus_set_err(job, error);
Does this need the lock? If not, why is block_job_iostatus_reset()
called with the hold?
signature.asc
Description: PGP signature
- [PATCH v5 14/20] block_job: rename block_job functions called with job_mutex held, (continued)
- [PATCH v5 14/20] block_job: rename block_job functions called with job_mutex held, Emanuele Giuseppe Esposito, 2022/02/08
- [PATCH v5 09/20] jobs: add job lock in find_* functions, Emanuele Giuseppe Esposito, 2022/02/08
- [PATCH v5 10/20] jobs: use job locks also in the unit tests, Emanuele Giuseppe Esposito, 2022/02/08
- [PATCH v5 16/20] commit and mirror: create new nodes using bdrv_get_aio_context, and not the job aiocontext, Emanuele Giuseppe Esposito, 2022/02/08
- [PATCH v5 13/20] job.h: rename job API functions called with job_mutex held, Emanuele Giuseppe Esposito, 2022/02/08
- [PATCH v5 05/20] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED, Emanuele Giuseppe Esposito, 2022/02/08
- [PATCH v5 08/20] jobs: protect jobs with job_lock/unlock, Emanuele Giuseppe Esposito, 2022/02/08
- Re: [PATCH v5 08/20] jobs: protect jobs with job_lock/unlock,
Stefan Hajnoczi <=
[PATCH v5 19/20] job.c: enable job lock/unlock and remove Aiocontext locks, Emanuele Giuseppe Esposito, 2022/02/08
[PATCH v5 20/20] block_job_query: remove atomic read, Emanuele Giuseppe Esposito, 2022/02/08
[PATCH v5 18/20] jobs: protect job.aio_context with BQL and job_mutex, Emanuele Giuseppe Esposito, 2022/02/08
[PATCH v5 12/20] jobs: rename static functions called with job_mutex held, Emanuele Giuseppe Esposito, 2022/02/08
[PATCH v5 11/20] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU, Emanuele Giuseppe Esposito, 2022/02/08
[PATCH v5 15/20] job.h: define unlocked functions, Emanuele Giuseppe Esposito, 2022/02/08