[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PATCH v2 07/11] blockjob: add .clean property
From: |
John Snow |
Subject: |
[Qemu-devel] [PATCH v2 07/11] blockjob: add .clean property |
Date: |
Fri, 30 Sep 2016 18:00:45 -0400 |
Cleaning up after we have deferred to the main thread but before the
transaction has converged can be dangerous and result in deadlocks
if the job cleanup invokes any BH polling loops.
A job may attempt to begin cleaning up, but may induce another job to
enter its cleanup routine. The second job, part of our same transaction,
will block waiting for the first job to finish, so neither job may now
make progress.
To rectify this, allow jobs to register a cleanup operation that will
always run regardless of if the job was in a transaction or not, and
if the transaction job group completed successfully or not.
Move sensitive cleanup to this callback instead which is guaranteed to
be run only after the transaction has converged, which removes sensitive
timing constraints from said cleanup.
Furthermore, in future patches these cleanup operations will be performed
regardless of whether or not we actually started the job. Therefore,
cleanup callbacks should essentially confine themselves to undoing create
operations, e.g. setup actions taken in what is now backup_run.
Reported-by: Vladimir Sementsov-Ogievskiy <address@hidden>
Signed-off-by: John Snow <address@hidden>
---
block/backup.c | 11 ++++++++---
blockjob.c | 5 +++--
include/block/blockjob_int.h | 8 ++++++++
3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index d667482..42ff4c0 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -242,6 +242,13 @@ static void backup_abort(BlockJob *job)
}
}
+static void backup_clean(BlockJob *job)
+{
+ BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+ assert(s->target);
+ blk_unref(s->target);
+}
+
static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common);
@@ -306,6 +313,7 @@ static const BlockJobDriver backup_job_driver = {
.set_speed = backup_set_speed,
.commit = backup_commit,
.abort = backup_abort,
+ .clean = backup_clean,
.attached_aio_context = backup_attached_aio_context,
};
@@ -327,11 +335,8 @@ typedef struct {
static void backup_complete(BlockJob *job, void *opaque)
{
- BackupBlockJob *s = container_of(job, BackupBlockJob, common);
BackupCompleteData *data = opaque;
- blk_unref(s->target);
-
block_job_completed(job, data->ret);
g_free(data);
}
diff --git a/blockjob.c b/blockjob.c
index 09fb602..44cbf6c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -217,7 +217,9 @@ static void block_job_completed_single(BlockJob *job)
job->driver->abort(job);
}
}
-
+ if (job->driver->clean) {
+ job->driver->clean(job);
+ }
if (job->cb) {
job->cb(job->opaque, job->ret);
}
@@ -230,7 +232,6 @@ static void block_job_completed_single(BlockJob *job)
}
block_job_event_completed(job, msg);
}
-
if (job->txn) {
QLIST_REMOVE(job, txn_list);
block_job_txn_unref(job->txn);
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index c6da7e4..b7aeaef 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -74,6 +74,14 @@ struct BlockJobDriver {
void (*abort)(BlockJob *job);
/**
+ * If the callback is not NULL, it will be invoked after a call to either
+ * .commit() or .abort(). Regardless of which callback is invoked after
+ * completion, .clean() will always be called, even if the job does not
+ * belong to a transaction group.
+ */
+ void (*clean)(BlockJob *job);
+
+ /**
* If the callback is not NULL, it will be invoked when the job transitions
* into the paused state. Paused jobs must not perform any asynchronous
* I/O or event loop activity. This callback is used to quiesce jobs.
--
2.7.4
- [Qemu-devel] [PATCH v2 00/11] blockjobs: Fix transactional race condition, John Snow, 2016/09/30
- [Qemu-devel] [PATCH v2 01/11] blockjob: fix dead pointer in txn list, John Snow, 2016/09/30
- [Qemu-devel] [PATCH v2 03/11] Blockjobs: Internalize user_pause logic, John Snow, 2016/09/30
- [Qemu-devel] [PATCH v2 02/11] blockjob: centralize QMP event emissions, John Snow, 2016/09/30
- [Qemu-devel] [PATCH v2 04/11] blockjobs: Always use block_job_get_aio_context, John Snow, 2016/09/30
- [Qemu-devel] [PATCH v2 06/11] blockjobs: fix documentation, John Snow, 2016/09/30
- [Qemu-devel] [PATCH v2 07/11] blockjob: add .clean property,
John Snow <=
- [Qemu-devel] [PATCH v2 05/11] blockjobs: split interface into public/private, John Snow, 2016/09/30
- [Qemu-devel] [PATCH v2 08/11] blockjob: add .start field, John Snow, 2016/09/30
- [Qemu-devel] [PATCH v2 09/11] blockjob: add block_job_start, John Snow, 2016/09/30
- [Qemu-devel] [PATCH v2 10/11] blockjob: refactor backup_start as backup_job_create, John Snow, 2016/09/30
- [Qemu-devel] [PATCH v2 11/11] iotests: add transactional failure race test, John Snow, 2016/09/30
- Re: [Qemu-devel] [PATCH v2 00/11] blockjobs: Fix transactional race condition, no-reply, 2016/09/30