From patchwork Thu Mar 7 14:03:12 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Lopez Pascual X-Patchwork-Id: 10842963 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 6C6E9922 for ; Thu, 7 Mar 2019 14:05:51 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5B49D283BB for ; Thu, 7 Mar 2019 14:05:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 597EA2EDDD; Thu, 7 Mar 2019 14:05: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=-2.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id EC2B5283BB for ; Thu, 7 Mar 2019 14:05:50 +0000 (UTC) Received: from localhost ([127.0.0.1]:52353 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h1tef-0005BV-Se for patchwork-qemu-devel@patchwork.kernel.org; Thu, 07 Mar 2019 09:05:49 -0500 Received: from eggs.gnu.org ([209.51.188.92]:39958) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h1tcj-0003lN-2t for qemu-devel@nongnu.org; Thu, 07 Mar 2019 09:03:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1h1tcd-0002hF-Ax for qemu-devel@nongnu.org; Thu, 07 Mar 2019 09:03:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49006) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1h1tcV-0002Qa-Ce; Thu, 07 Mar 2019 09:03:35 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D5395C05FF85; Thu, 7 Mar 2019 14:03:32 +0000 (UTC) Received: from dritchie.redhat.com (unknown [10.33.36.137]) by smtp.corp.redhat.com (Postfix) with ESMTP id D7A345D788; Thu, 7 Mar 2019 14:03:25 +0000 (UTC) From: Sergio Lopez To: qemu-block@nongnu.org Date: Thu, 7 Mar 2019 15:03:12 +0100 Message-Id: <20190307140312.28072-1-slp@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 07 Mar 2019 14:03:32 +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] [PATCH] mirror: Confirm we're quiesced only if the job is paused or cancelled 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: kwolf@redhat.com, Sergio Lopez , qemu-devel@nongnu.org, stefanha@redhat.com, mreitz@redhat.com Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP While child_job_drained_begin() calls to job_pause(), the job doesn't actually transition between states until it runs again and reaches a pause point. This means bdrv_drained_begin() may return with some jobs using the node still having 'busy == true'. As a consequence, block_job_detach_aio_context() may get into a deadlock, waiting for the job to be actually paused, while the coroutine servicing the job is yielding and doesn't get the opportunity to get scheduled again. This situation can be reproduced by issuing a 'block-commit' immediately followed by a 'device_del'. To ensure bdrv_drained_begin() only returns when the jobs have been paused, we change mirror_drained_poll() to only confirm it's quiesced when job->paused == true and there aren't any in-flight requests, except if we reached that point by a drained section initiated by the mirror/commit job itself. The other block jobs shouldn't need any changes, as the default drained_poll() behavior is to only confirm it's quiesced if the job is not busy or completed. Signed-off-by: Sergio Lopez --- block/mirror.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/block/mirror.c b/block/mirror.c index 726d3c27fb..b7f076f97c 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -80,6 +80,7 @@ typedef struct MirrorBlockJob { bool initial_zeroing_ongoing; int in_active_write_counter; bool prepared; + bool in_drain; } MirrorBlockJob; typedef struct MirrorBDSOpaque { @@ -679,9 +680,11 @@ static int mirror_exit_common(Job *job) /* The mirror job has no requests in flight any more, but we need to * drain potential other users of the BDS before changing the graph. */ + s->in_drain = true; bdrv_drained_begin(target_bs); bdrv_replace_node(to_replace, target_bs, &local_err); bdrv_drained_end(target_bs); + s->in_drain = false; if (local_err) { error_report_err(local_err); ret = -EPERM; @@ -717,6 +720,7 @@ static int mirror_exit_common(Job *job) bs_opaque->job = NULL; bdrv_drained_end(src); + s->in_drain = false; bdrv_unref(mirror_top_bs); bdrv_unref(src); @@ -1000,10 +1004,12 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) */ trace_mirror_before_drain(s, cnt); + s->in_drain = true; bdrv_drained_begin(bs); cnt = bdrv_get_dirty_count(s->dirty_bitmap); if (cnt > 0 || mirror_flush(s) < 0) { bdrv_drained_end(bs); + s->in_drain = false; continue; } @@ -1051,6 +1057,7 @@ immediate_exit: bdrv_dirty_iter_free(s->dbi); if (need_drain) { + s->in_drain = true; bdrv_drained_begin(bs); } @@ -1119,6 +1126,16 @@ static void coroutine_fn mirror_pause(Job *job) static bool mirror_drained_poll(BlockJob *job) { MirrorBlockJob *s = container_of(job, MirrorBlockJob, common); + + /* If the job isn't paused nor cancelled, we can't be sure that it won't + * issue more requets. We make an exception if we've reached this point + * from one of our own drain sections, to avoid a deadlock waiting for + * ourselves. + */ + if (!s->common.job.paused && !s->common.job.cancelled && !s->in_drain) { + return true; + } + return !!s->in_flight; }