From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49823) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dFK0b-0001b9-EH for qemu-devel@nongnu.org; Mon, 29 May 2017 08:42:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dFK0a-0002No-Is for qemu-devel@nongnu.org; Mon, 29 May 2017 08:42:53 -0400 References: <1496060313-30190-1-git-send-email-kwolf@redhat.com> From: Paolo Bonzini Message-ID: <80923795-fa11-3c47-8dba-81f38574f100@redhat.com> Date: Mon, 29 May 2017 14:42:38 +0200 MIME-Version: 1.0 In-Reply-To: <1496060313-30190-1-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] mirror: Drop permissions on s->target on completion List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: mreitz@redhat.com, famz@redhat.com, ymankad@redhat.com, qemu-devel@nongnu.org, qemu-stable@nongnu.org On 29/05/2017 14:18, Kevin Wolf wrote: > This fixes an assertion failure that was triggered by qemu-iotests 129 > on some CI host, while the same test case didn't seem to fail on other > hosts. > > Essentially the problem is that the blk_unref(s->target) in > mirror_exit() doesn't necessarily mean that the BlockBackend goes away > immediately. It is possible that the job completion was triggered nested > in mirror_drain(), which looks like this: > > BlockBackend *target = s->target; > blk_ref(target); > blk_drain(target); > blk_unref(target); > > In this case, the write permissions for s->target are retained until > after blk_drain(), which makes removing mirror_top_bs fail for the > active commit case (can't have a writable backing file in the chain > without the filter driver). > > Explicitly dropping the permissions first means that the additional > reference doesn't hurt and the job can complete successfully even if > called from the nested blk_drain(). > > Cc: qemu-stable@nongnu.org > Signed-off-by: Kevin Wolf > --- > block/mirror.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/block/mirror.c b/block/mirror.c > index e86f8f8..e778ee0 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -514,7 +514,12 @@ static void mirror_exit(BlockJob *job, void *opaque) > > /* Remove target parent that still uses BLK_PERM_WRITE/RESIZE before > * inserting target_bs at s->to_replace, where we might not be able to get > - * these permissions. */ > + * these permissions. > + * > + * Note that blk_unref() alone doesn't necessarily drop permissions because > + * we might be running nested inside mirror_drain(), which takes an extra > + * reference, so use an explicit blk_set_perm() first. */ > + blk_set_perm(s->target, 0, BLK_PERM_ALL, &error_abort); > blk_unref(s->target); > s->target = NULL; > > Thanks, this looks good. Paolo