* [PATCH 2/2] xen-blkfront: fix resume issues [not found] <1464685157-30738-1-git-send-email-bob.liu@oracle.com> @ 2016-05-31 8:59 ` Bob Liu 2016-05-31 20:33 ` [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback Konrad Rzeszutek Wilk [not found] ` <20160531203307.GC23808@char.us.oracle.com> 2 siblings, 0 replies; 10+ messages in thread From: Bob Liu @ 2016-05-31 8:59 UTC (permalink / raw) To: linux-kernel; +Cc: xen-devel, Bob Liu, roger.pau After migrate to another host, the number of rings(block hardware queues) may be changed and the ring info structure will also be reallocated. This patch fix two related place: * call blk_mq_update_nr_hw_queues() to make blk-core knows the number of hardware queues have been changed. * Don't store rinfo pointer to hctx->driver_data, because rinfo may be * reallocated so using hctx->queue_num to get the rinfo structure instead. Signed-off-by: Bob Liu <bob.liu@oracle.com> --- drivers/block/xen-blkfront.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index 01aa460..83e36c5 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -874,8 +874,12 @@ static int blkif_queue_rq(struct blk_mq_hw_ctx *hctx, const struct blk_mq_queue_data *qd) { unsigned long flags; - struct blkfront_ring_info *rinfo = (struct blkfront_ring_info *)hctx->driver_data; + int qid = hctx->queue_num; + struct blkfront_info *info = hctx->queue->queuedata; + struct blkfront_ring_info *rinfo = NULL; + BUG_ON(info->nr_rings <= qid); + rinfo = &info->rinfo[qid]; blk_mq_start_request(qd->rq); spin_lock_irqsave(&rinfo->ring_lock, flags); if (RING_FULL(&rinfo->ring)) @@ -901,20 +905,9 @@ out_busy: return BLK_MQ_RQ_QUEUE_BUSY; } -static int blk_mq_init_hctx(struct blk_mq_hw_ctx *hctx, void *data, - unsigned int index) -{ - struct blkfront_info *info = (struct blkfront_info *)data; - - BUG_ON(info->nr_rings <= index); - hctx->driver_data = &info->rinfo[index]; - return 0; -} - static struct blk_mq_ops blkfront_mq_ops = { .queue_rq = blkif_queue_rq, .map_queue = blk_mq_map_queue, - .init_hctx = blk_mq_init_hctx, }; static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size, @@ -950,6 +943,7 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size, return PTR_ERR(rq); } + rq->queuedata = info; queue_flag_set_unlocked(QUEUE_FLAG_VIRT, rq); if (info->feature_discard) { @@ -2149,6 +2143,8 @@ static int blkfront_resume(struct xenbus_device *dev) return err; err = talk_to_blkback(dev, info); + if (!err) + blk_mq_update_nr_hw_queues(&info->tag_set, info->nr_rings); /* * We have to wait for the backend to switch to -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback [not found] <1464685157-30738-1-git-send-email-bob.liu@oracle.com> 2016-05-31 8:59 ` [PATCH 2/2] xen-blkfront: fix resume issues Bob Liu @ 2016-05-31 20:33 ` Konrad Rzeszutek Wilk [not found] ` <20160531203307.GC23808@char.us.oracle.com> 2 siblings, 0 replies; 10+ messages in thread From: Konrad Rzeszutek Wilk @ 2016-05-31 20:33 UTC (permalink / raw) To: Bob Liu; +Cc: xen-devel, linux-kernel, roger.pau 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? > > Signed-off-by: Bob Liu <bob.liu@oracle.com> > --- > 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 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <20160531203307.GC23808@char.us.oracle.com>]
* Re: [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback [not found] ` <20160531203307.GC23808@char.us.oracle.com> @ 2016-06-01 5:49 ` Bob Liu [not found] ` <574E7763.9060001@oracle.com> 1 sibling, 0 replies; 10+ messages in thread From: Bob Liu @ 2016-06-01 5:49 UTC (permalink / raw) To: Konrad Rzeszutek Wilk; +Cc: xen-devel, linux-kernel, roger.pau 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 <bob.liu@oracle.com> >> --- >> 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 >> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <574E7763.9060001@oracle.com>]
* Re: [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback [not found] ` <574E7763.9060001@oracle.com> @ 2016-06-02 14:30 ` Konrad Rzeszutek Wilk 2016-06-07 15:25 ` Konrad Rzeszutek Wilk [not found] ` <20160607152524.GA10281@localhost.localdomain> 2 siblings, 0 replies; 10+ messages in thread From: Konrad Rzeszutek Wilk @ 2016-06-02 14:30 UTC (permalink / raw) To: Bob Liu; +Cc: xen-devel, linux-kernel, roger.pau 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! Could the control stack (xend) be doing this? > 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. Ah! > > >> > >> Signed-off-by: Bob Liu <bob.liu@oracle.com> > >> --- > >> 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 > >> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback [not found] ` <574E7763.9060001@oracle.com> 2016-06-02 14:30 ` Konrad Rzeszutek Wilk @ 2016-06-07 15:25 ` Konrad Rzeszutek Wilk [not found] ` <20160607152524.GA10281@localhost.localdomain> 2 siblings, 0 replies; 10+ messages in thread From: Konrad Rzeszutek Wilk @ 2016-06-07 15:25 UTC (permalink / raw) To: Bob Liu; +Cc: xen-devel, linux-kernel, roger.pau 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 <bob.liu@oracle.com> 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 <bob.liu@oracle.com> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- 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; -- 2.4.11 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <20160607152524.GA10281@localhost.localdomain>]
* Re: [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback [not found] ` <20160607152524.GA10281@localhost.localdomain> @ 2016-06-08 6:46 ` Bob Liu [not found] ` <5757BF4E.9080307@oracle.com> 1 sibling, 0 replies; 10+ messages in thread From: Bob Liu @ 2016-06-08 6:46 UTC (permalink / raw) To: Konrad Rzeszutek Wilk; +Cc: xen-devel, linux-kernel, roger.pau 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! Bob > From e49dc9fc65eda4923b41d903ac51a7ddee182bcd Mon Sep 17 00:00:00 2001 > From: Bob Liu <bob.liu@oracle.com> > 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 <bob.liu@oracle.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > 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; > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <5757BF4E.9080307@oracle.com>]
* Re: [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback [not found] ` <5757BF4E.9080307@oracle.com> @ 2016-06-08 14:47 ` Konrad Rzeszutek Wilk [not found] ` <20160608144739.GD17027@char.us.oracle.com> 1 sibling, 0 replies; 10+ messages in thread From: Konrad Rzeszutek Wilk @ 2016-06-08 14:47 UTC (permalink / raw) To: Bob Liu; +Cc: xen-devel, linux-kernel, roger.pau 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <20160608144739.GD17027@char.us.oracle.com>]
* Re: [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback [not found] ` <20160608144739.GD17027@char.us.oracle.com> @ 2016-06-15 8:39 ` Ross Lagerwall [not found] ` <57611424.2020802@citrix.com> 1 sibling, 0 replies; 10+ messages in thread From: Ross Lagerwall @ 2016-06-15 8:39 UTC (permalink / raw) To: Konrad Rzeszutek Wilk, Bob Liu; +Cc: xen-devel, linux-kernel, roger.pau On 06/08/2016 03:47 PM, Konrad Rzeszutek Wilk wrote: > 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. >>>>> ... snip >>>> 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. > I'd like to request that this patch is backported to Linux 4.5 and both of the patches in this series are backported to Linux 4.6. This is affecting Debian Testing (using Linux 4.6). It fails to recover its disk when resuming or migrating. Thanks, -- Ross Lagerwall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <57611424.2020802@citrix.com>]
* Re: [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback [not found] ` <57611424.2020802@citrix.com> @ 2016-06-15 14:08 ` Konrad Rzeszutek Wilk 0 siblings, 0 replies; 10+ messages in thread From: Konrad Rzeszutek Wilk @ 2016-06-15 14:08 UTC (permalink / raw) To: Ross Lagerwall; +Cc: xen-devel, linux-kernel, roger.pau On Wed, Jun 15, 2016 at 09:39:00AM +0100, Ross Lagerwall wrote: > On 06/08/2016 03:47 PM, Konrad Rzeszutek Wilk wrote: > >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. > >>>>> > ... snip > >>>>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. > > > > I'd like to request that this patch is backported to Linux 4.5 and both of > the patches in this series are backported to Linux 4.6. This is affecting > Debian Testing (using Linux 4.6). It fails to recover its disk when resuming > or migrating. Good idea. Done. > > Thanks, > -- > Ross Lagerwall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback @ 2016-05-31 8:59 Bob Liu 0 siblings, 0 replies; 10+ messages in thread From: Bob Liu @ 2016-05-31 8:59 UTC (permalink / raw) To: linux-kernel; +Cc: xen-devel, Bob Liu, roger.pau Sometimes blkfont may receive twice blkback_changed() notification after migration, then talk_to_blkback() will be called twice too and confused xen-blkback. Signed-off-by: Bob Liu <bob.liu@oracle.com> --- 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-06-15 14:08 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1464685157-30738-1-git-send-email-bob.liu@oracle.com>
2016-05-31 8:59 ` [PATCH 2/2] xen-blkfront: fix resume issues Bob Liu
2016-05-31 20:33 ` [PATCH 1/2] xen-blkfront: don't call talk_to_blkback when already connected to blkback Konrad Rzeszutek Wilk
[not found] ` <20160531203307.GC23808@char.us.oracle.com>
2016-06-01 5:49 ` Bob Liu
[not found] ` <574E7763.9060001@oracle.com>
2016-06-02 14:30 ` Konrad Rzeszutek Wilk
2016-06-07 15:25 ` Konrad Rzeszutek Wilk
[not found] ` <20160607152524.GA10281@localhost.localdomain>
2016-06-08 6:46 ` Bob Liu
[not found] ` <5757BF4E.9080307@oracle.com>
2016-06-08 14:47 ` Konrad Rzeszutek Wilk
[not found] ` <20160608144739.GD17027@char.us.oracle.com>
2016-06-15 8:39 ` Ross Lagerwall
[not found] ` <57611424.2020802@citrix.com>
2016-06-15 14:08 ` Konrad Rzeszutek Wilk
2016-05-31 8:59 Bob Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).