On 7/25/22 10:38, Emanuele Giuseppe Esposito wrote:
With "intact" we mean that all job.h functions implicitly
take the lock. Therefore API callers are unmodified.
This means that:
- many static functions that will be always called with job lock held
become _locked, and call _locked functions
- all public functions take the lock internally if needed, and call _locked
functions
- all public functions called internally by other functions in job.c will have a
_locked counterpart (sometimes public), to avoid deadlocks (job lock already
taken).
These functions are not used for now.
- some public functions called only from exernal files (not job.c) do not
have _locked() counterpart and take the lock inside. Others won't need
the lock at all because use fields only set at initialization and
never modified.
job_{lock/unlock} is independent from real_job_{lock/unlock}.
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
Honestly, you've changed the patch enough to drop my r-b.
Signed-off-by: Emanuele Giuseppe Esposito <eespo...@redhat.com>
---
include/qemu/job.h | 138 ++++++++++-
[..]
+/* Called with job_mutex held, but releases it temporarily */
+static int job_finalize_single_locked(Job *job)
{
- assert(job_is_completed(job));
+ int job_ret;
+
+ assert(job_is_completed_locked(job));
/* Ensure abort is called for late-transactional failures */
- job_update_rc(job);
+ job_update_rc_locked(job);
+
+ job_unlock();
With this new variant of code you read job->ret not under mutex.. Is it correct?
Probably it's OK, as here we shouldn't race with something other.. But it's simple to just move
job_unlock() to beginning of "if" body, and copy to the beginning of "else"
body.
if (!job->ret) {
job_commit(job);
@@ -739,29 +895,37 @@ static int job_finalize_single(Job *job)
}
job_clean(job);
+ job_lock();
+
if (job->cb) {
- job->cb(job->opaque, job->ret);
+ job_ret = job->ret;
+ job_unlock();
+ job->cb(job->opaque, job_ret);
+ job_lock();
}
[..]
--
Best regards,
Vladimir