Joe Jin
2011-Jul-13 00:47 UTC
[Xen-devel] [PATCH resubmit] xen-blkfront: Don''t send closing notification to backend in blkfront_closing()
When we do block device attach/detach test with below steps, umount hang and the
guest unable to shutdown:
1. start guest with the latest kernel.
2. attach new block device by xm block-attach in Dom0
3. mount new disk in guest
4. execute xm block-detach to detach the block device in dom0 until timeout
5. try to unmount the disk in guest, umount hung. at here, any IOs to the
device will hang.
Checking the code found when ''xm block-detach'' set backend
device''s state to
''XenbusStateClosing'', frontend received the notification and
blkfront_closing()
be called, at the moment, the disk still using by guest, so frontend refused
to close. In the blkfront_closing(), frontend send a notification to backend
said that the its state switched to ''Closing'', when backend
got the
event, it will disconnect from real device, at here any IO request will
be stuck, even tried to release the disk by umount.
Per our test, below patch fix this issue.
Signed-off-by: Joe Jin <joe.jin@oracle.com>
Signed-off-by: Annie Li <annie.li@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Jens Axboe <jaxboe@fusionio.com>
Cc: stable@kernel.org
---
xen-blkfront.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index b536a9c..f6d8ac2 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1088,7 +1088,7 @@ blkfront_closing(struct blkfront_info *info)
if (bdev->bd_openers) {
xenbus_dev_error(xbdev, -EBUSY,
"Device in use; refusing to close");
- xenbus_switch_state(xbdev, XenbusStateClosing);
+ xbdev->state = XenbusStateClosing;
} else {
xlvbd_release_gendisk(info);
xenbus_frontend_closed(xbdev);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jul-14 08:13 UTC
[Xen-devel] Re: [PATCH resubmit] xen-blkfront: Don''t send closing notification to backend in blkfront_closing()
On Wed, 2011-07-13 at 01:47 +0100, Joe Jin wrote:> When we do block device attach/detach test with below steps, umount hang and the > guest unable to shutdown: > > 1. start guest with the latest kernel. > 2. attach new block device by xm block-attach in Dom0 > 3. mount new disk in guest > 4. execute xm block-detach to detach the block device in dom0 until timeout > 5. try to unmount the disk in guest, umount hung. at here, any IOs to the > device will hang. > > Checking the code found when ''xm block-detach'' set backend device''s state to > ''XenbusStateClosing'', frontend received the notification and blkfront_closing() > be called, at the moment, the disk still using by guest, so frontend refused > to close. In the blkfront_closing(), frontend send a notification to backend > said that the its state switched to ''Closing'', when backend got the > event, it will disconnect from real device, at here any IO request will > be stuck, even tried to release the disk by umount. > > Per our test, below patch fix this issue.It''s worth mentioning here that the change to xbdev->state is picked up in blkif_release() when the device is closed and the disconnect happens at that point instead. I''m wondering if we might not be better off deferring the disconnect on the backend side until the frontend enters XenbusStateClosed instead of doing it in closing. Ian> > Signed-off-by: Joe Jin <joe.jin@oracle.com> > Signed-off-by: Annie Li <annie.li@oracle.com> > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Jens Axboe <jaxboe@fusionio.com> > Cc: stable@kernel.org > > --- > xen-blkfront.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index b536a9c..f6d8ac2 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -1088,7 +1088,7 @@ blkfront_closing(struct blkfront_info *info) > if (bdev->bd_openers) { > xenbus_dev_error(xbdev, -EBUSY, > "Device in use; refusing to close"); > - xenbus_switch_state(xbdev, XenbusStateClosing); > + xbdev->state = XenbusStateClosing; > } else { > xlvbd_release_gendisk(info); > xenbus_frontend_closed(xbdev);_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Joe Jin
2011-Jul-14 08:55 UTC
[Xen-devel] Re: [PATCH resubmit] xen-blkfront: Don''t send closing notification to backend in blkfront_closing()
On 07/14/11 16:13, Ian Campbell wrote:> On Wed, 2011-07-13 at 01:47 +0100, Joe Jin wrote: >> When we do block device attach/detach test with below steps, umount hang and the >> guest unable to shutdown: >> >> 1. start guest with the latest kernel. >> 2. attach new block device by xm block-attach in Dom0 >> 3. mount new disk in guest >> 4. execute xm block-detach to detach the block device in dom0 until timeout >> 5. try to unmount the disk in guest, umount hung. at here, any IOs to the >> device will hang. >> >> Checking the code found when ''xm block-detach'' set backend device''s state to >> ''XenbusStateClosing'', frontend received the notification and blkfront_closing() >> be called, at the moment, the disk still using by guest, so frontend refused >> to close. In the blkfront_closing(), frontend send a notification to backend >> said that the its state switched to ''Closing'', when backend got the >> event, it will disconnect from real device, at here any IO request will >> be stuck, even tried to release the disk by umount. >> >> Per our test, below patch fix this issue. > > It''s worth mentioning here that the change to xbdev->state is picked up > in blkif_release() when the device is closed and the disconnect happens > at that point instead.This is right, thanks for the suggestions.> > I''m wondering if we might not be better off deferring the disconnect on > the backend side until the frontend enters XenbusStateClosed instead of > doing it in closing.Yes this fix from backend side works too, also this looks reasonable than fix in frontend. Konrad, any advice? Thanks, Joe> > Ian > >> >> Signed-off-by: Joe Jin <joe.jin@oracle.com> >> Signed-off-by: Annie Li <annie.li@oracle.com> >> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> Cc: Jens Axboe <jaxboe@fusionio.com> >> Cc: stable@kernel.org >> >> --- >> xen-blkfront.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c >> index b536a9c..f6d8ac2 100644 >> --- a/drivers/block/xen-blkfront.c >> +++ b/drivers/block/xen-blkfront.c >> @@ -1088,7 +1088,7 @@ blkfront_closing(struct blkfront_info *info) >> if (bdev->bd_openers) { >> xenbus_dev_error(xbdev, -EBUSY, >> "Device in use; refusing to close"); >> - xenbus_switch_state(xbdev, XenbusStateClosing); >> + xbdev->state = XenbusStateClosing; >> } else { >> xlvbd_release_gendisk(info); >> xenbus_frontend_closed(xbdev); > >-- Oracle <http://www.oracle.com> Joe Jin | Team Leader, Software Development | +8610.6106.5624 ORACLE | Linux and Virtualization No. 24 Zhongguancun Software Park, Haidian District | 100193 Beijing _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jul-14 09:12 UTC
[Xen-devel] Re: [PATCH resubmit] xen-blkfront: Don''t send closing notification to backend in blkfront_closing()
On Thu, 2011-07-14 at 09:55 +0100, Joe Jin wrote:> On 07/14/11 16:13, Ian Campbell wrote: > > On Wed, 2011-07-13 at 01:47 +0100, Joe Jin wrote:> > > > I''m wondering if we might not be better off deferring the disconnect on > > the backend side until the frontend enters XenbusStateClosed instead of > > doing it in closing. > > Yes this fix from backend side works too, also this looks reasonable than > fix in frontend.I guess there is either a missing "more" or "less" in that sentence ;-) The nice thing about a backend fix is that you don''t need to go round propagating the fix into distros etc and updating existing deployed guests (not to mention non-Linux frontends). I actually thought blkback had something like this already in older "classic" Xen kernels but it seems like the patch never made it out of the XCP patch queue :-(. See http://xenbits.xen.org/hg/XCP/linux-2.6.32.pq.hg/file/tip/CA-7672-blkback-shutdown.patch (which appears to also incorporate a toolstack directed graceful shutdown of some sort as well as deferring the device close). Daniel, do you know of any plans to extricate that stuff from the XCP patch queue for upstream? Are there any other patches (e.g. incremental fixes) in that pq which should go along with it? Ian.> > Konrad, any advice? > > Thanks, > Joe > > > > > Ian > > > >> > >> Signed-off-by: Joe Jin <joe.jin@oracle.com> > >> Signed-off-by: Annie Li <annie.li@oracle.com> > >> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > >> Cc: Jens Axboe <jaxboe@fusionio.com> > >> Cc: stable@kernel.org > >> > >> --- > >> xen-blkfront.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > >> index b536a9c..f6d8ac2 100644 > >> --- a/drivers/block/xen-blkfront.c > >> +++ b/drivers/block/xen-blkfront.c > >> @@ -1088,7 +1088,7 @@ blkfront_closing(struct blkfront_info *info) > >> if (bdev->bd_openers) { > >> xenbus_dev_error(xbdev, -EBUSY, > >> "Device in use; refusing to close"); > >> - xenbus_switch_state(xbdev, XenbusStateClosing); > >> + xbdev->state = XenbusStateClosing; > >> } else { > >> xlvbd_release_gendisk(info); > >> xenbus_frontend_closed(xbdev); > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Joe Jin
2011-Jul-15 07:58 UTC
[Xen-devel] Re: [PATCH resubmit] xen-blkfront: Don''t send closing notification to backend in blkfront_closing()
On 07/14/11 17:12, Ian Campbell wrote:> On Thu, 2011-07-14 at 09:55 +0100, Joe Jin wrote: >> On 07/14/11 16:13, Ian Campbell wrote: >>> On Wed, 2011-07-13 at 01:47 +0100, Joe Jin wrote: > >>> >>> I''m wondering if we might not be better off deferring the disconnect on >>> the backend side until the frontend enters XenbusStateClosed instead of >>> doing it in closing. >> >> Yes this fix from backend side works too, also this looks reasonable than >> fix in frontend. > > I guess there is either a missing "more" or "less" in that sentence ;-) > > The nice thing about a backend fix is that you don''t need to go round > propagating the fix into distros etc and updating existing deployed > guests (not to mention non-Linux frontends).Indeed ;)> > I actually thought blkback had something like this already in older > "classic" Xen kernels but it seems like the patch never made it out of > the XCP patch queue :-(. See > http://xenbits.xen.org/hg/XCP/linux-2.6.32.pq.hg/file/tip/CA-7672-blkback-shutdown.patch > (which appears to also incorporate a toolstack directed graceful > shutdown of some sort as well as deferring the device close). > > Daniel, do you know of any plans to extricate that stuff from the XCP > patch queue for upstream? Are there any other patches (e.g. incremental > fixes) in that pq which should go along with it?Looking forward push this patch to upstream! Joe _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Jul-19 13:21 UTC
Re: [Xen-devel] Re: [PATCH resubmit] xen-blkfront: Don''t send closing notification to backend in blkfront_closing()
On Fri, Jul 15, 2011 at 03:58:50PM +0800, Joe Jin wrote:> On 07/14/11 17:12, Ian Campbell wrote: > > On Thu, 2011-07-14 at 09:55 +0100, Joe Jin wrote: > >> On 07/14/11 16:13, Ian Campbell wrote: > >>> On Wed, 2011-07-13 at 01:47 +0100, Joe Jin wrote: > > > >>> > >>> I''m wondering if we might not be better off deferring the disconnect on > >>> the backend side until the frontend enters XenbusStateClosed instead of > >>> doing it in closing. > >> > >> Yes this fix from backend side works too, also this looks reasonable than > >> fix in frontend. > > > > I guess there is either a missing "more" or "less" in that sentence ;-) > > > > The nice thing about a backend fix is that you don''t need to go round > > propagating the fix into distros etc and updating existing deployed > > guests (not to mention non-Linux frontends). > > Indeed ;) > > > > > I actually thought blkback had something like this already in older > > "classic" Xen kernels but it seems like the patch never made it out of > > the XCP patch queue :-(. See > > http://xenbits.xen.org/hg/XCP/linux-2.6.32.pq.hg/file/tip/CA-7672-blkback-shutdown.patch > > (which appears to also incorporate a toolstack directed graceful > > shutdown of some sort as well as deferring the device close). > > > > Daniel, do you know of any plans to extricate that stuff from the XCP > > patch queue for upstream? Are there any other patches (e.g. incremental > > fixes) in that pq which should go along with it? > > Looking forward push this patch to upstream!Not sure if Daniel is going to get to this. Joe, and chance you could take a look at it and up-port in the 3.0 kernel? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel