Re: [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback

[Konrad Rzeszutek Wilk]

On Wed, Jun 08, 2016 at 02:46:38PM +0800, Bob Liu wrote:
> 
> On 06/07/2016 11:25 PM, Konrad Rzeszutek Wilk wrote:
> > 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:
> > 
> 
> Looks much better, thank you very much!

Great! I also had it tested overnight and there was no hitch will send it
out soon.