From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757356AbcFAFtj (ORCPT ); Wed, 1 Jun 2016 01:49:39 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:33647 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757317AbcFAFti (ORCPT ); Wed, 1 Jun 2016 01:49:38 -0400 Message-ID: <574E7763.9060001@oracle.com> Date: Wed, 01 Jun 2016 13:49:23 +0800 From: Bob Liu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130308 Thunderbird/17.0.4 MIME-Version: 1.0 To: Konrad Rzeszutek Wilk CC: linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org, roger.pau@citrix.com Subject: Re: [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback References: <1464685157-30738-1-git-send-email-bob.liu@oracle.com> <20160531203307.GC23808@char.us.oracle.com> In-Reply-To: <20160531203307.GC23808@char.us.oracle.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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! Not sure why. 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 XenbusStateConnected but blkfront still XenbusStateInitialised. >> >> Signed-off-by: Bob Liu >> --- >> drivers/block/xen-blkfront.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c >> index ca13df8..01aa460 100644 >> --- a/drivers/block/xen-blkfront.c >> +++ b/drivers/block/xen-blkfront.c >> @@ -2485,7 +2485,8 @@ static void blkback_changed(struct xenbus_device *dev, >> break; >> >> case XenbusStateConnected: >> - if (dev->state != XenbusStateInitialised) { >> + if ((dev->state != XenbusStateInitialised) && >> + (dev->state != XenbusStateConnected)) { >> if (talk_to_blkback(dev, info)) >> break; >> } >> -- >> 2.7.4 >>