yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone

Blame SOURCES/kvm-job-take-each-job-s-lock-individually-in-job_txn_app.patch

ddf19c
From 3f16b8a33bd7503cbe857fbeb45fff7301b6bb5f Mon Sep 17 00:00:00 2001
ddf19c
From: Kevin Wolf <kwolf@redhat.com>
ddf19c
Date: Wed, 8 Apr 2020 17:29:12 +0100
ddf19c
Subject: [PATCH 1/6] job: take each job's lock individually in job_txn_apply
ddf19c
ddf19c
RH-Author: Kevin Wolf <kwolf@redhat.com>
ddf19c
Message-id: <20200408172917.18712-2-kwolf@redhat.com>
ddf19c
Patchwork-id: 94597
ddf19c
O-Subject: [RHEL-AV-8.2.0 qemu-kvm PATCH 1/6] job: take each job's lock individually in job_txn_apply
ddf19c
Bugzilla: 1817621
ddf19c
RH-Acked-by: Eric Blake <eblake@redhat.com>
ddf19c
RH-Acked-by: Danilo de Paula <ddepaula@redhat.com>
ddf19c
RH-Acked-by: Max Reitz <mreitz@redhat.com>
ddf19c
ddf19c
From: Stefan Reiter <s.reiter@proxmox.com>
ddf19c
ddf19c
All callers of job_txn_apply hold a single job's lock, but different
ddf19c
jobs within a transaction can have different contexts, thus we need to
ddf19c
lock each one individually before applying the callback function.
ddf19c
ddf19c
Similar to job_completed_txn_abort this also requires releasing the
ddf19c
caller's context before and reacquiring it after to avoid recursive
ddf19c
locks which might break AIO_WAIT_WHILE in the callback. This is safe, since
ddf19c
existing code would already have to take this into account, lest
ddf19c
job_completed_txn_abort might have broken.
ddf19c
ddf19c
This also brings to light a different issue: When a callback function in
ddf19c
job_txn_apply moves it's job to a different AIO context, callers will
ddf19c
try to release the wrong lock (now that we re-acquire the lock
ddf19c
correctly, previously it would just continue with the old lock, leaving
ddf19c
the job unlocked for the rest of the return path). Fix this by not caching
ddf19c
the job's context.
ddf19c
ddf19c
This is only necessary for qmp_block_job_finalize, qmp_job_finalize and
ddf19c
job_exit, since everyone else calls through job_exit.
ddf19c
ddf19c
One test needed adapting, since it calls job_finalize directly, so it
ddf19c
manually needs to acquire the correct context.
ddf19c
ddf19c
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
ddf19c
Message-Id: <20200407115651.69472-2-s.reiter@proxmox.com>
ddf19c
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
ddf19c
(cherry picked from commit b660a84bbb0eb1a76b505648d31d5e82594fb75e)
ddf19c
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
ddf19c
Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>
ddf19c
---
ddf19c
 blockdev.c            |  9 +++++++++
ddf19c
 job-qmp.c             |  9 +++++++++
ddf19c
 job.c                 | 50 ++++++++++++++++++++++++++++++++++++++++----------
ddf19c
 tests/test-blockjob.c |  2 ++
ddf19c
 4 files changed, 60 insertions(+), 10 deletions(-)
ddf19c
ddf19c
diff --git a/blockdev.c b/blockdev.c
ddf19c
index c8d4b51..86eb115 100644
ddf19c
--- a/blockdev.c
ddf19c
+++ b/blockdev.c
ddf19c
@@ -4215,7 +4215,16 @@ void qmp_block_job_finalize(const char *id, Error **errp)
ddf19c
     }
ddf19c
 
ddf19c
     trace_qmp_block_job_finalize(job);
ddf19c
+    job_ref(&job->job);
ddf19c
     job_finalize(&job->job, errp);
ddf19c
+
ddf19c
+    /*
ddf19c
+     * Job's context might have changed via job_finalize (and job_txn_apply
ddf19c
+     * automatically acquires the new one), so make sure we release the correct
ddf19c
+     * one.
ddf19c
+     */
ddf19c
+    aio_context = blk_get_aio_context(job->blk);
ddf19c
+    job_unref(&job->job);
ddf19c
     aio_context_release(aio_context);
ddf19c
 }
ddf19c
 
ddf19c
diff --git a/job-qmp.c b/job-qmp.c
ddf19c
index fbfed25..a201220 100644
ddf19c
--- a/job-qmp.c
ddf19c
+++ b/job-qmp.c
ddf19c
@@ -114,7 +114,16 @@ void qmp_job_finalize(const char *id, Error **errp)
ddf19c
     }
ddf19c
 
ddf19c
     trace_qmp_job_finalize(job);
ddf19c
+    job_ref(job);
ddf19c
     job_finalize(job, errp);
ddf19c
+
ddf19c
+    /*
ddf19c
+     * Job's context might have changed via job_finalize (and job_txn_apply
ddf19c
+     * automatically acquires the new one), so make sure we release the correct
ddf19c
+     * one.
ddf19c
+     */
ddf19c
+    aio_context = job->aio_context;
ddf19c
+    job_unref(job);
ddf19c
     aio_context_release(aio_context);
ddf19c
 }
ddf19c
 
ddf19c
diff --git a/job.c b/job.c
ddf19c
index 04409b4..48fc4ad 100644
ddf19c
--- a/job.c
ddf19c
+++ b/job.c
ddf19c
@@ -136,17 +136,38 @@ static void job_txn_del_job(Job *job)
ddf19c
     }
ddf19c
 }
ddf19c
 
ddf19c
-static int job_txn_apply(JobTxn *txn, int fn(Job *))
ddf19c
+static int job_txn_apply(Job *job, int fn(Job *))
ddf19c
 {
ddf19c
-    Job *job, *next;
ddf19c
+    AioContext *inner_ctx;
ddf19c
+    Job *other_job, *next;
ddf19c
+    JobTxn *txn = job->txn;
ddf19c
     int rc = 0;
ddf19c
 
ddf19c
-    QLIST_FOREACH_SAFE(job, &txn->jobs, txn_list, next) {
ddf19c
-        rc = fn(job);
ddf19c
+    /*
ddf19c
+     * Similar to job_completed_txn_abort, we take each job's lock before
ddf19c
+     * applying fn, but since we assume that outer_ctx is held by the caller,
ddf19c
+     * we need to release it here to avoid holding the lock twice - which would
ddf19c
+     * break AIO_WAIT_WHILE from within fn.
ddf19c
+     */
ddf19c
+    job_ref(job);
ddf19c
+    aio_context_release(job->aio_context);
ddf19c
+
ddf19c
+    QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
ddf19c
+        inner_ctx = other_job->aio_context;
ddf19c
+        aio_context_acquire(inner_ctx);
ddf19c
+        rc = fn(other_job);
ddf19c
+        aio_context_release(inner_ctx);
ddf19c
         if (rc) {
ddf19c
             break;
ddf19c
         }
ddf19c
     }
ddf19c
+
ddf19c
+    /*
ddf19c
+     * Note that job->aio_context might have been changed by calling fn, so we
ddf19c
+     * can't use a local variable to cache it.
ddf19c
+     */
ddf19c
+    aio_context_acquire(job->aio_context);
ddf19c
+    job_unref(job);
ddf19c
     return rc;
ddf19c
 }
ddf19c
 
ddf19c
@@ -774,11 +795,11 @@ static void job_do_finalize(Job *job)
ddf19c
     assert(job && job->txn);
ddf19c
 
ddf19c
     /* prepare the transaction to complete */
ddf19c
-    rc = job_txn_apply(job->txn, job_prepare);
ddf19c
+    rc = job_txn_apply(job, job_prepare);
ddf19c
     if (rc) {
ddf19c
         job_completed_txn_abort(job);
ddf19c
     } else {
ddf19c
-        job_txn_apply(job->txn, job_finalize_single);
ddf19c
+        job_txn_apply(job, job_finalize_single);
ddf19c
     }
ddf19c
 }
ddf19c
 
ddf19c
@@ -824,10 +845,10 @@ static void job_completed_txn_success(Job *job)
ddf19c
         assert(other_job->ret == 0);
ddf19c
     }
ddf19c
 
ddf19c
-    job_txn_apply(txn, job_transition_to_pending);
ddf19c
+    job_txn_apply(job, job_transition_to_pending);
ddf19c
 
ddf19c
     /* If no jobs need manual finalization, automatically do so */
ddf19c
-    if (job_txn_apply(txn, job_needs_finalize) == 0) {
ddf19c
+    if (job_txn_apply(job, job_needs_finalize) == 0) {
ddf19c
         job_do_finalize(job);
ddf19c
     }
ddf19c
 }
ddf19c
@@ -849,9 +870,10 @@ static void job_completed(Job *job)
ddf19c
 static void job_exit(void *opaque)
ddf19c
 {
ddf19c
     Job *job = (Job *)opaque;
ddf19c
-    AioContext *ctx = job->aio_context;
ddf19c
+    AioContext *ctx;
ddf19c
 
ddf19c
-    aio_context_acquire(ctx);
ddf19c
+    job_ref(job);
ddf19c
+    aio_context_acquire(job->aio_context);
ddf19c
 
ddf19c
     /* This is a lie, we're not quiescent, but still doing the completion
ddf19c
      * callbacks. However, completion callbacks tend to involve operations that
ddf19c
@@ -862,6 +884,14 @@ static void job_exit(void *opaque)
ddf19c
 
ddf19c
     job_completed(job);
ddf19c
 
ddf19c
+    /*
ddf19c
+     * Note that calling job_completed can move the job to a different
ddf19c
+     * aio_context, so we cannot cache from above. job_txn_apply takes care of
ddf19c
+     * acquiring the new lock, and we ref/unref to avoid job_completed freeing
ddf19c
+     * the job underneath us.
ddf19c
+     */
ddf19c
+    ctx = job->aio_context;
ddf19c
+    job_unref(job);
ddf19c
     aio_context_release(ctx);
ddf19c
 }
ddf19c
 
ddf19c
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
ddf19c
index 7844c9f..6d857fd 100644
ddf19c
--- a/tests/test-blockjob.c
ddf19c
+++ b/tests/test-blockjob.c
ddf19c
@@ -368,7 +368,9 @@ static void test_cancel_concluded(void)
ddf19c
     aio_poll(qemu_get_aio_context(), true);
ddf19c
     assert(job->status == JOB_STATUS_PENDING);
ddf19c
 
ddf19c
+    aio_context_acquire(job->aio_context);
ddf19c
     job_finalize(job, &error_abort);
ddf19c
+    aio_context_release(job->aio_context);
ddf19c
     assert(job->status == JOB_STATUS_CONCLUDED);
ddf19c
 
ddf19c
     cancel_common(s);
ddf19c
-- 
ddf19c
1.8.3.1
ddf19c