From patchwork Wed Apr 9 18:29:13 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Cody X-Patchwork-Id: 337909 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org 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 ozlabs.org (Postfix) with ESMTPS id C536B1400E4 for ; Thu, 10 Apr 2014 04:29:51 +1000 (EST) Received: from localhost ([::1]:47576 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WXxFt-0007Ds-Sm for incoming@patchwork.ozlabs.org; Wed, 09 Apr 2014 14:29:49 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55459) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WXxFV-0007CB-1V for qemu-devel@nongnu.org; Wed, 09 Apr 2014 14:29:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WXxFO-0003b6-Ho for qemu-devel@nongnu.org; Wed, 09 Apr 2014 14:29:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29971) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WXxFO-0003b0-9Z for qemu-devel@nongnu.org; Wed, 09 Apr 2014 14:29:18 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s39ITFBq011826 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 9 Apr 2014 14:29:15 -0400 Received: from localhost ([10.3.112.13]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s39ITDJA017607 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Wed, 9 Apr 2014 14:29:14 -0400 Date: Wed, 9 Apr 2014 14:29:13 -0400 From: Jeff Cody To: Fam Zheng Message-ID: <20140409182913.GC8087@localhost.localdomain> References: <1394436370-8908-1-git-send-email-famz@redhat.com> <1394436370-8908-7-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1394436370-8908-7-git-send-email-famz@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, benoit.canet@irqsave.net, rjones@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, ptoscano@redhat.com, imain@redhat.com, stefanha@redhat.com, pbonzini@redhat.com Subject: Re: [Qemu-devel] [PATCH v17 06/14] block: Add backing_blocker in BlockDriverState X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Mon, Mar 10, 2014 at 03:26:02PM +0800, Fam Zheng wrote: > This makes use of op_blocker and blocks all the operations except for > commit target, on each BlockDriverState->backing_hd. > > The asserts for op_blocker in bdrv_swap are removed because with this > change, the target of block commit has at least the backing blocker of > its child, so the assertion is not true. Callers should do their check. > > Signed-off-by: Fam Zheng > --- > block.c | 24 ++++++++++++++++++++---- > block/mirror.c | 1 + > include/block/block_int.h | 3 +++ > 3 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index 64738dc..95247c8 100644 > --- a/block.c > +++ b/block.c > @@ -1050,16 +1050,33 @@ fail: > void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > { > > + if (bs->backing_hd) { > + assert(error_is_set(&bs->backing_blocker)); > + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); > + } else if (backing_hd) { > + error_setg(&bs->backing_blocker, > + "device is used as backing hd of '%s'", > + bs->device_name); > + } > + > bs->backing_hd = backing_hd; > if (!backing_hd) { > bs->backing_file[0] = '\0'; > bs->backing_format[0] = '\0'; > + if (error_is_set(&bs->backing_blocker)) { > + error_free(bs->backing_blocker); > + } > goto out; > } > bs->open_flags &= ~BDRV_O_NO_BACKING; > pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); > pstrcpy(bs->backing_format, sizeof(bs->backing_format), > backing_hd->drv ? backing_hd->drv->format_name : ""); > + > + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker); > + /* Otherwise we won't be able to commit due to check in bdrv_commit */ > + bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, > + bs->backing_blocker); > out: > bdrv_refresh_limits(bs); > } > @@ -1699,8 +1716,9 @@ void bdrv_close(BlockDriverState *bs) > > if (bs->drv) { > if (bs->backing_hd) { > - bdrv_unref(bs->backing_hd); > - bs->backing_hd = NULL; > + BlockDriverState *backing_hd = bs->backing_hd; > + bdrv_set_backing_hd(bs, NULL); > + bdrv_unref(backing_hd); > } > bs->drv->bdrv_close(bs); > g_free(bs->opaque); > @@ -1908,7 +1926,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) > assert(QLIST_EMPTY(&bs_new->dirty_bitmaps)); > assert(bs_new->job == NULL); > assert(bs_new->dev == NULL); > - assert(bdrv_op_blocker_is_empty(bs_new)); > assert(bs_new->io_limits_enabled == false); > assert(!throttle_have_timer(&bs_new->throttle_state)); > > @@ -1927,7 +1944,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) > /* Check a few fields that should remain attached to the device */ > assert(bs_new->dev == NULL); > assert(bs_new->job == NULL); > - assert(bdrv_op_blocker_is_empty(bs_new)); > assert(bs_new->io_limits_enabled == false); > assert(!throttle_have_timer(&bs_new->throttle_state)); > > diff --git a/block/mirror.c b/block/mirror.c > index dd5ee05..6dc84e8 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -487,6 +487,7 @@ immediate_exit: > * trigger the unref from the top one */ > BlockDriverState *p = s->base->backing_hd; > s->base->backing_hd = NULL; > + bdrv_op_unblock_all(p, s->base->backing_blocker); > bdrv_unref(p); FYI, this is what I changed in my testing, to try out the active layer case in bdrv_drop_intermediate(). Since you'll need to respin anyway, might as well clean this up to use the updated bdrv_drop_intermediate(): --- a/block/mirror.c +++ b/block/mirror.c @@ -490,15 +490,8 @@ immediate_exit: if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) { bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL); } - bdrv_swap(s->target, s->common.bs); - if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) { - /* drop the bs loop chain formed by the swap: break the loop then - * trigger the unref from the top one */ - BlockDriverState *p = s->base->backing_hd; - s->base->backing_hd = NULL; - bdrv_op_unblock_all(p, s->base->backing_blocker); - bdrv_unref(p); - } + bdrv_op_unblock_all(s->common.bs->backing_hd, s->common.bs->backing_blocker); + bdrv_drop_intermediate(s->common.bs, s->common.bs, s->target); } bdrv_unref(s->target); block_job_completed(&s->common, ret);