On 08/22/2018 12:48 AM, Fam Zheng wrote:
However, I was unable to quickly audit whether all callers really did have
the lock (it balloons into whether all callers of job_finalize() have the
lock), so I'm reluctant to give R-b.
@@ -857,10 +849,10 @@ static void job_completed_txn_success(Job *job)
assert(other_job->ret == 0);
}
- job_txn_apply(txn, job_transition_to_pending, false);
+ job_txn_apply(txn, job_transition_to_pending);
/* If no jobs need manual finalization, automatically do so */
- if (job_txn_apply(txn, job_needs_finalize, false) == 0) {
+ if (job_txn_apply(txn, job_needs_finalize) == 0) {
job_do_finalize(job);
That said, the change makes sense here: since the first direct call to
job_txn_apply() did not need to lock, why should the second indirect call
through job_do_finalize() need it?
Or, is the fix to have job_do_finalize() gain a bool parameter, where
job_completed_txn_success() would pass false to that parameter, while
job_finalize() would pass true (again, going back to auditing whether all
callers of job_finalize() have already acquired the context).
There are two callers of job_finalize():
Okay, so the audit would have been easier if I had actually tried
grepping for it. Still, it can't hurt to make the commit message give
more details on what you checked, to make it easier for the reviewers.
fam@lemon:~/work/qemu [master]$ git grep -w -A1 '^\s*job_finalize'
blockdev.c: job_finalize(&job->job, errp);
blockdev.c- aio_context_release(aio_context);
--
job-qmp.c: job_finalize(job, errp);
job-qmp.c- aio_context_release(aio_context);
Yep, that's pretty conclusive that the context is already held by all
callers.
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
