From patchwork Tue Nov 15 04:14:40 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Cody X-Patchwork-Id: 9428959 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4BF5260471 for ; Tue, 15 Nov 2016 04:15:51 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2EE1828ABC for ; Tue, 15 Nov 2016 04:15:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 227EE28B4D; Tue, 15 Nov 2016 04:15:51 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id AEE8728ABC for ; Tue, 15 Nov 2016 04:15:50 +0000 (UTC) Received: from localhost ([::1]:44039 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6V9x-0002KJ-Tf for patchwork-qemu-devel@patchwork.kernel.org; Mon, 14 Nov 2016 23:15:49 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50523) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6V9C-0002Ak-9X for qemu-devel@nongnu.org; Mon, 14 Nov 2016 23:15:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6V9B-0008Bp-7W for qemu-devel@nongnu.org; Mon, 14 Nov 2016 23:15:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45606) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c6V98-0008A6-I6; Mon, 14 Nov 2016 23:14:58 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DBF988553D; Tue, 15 Nov 2016 04:14:57 +0000 (UTC) Received: from localhost (ovpn-112-50.phx2.redhat.com [10.3.112.50]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAF4EuT2015216 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 14 Nov 2016 23:14:57 -0500 From: Jeff Cody To: qemu-block@nongnu.org Date: Mon, 14 Nov 2016 23:14:40 -0500 Message-Id: <1479183291-14086-3-git-send-email-jcody@redhat.com> In-Reply-To: <1479183291-14086-1-git-send-email-jcody@redhat.com> References: <1479183291-14086-1-git-send-email-jcody@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 15 Nov 2016 04:14:57 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PULL for-2.8 02/13] blockjob: add .clean property X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: peter.maydell@linaro.org, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP From: John Snow 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_start. Reported-by: Vladimir Sementsov-Ogievskiy Signed-off-by: John Snow Reviewed-by: Kevin Wolf Message-id: 1478587839-9834-3-git-send-email-jsnow@redhat.com Signed-off-by: Jeff Cody --- block/backup.c | 15 ++++++++++----- blockjob.c | 3 +++ include/block/blockjob_int.h | 8 ++++++++ 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/block/backup.c b/block/backup.c index 7b5d8a3..734a24c 100644 --- a/block/backup.c +++ b/block/backup.c @@ -242,6 +242,14 @@ 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); + s->target = NULL; +} + static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context) { BackupBlockJob *s = container_of(job, BackupBlockJob, common); @@ -321,6 +329,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, .drain = backup_drain, }; @@ -343,12 +352,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); - s->target = NULL; - block_job_completed(job, data->ret); g_free(data); } @@ -658,7 +663,7 @@ void backup_start(const char *job_id, BlockDriverState *bs, bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL); } if (job) { - blk_unref(job->target); + backup_clean(&job->common); block_job_unref(&job->common); } } diff --git a/blockjob.c b/blockjob.c index 4d0ef53..e3c458c 100644 --- a/blockjob.c +++ b/blockjob.c @@ -241,6 +241,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); diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h index 40275e4..60d91a0 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.