From patchwork Tue Jun 7 15:25:35 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Konrad Rzeszutek Wilk X-Patchwork-Id: 9161675 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 98EC660571 for ; Tue, 7 Jun 2016 15:28:39 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 87AB926E1A for ; Tue, 7 Jun 2016 15:28:39 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7CC2927248; Tue, 7 Jun 2016 15:28:39 +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=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id F248E26E1A for ; Tue, 7 Jun 2016 15:28:38 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bAIt9-0006bv-Dg; Tue, 07 Jun 2016 15:25:55 +0000 Received: from mail6.bemta6.messagelabs.com ([85.158.143.247]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bAIt8-0006bp-1Y for xen-devel@lists.xenproject.org; Tue, 07 Jun 2016 15:25:54 +0000 Received: from [85.158.143.35] by server-1.bemta-6.messagelabs.com id FC/2B-30266-187E6575; Tue, 07 Jun 2016 15:25:53 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrIIsWRWlGSWpSXmKPExsXSO6nOVbfheVi 4wftDRhbft0xmcmD0OPzhCksAYxRrZl5SfkUCa8bHxqyCa9oVCx+dZ21g/KnQxcjFISTQziTx rfcxI4TzmVGifUY3O4SzgVFi0eGNzBBOL6PE5Q8/2LoYOTlYBFQkDu5ZytrFyMHBJmAi8WaVI 0hYREBB4vWn80wgNrNAnMTuO61g5cICKRLtH5rAbF4BK4ndn05BLehjlHjzey0jREJQ4uTMJy wQzVoSN/69ZAKZzywgLbH8HwdImBMo/PbGM7D5ogLKEg0zHjCD2BICxhLtby+yTWAUnIVk0iw kk2YhTFrAyLyKUb04tagstUjXSC+pKDM9oyQ3MTNH19DATC83tbg4MT01JzGpWC85P3cTIzBo GYBgB+Oyv06HGCU5mJREeR96h4UL8SXlp1RmJBZnxBeV5qQWH2KU4eBQkuCd9gwoJ1iUmp5ak ZaZA4wfmLQEB4+SCG8uSJq3uCAxtzgzHSJ1ilFRSpxXGyQhAJLIKM2Da4PF7CVGWSlhXkagQ4 R4ClKLcjNLUOVfMYpzMCoJ8+4EmcKTmVcCN/0V0GImoMXLrgWDLC5JREhJNTCy2+UIOC35+Oi /rPhFnT+P/ugtbf1m1K5/2YT7edWu2ZYN9pNPK3u6TT9sHvtFpz1cd0rcyr3cB5tfBGqu/XRP SW7W/8uuCgvP9a9XeB5mvVl75RY+h8Lcfd3Vi/MXshsz+W/J2Gr6cdvb2sgHwTWzJeeFHFL1K btVVSb24m4dc+AZjWuvyiqUWIozEg21mIuKEwFhQrMs1AIAAA== X-Env-Sender: konrad.wilk@oracle.com X-Msg-Ref: server-5.tower-21.messagelabs.com!1465313151!17806494!1 X-Originating-IP: [141.146.126.69] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogMTQxLjE0Ni4xMjYuNjkgPT4gMjc3MjE4\n X-StarScan-Received: X-StarScan-Version: 8.46; banners=-,-,- X-VirusChecked: Checked Received: (qmail 4693 invoked from network); 7 Jun 2016 15:25:52 -0000 Received: from aserp1040.oracle.com (HELO aserp1040.oracle.com) (141.146.126.69) by server-5.tower-21.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 7 Jun 2016 15:25:52 -0000 Received: from aserv0022.oracle.com (aserv0022.oracle.com [141.146.126.234]) by aserp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id u57FPnt4010966 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 7 Jun 2016 15:25:49 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by aserv0022.oracle.com (8.13.8/8.13.8) with ESMTP id u57FPnHU007098 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 7 Jun 2016 15:25:49 GMT Received: from abhmp0017.oracle.com (abhmp0017.oracle.com [141.146.116.23]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id u57FPmA5022657; Tue, 7 Jun 2016 15:25:48 GMT Received: from localhost.localdomain (/172.56.37.181) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 07 Jun 2016 08:25:48 -0700 Date: Tue, 7 Jun 2016 11:25:35 -0400 From: Konrad Rzeszutek Wilk To: Bob Liu Message-ID: <20160607152524.GA10281@localhost.localdomain> References: <1464685157-30738-1-git-send-email-bob.liu@oracle.com> <20160531203307.GC23808@char.us.oracle.com> <574E7763.9060001@oracle.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <574E7763.9060001@oracle.com> User-Agent: Mutt/1.6.1 (2016-04-27) X-Source-IP: aserv0022.oracle.com [141.146.126.234] Cc: xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, roger.pau@citrix.com Subject: Re: [Xen-devel] [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Jun 01, 2016 at 01:49:23PM +0800, Bob Liu wrote: > > On 06/01/2016 04:33 AM, Konrad Rzeszutek Wilk wrote: > > On Tue, May 31, 2016 at 04:59:16PM +0800, Bob Liu wrote: > >> Sometimes blkfont may receive twice blkback_changed() notification after > >> migration, then talk_to_blkback() will be called twice too and confused > >> xen-blkback. > > > > Could you enlighten the patch description by having some form of > > state transition here? I am curious how you got the frontend > > to get in XenbusStateConnected (via blkif_recover right) and then > > the backend triggering the update once more? > > > > Or is just a simple race - the backend moves from XenbusStateConnected-> > > XenbusStateConnected - which retriggers the frontend to hit in > > blkback_changed the XenbusStateConnected state and go in there? > > (That would be in conenct_ring changing the state). But I don't > > see how the frontend_changed code get there as we have: > > > > 770 /* > > 771 * Ensure we connect even when two watches fire in > > 772 * close succession and we miss the intermediate value > > 773 * of frontend_state. > > 774 */ > > 775 if (dev->state == XenbusStateConnected) > > 776 break; > > 777 > > > > ? > > > > Now what about 'blkfront_connect' being called on the second time? > > > > Ah, info->connected is probably by then in BLKIF_STATE_CONNECTED > > (as blkif_recover changed) and we just reread the size of the disk. > > > > Is that how about the flow goes? > > blkfront blkback > blkfront_resume() > > talk_to_blkback() > > Set blkfront to XenbusStateInitialised > Front changed() > > Connect() > > Set blkback to XenbusStateConnected > > blkback_changed() > > Skip talk_to_blkback() > because frontstate == XenbusStateInitialised > > blkfront_connect() > > Set blkfront to XenbusStateConnected > > > ------------------------------------------------------------------ > But sometimes blkfront receives > blkback_changed() event more than once! I think I know why. The udev scripts that get invoked when when we attach a disk are a bit custom. As such I think they just revalidate the size leading to this. And this 'poke-at-XenbusStateConnected' state multiple times is allowed. It is used to signal disk changes (or just to revalidate). Hence it does not matter why really - we need to deal with this. I modified your patch a bit and are testing it: From e49dc9fc65eda4923b41d903ac51a7ddee182bcd Mon Sep 17 00:00:00 2001 From: Bob Liu Date: Tue, 7 Jun 2016 10:43:15 -0400 Subject: [PATCH] xen-blkfront: don't call talk_to_blkback when already connected to blkback Sometimes blkfront may twice receive blkback_changed() notification (XenbusStateConnected) after migration, which will cause talk_to_blkback() to be called twice too and confuse xen-blkback. The flow is as follow: blkfront blkback blkfront_resume() > talk_to_blkback() > Set blkfront to XenbusStateInitialised front changed() > Connect() > Set blkback to XenbusStateConnected blkback_changed() > Skip talk_to_blkback() because frontstate == XenbusStateInitialised > blkfront_connect() > Set blkfront to XenbusStateConnected ----- And here we get another XenbusStateConnected notification leading to: ----- blkback_changed() > because now frontstate != XenbusStateInitialised talk_to_blkback() is also called again > blkfront state changed from XenbusStateConnected to XenbusStateInitialised (Which is not correct!) front_changed(): > Do nothing because blkback already in XenbusStateConnected Now blkback is in XenbusStateConnected but blkfront is still in XenbusStateInitialised - leading to no disks. Poking of the XenbusStateConnected state is allowed (to deal with block disk change) and has to be dealt with. The most likely cause of this bug are custom udev scripts hooking up the disks and then validating the size. Signed-off-by: Bob Liu Signed-off-by: Konrad Rzeszutek Wilk --- drivers/block/xen-blkfront.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index b4b8fbd..7765ad5 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -2484,10 +2484,23 @@ static void blkback_changed(struct xenbus_device *dev, break; case XenbusStateConnected: - if (dev->state != XenbusStateInitialised) { + /* + * talk_to_blkback sets state to XenbusStateInitialised + * and blkfront_connect sets it to XenbusStateConnected + * (if connection went OK). + * + * If the backend (or toolstack) decides to poke at backend + * state (and re-trigger the watch by setting the state repeatedly + * to XenbusStateConnected (4)) we need to deal with this. + * This is allowed as this is used to communicate to the guest + * that the size of disk has changed! + */ + if ((dev->state != XenbusStateInitialised) && + (dev->state != XenbusStateConnected)) { if (talk_to_blkback(dev, info)) break; } + blkfront_connect(info); break;