From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39074) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buHRb-0003QX-Q6 for qemu-devel@nongnu.org; Wed, 12 Oct 2016 07:11:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buHRa-0001RE-Gx for qemu-devel@nongnu.org; Wed, 12 Oct 2016 07:11:31 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:40514 helo=relay.sw.ru) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buHRa-0001Qr-4h for qemu-devel@nongnu.org; Wed, 12 Oct 2016 07:11:30 -0400 References: <1475272849-19990-1-git-send-email-jsnow@redhat.com> <1475272849-19990-8-git-send-email-jsnow@redhat.com> From: Vladimir Sementsov-Ogievskiy Message-ID: <57FE1A4E.4040409@virtuozzo.com> Date: Wed, 12 Oct 2016 14:11:10 +0300 MIME-Version: 1.0 In-Reply-To: <1475272849-19990-8-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 07/11] blockjob: add .clean property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-block@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, stefanha@redhat.com, jcody@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org On 01.10.2016 01:00, John Snow wrote: > 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 > Signed-off-by: John Snow > --- > 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); > } > } > - I think empty line is removed by mistake, it returns back in 09 > + 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); > } > - and here > 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. -- Best regards, Vladimir