On 01.04.20 10:15, Stefan Reiter wrote: > job_cancel_sync requires the job's lock to be held, all other callers > already do this (replication_stop, drive_backup_abort, > blockdev_backup_abort, job_cancel_sync_all, cancel_common).
I think all other callers come directly from QMP, though, so they have no locks yet. This OTOH is called from a block driver function, so I would assume the BDS context is locked already (or rather, this is executed in the BDS context). I also think that the commit job runs in the same context. So I would assume that this would be a nested lock, which should be unnecessary and might cause problems. Maybe we should just assert that the job’s context is the current context? (Or would that still be problematic because now job_txn_apply() wants to release some context, and that isn’t possible without this patch? I would hope it’s possible, because I think we shouldn’t have to acquire the current context, and should be free to release it...? I have no idea. Maybe we are actually free to reacquire the current context here.) > Signed-off-by: Stefan Reiter <s.rei...@proxmox.com> > --- > block/replication.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/block/replication.c b/block/replication.c > index 413d95407d..17ddc31569 100644 > --- a/block/replication.c > +++ b/block/replication.c > @@ -144,12 +144,18 @@ fail: > static void replication_close(BlockDriverState *bs) > { > BDRVReplicationState *s = bs->opaque; > + Job *commit_job; > + AioContext *commit_ctx; > > if (s->stage == BLOCK_REPLICATION_RUNNING) { > replication_stop(s->rs, false, NULL); > } > if (s->stage == BLOCK_REPLICATION_FAILOVER) { > - job_cancel_sync(&s->commit_job->job); > + commit_job = &s->commit_job->job; > + commit_ctx = commit_job->aio_context; > + aio_context_acquire(commit_ctx); > + job_cancel_sync(commit_job); > + aio_context_release(commit_ctx); Anyway, there’s also the problem that I would guess the job_cancel_sync() might move the job from its current context back into the main context. Then we’d release the wrong context here. Max > } > > if (s->mode == REPLICATION_MODE_SECONDARY) { >
signature.asc
Description: OpenPGP digital signature