Joe Jin
2011-Aug-03 02:08 UTC
[Xen-devel] [PATCH 0/3] xen-blkback: refactor vbd remove/disconnect.
This patchset is a backport and original patch''s author is Daniel Stodden: http://xenbits.xen.org/hg/XCP/linux-2.6.32.pq.hg/file/tip/CA-7672-blkback-shutdown.patch Initial issue: When we do block device attach/detach test with below steps, umount hang in guest 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. Root cause: This caused by ''xm block-detach'' in Dom0 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. So this may fix either frontend or backend, I have send a fix for frontend: https://lkml.org/lkml/2011/7/8/159 Ian think we should fix it from backend and he pointed out Daniel Stodden have submitted a patch(see above link) for xen-blkback, I tried it and it works well. drivers/block/xen-blkback/blkback.c | 10 +-- drivers/block/xen-blkback/common.h | 5 + drivers/block/xen-blkback/xenbus.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------- 3 files changed, 191 insertions(+), 26 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Joe Jin
2011-Aug-03 02:12 UTC
[Xen-devel] [PATCH 1/3] xen-blkback: add remove_requested to xen_blkif and some declares
Add remove_requested to xen_blkif and some declares. Signed-off-by: Joe Jin <joe.jin@oracle.com> Cc: Daniel Stodden <daniel.stodden@citrix.com> Cc: Jens Axboe <jaxboe@fusionio.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Annie Li <annie.li@oracle.com> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com> --- drivers/block/xen-blkback/common.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h index 9e40b28..453fecf 100644 --- a/drivers/block/xen-blkback/common.h +++ b/drivers/block/xen-blkback/common.h @@ -49,6 +49,7 @@ pr_debug(DRV_PFX "(%s:%d) " fmt ".\n", \ __func__, __LINE__, ##args) +#define WPRINTK(fmt, args...) printk(KERN_WARNING "xen-blkback: " fmt, ##args) /* Not a real protocol. Used to generate ring structs which contain * the elements common to all protocols only. This way we get a @@ -145,6 +146,7 @@ struct xen_blkif { /* Back pointer to the backend_info. */ struct backend_info *be; /* Private fields. */ + int remove_requested; spinlock_t blk_ring_lock; atomic_t refcnt; @@ -198,6 +200,9 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt, struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be); +void xen_vbd_sync(struct xen_vbd *vbd); +void xen_blkback_close(struct xen_blkif *blkif); + static inline void blkif_get_x86_32_req(struct blkif_request *dst, struct blkif_x86_32_request *src) { _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Joe Jin
2011-Aug-03 02:13 UTC
[Xen-devel] [PATCH 2/3] xen-blkback: repleace check kthread_should_stop() to remove_requested in xen_blkif_schedule() loop.
When backend state change to XenbusStateClosed, remove_requested will be set, so repleace check kthread_should_stop() to remove_requested in xen_blkif_schedule() loop. Signed-off-by: Joe Jin <joe.jin@oracle.com> Cc: Daniel Stodden <daniel.stodden@citrix.com> Cc: Jens Axboe <jaxboe@fusionio.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Annie Li <annie.li@oracle.com> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com> -- drivers/block/xen-blkback/blkback.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c index 2330a9a..3d64b52 100644 --- a/drivers/block/xen-blkback/blkback.c +++ b/drivers/block/xen-blkback/blkback.c @@ -274,7 +274,7 @@ int xen_blkif_schedule(void *arg) xen_blkif_get(blkif); - while (!kthread_should_stop()) { + while (!blkif->remove_requested) { if (try_to_freeze()) continue; if (unlikely(vbd->size != vbd_sz(vbd))) @@ -282,11 +282,11 @@ int xen_blkif_schedule(void *arg) wait_event_interruptible( blkif->wq, - blkif->waiting_reqs || kthread_should_stop()); + blkif->waiting_reqs || blkif->remove_requested); wait_event_interruptible( blkbk->pending_free_wq, !list_empty(&blkbk->pending_free) || - kthread_should_stop()); + blkif->remove_requested); blkif->waiting_reqs = 0; smp_mb(); /* clear flag *before* checking for work */ @@ -301,8 +301,8 @@ int xen_blkif_schedule(void *arg) if (log_stats) print_stats(blkif); - blkif->xenblkd = NULL; xen_blkif_put(blkif); + xen_blkback_close(blkif); return 0; } @@ -476,7 +476,7 @@ __do_block_io_op(struct xen_blkif *blkif) if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rc)) break; - if (kthread_should_stop()) { + if (blkif->remove_requested) { more_to_do = 1; break; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Joe Jin
2011-Aug-03 02:14 UTC
[Xen-devel] [PATCH 3/3] xen-blkback: refactor vbd remove/disconnect.
This patch refactor vbd remove/disconnect. 1. Add blkback shutdown watch for the remove/disconnect. 2. Don''t disconnect backend when frontend state is XenbusStateClosing until frontend state changed to XenbusStateClosed. Signed-off-by: Joe Jin <joe.jin@oracle.com> Cc: Daniel Stodden <daniel.stodden@citrix.com> Cc: Jens Axboe <jaxboe@fusionio.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Annie Li <annie.li@oracle.com> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com> --- drivers/block/xen-blkback/xenbus.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 181 insertions(+), 21 deletions(-) diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c index 3f129b4..2bb9727 100644 --- a/drivers/block/xen-blkback/xenbus.c +++ b/drivers/block/xen-blkback/xenbus.c @@ -25,16 +25,25 @@ struct backend_info { struct xenbus_device *dev; struct xen_blkif *blkif; struct xenbus_watch backend_watch; + struct xenbus_watch shutdown_watch; unsigned major; unsigned minor; char *mode; + int group_added; + char *nodename; + atomic_t refcnt; + pid_t kthread_pid; + int shutdown_signalled; }; +DEFINE_SEMAPHORE(blkback_dev_sem); + static struct kmem_cache *xen_blkif_cachep; static void connect(struct backend_info *); static int connect_ring(struct backend_info *); static void backend_changed(struct xenbus_watch *, const char **, unsigned int); +static void xen_vbd_free(struct xen_vbd *vbd); struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be) { @@ -99,6 +108,16 @@ static void xen_update_blkif_status(struct xen_blkif *blkif) blkif->xenblkd = NULL; xenbus_dev_error(blkif->be->dev, err, "start xenblkd"); } + + blkif->be->kthread_pid = blkif->xenblkd->pid; + atomic_inc(&blkif->be->refcnt); + + err = xenbus_printf(XBT_NIL, blkif->be->dev->nodename, "kthread-pid", + "%d", blkif->xenblkd->pid); + if (err) { + xenbus_dev_error(blkif->be->dev, err, "write kthread-pid"); + return; + } } static struct xen_blkif *xen_blkif_alloc(domid_t domid) @@ -213,11 +232,6 @@ static int xen_blkif_map(struct xen_blkif *blkif, unsigned long shared_page, static void xen_blkif_disconnect(struct xen_blkif *blkif) { - if (blkif->xenblkd) { - kthread_stop(blkif->xenblkd); - blkif->xenblkd = NULL; - } - atomic_dec(&blkif->refcnt); wait_event(blkif->waiting_to_free, atomic_read(&blkif->refcnt) == 0); atomic_inc(&blkif->refcnt); @@ -296,6 +310,7 @@ VBD_SHOW(mode, "%s\n", be->mode); int xenvbd_sysfs_addif(struct xenbus_device *dev) { int error; + struct backend_info *be = dev_get_drvdata(&dev->dev); error = device_create_file(&dev->dev, &dev_attr_physical_device); if (error) @@ -309,6 +324,8 @@ int xenvbd_sysfs_addif(struct xenbus_device *dev) if (error) goto fail3; + be->group_added = 1; + return 0; fail3: sysfs_remove_group(&dev->dev.kobj, &xen_vbdstat_group); @@ -319,11 +336,73 @@ fail1: device_remove_file(&dev->dev, &dev_attr_physical_device); void xenvbd_sysfs_delif(struct xenbus_device *dev) { + struct backend_info *be = dev_get_drvdata(&dev->dev); + if (be->group_added == 0) + return; sysfs_remove_group(&dev->dev.kobj, &xen_vbdstat_group); device_remove_file(&dev->dev, &dev_attr_mode); device_remove_file(&dev->dev, &dev_attr_physical_device); + be->group_added = 0; +} + +static int xenvbd_kthread_remove(struct backend_info *be) +{ + struct xen_blkif *blkif = be->blkif; + + if (!blkif || !blkif->xenblkd) + return 0; + + blkif->remove_requested = 1; + wake_up_process(blkif->xenblkd); + + return -EBUSY; +} + +static void xenvbd_signal_shutdown(struct backend_info *be) +{ + int err; + + down(&blkback_dev_sem); + + if (be->shutdown_signalled) + goto out; + + err = xenbus_write(XBT_NIL, be->nodename, "shutdown-done", ""); + if (err) + WPRINTK("Error writing shutdown-done for %s: %d\n", + be->nodename, err); + + if (be->dev) + xenbus_switch_state(be->dev, XenbusStateClosed); + + be->shutdown_signalled = 1; + + out: + up(&blkback_dev_sem); } +static void backend_release(struct backend_info *be) +{ + struct xen_blkif *blkif = be->blkif; + + if (current->pid == be->kthread_pid) + xenvbd_signal_shutdown(be); + + if (!atomic_dec_and_test(&be->refcnt)) + return; + + xenvbd_signal_shutdown(be); + + if (blkif) { + xen_blkif_disconnect(blkif); + xen_vbd_free(&blkif->vbd); + xen_blkif_free(blkif); + be->blkif = NULL; + } + + kfree(be->nodename); + kfree(be); + } static void xen_vbd_free(struct xen_vbd *vbd) { @@ -332,6 +411,12 @@ static void xen_vbd_free(struct xen_vbd *vbd) vbd->bdev = NULL; } +void xen_vbd_sync(struct xen_vbd *vbd) +{ + if (vbd->bdev) + fsync_bdev(vbd->bdev); +} + static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle, unsigned major, unsigned minor, int readonly, int cdrom) @@ -384,8 +469,9 @@ static int xen_blkbk_remove(struct xenbus_device *dev) DPRINTK(""); - if (be->major || be->minor) - xenvbd_sysfs_delif(dev); + down(&blkback_dev_sem); + be->dev = NULL; + up(&blkback_dev_sem); if (be->backend_watch.node) { unregister_xenbus_watch(&be->backend_watch); @@ -393,18 +479,73 @@ static int xen_blkbk_remove(struct xenbus_device *dev) be->backend_watch.node = NULL; } - if (be->blkif) { - xen_blkif_disconnect(be->blkif); - xen_vbd_free(&be->blkif->vbd); - xen_blkif_free(be->blkif); - be->blkif = NULL; + if (be->shutdown_watch.node) { + unregister_xenbus_watch(&be->shutdown_watch); + kfree(be->shutdown_watch.node); + be->shutdown_watch.node = NULL; } - kfree(be); + if (xenvbd_kthread_remove(be)) + WPRINTK("BAD REMOVE REQUEST for %s\n", be->nodename); + + xenvbd_sysfs_delif(dev); + backend_release(be); + dev_set_drvdata(&dev->dev, NULL); + return 0; } +/* + * called by kthread when closing + */ +void xen_blkback_close(struct xen_blkif *blkif) +{ + xen_blkif_disconnect(blkif); + xen_vbd_sync(&blkif->vbd); + blkif->remove_requested = 0; + + down(&blkback_dev_sem); + if (blkif->be->dev) + xenvbd_sysfs_delif(blkif->be->dev); + up(&blkback_dev_sem); + + backend_release(blkif->be); + blkif->xenblkd = NULL; +} + +static void xenvbd_start_shutdown(struct xenbus_watch *watch, + const char **vec, unsigned int length) +{ + int err; + char *type; + unsigned int len; + struct backend_info *be + = container_of(watch, struct backend_info, shutdown_watch); + struct xenbus_device *dev = be->dev; + + if (be->shutdown_signalled) + return; + + type = xenbus_read(XBT_NIL, dev->nodename, "shutdown-request", &len); + err = (IS_ERR(type) ? PTR_ERR(type) : 0); + + if (XENBUS_EXIST_ERR(err)) + return; + + if (err) { + xenbus_dev_fatal(dev, err, "reading shutdown-request"); + return; + } + + xenbus_switch_state(dev, XenbusStateClosing); + + if (len == sizeof("force") - 1 && !memcmp(type, "force", len)) + if (!xenvbd_kthread_remove(be)) + xenvbd_signal_shutdown(be); /* shutdown immediately */ + + kfree(type); + } int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt, struct backend_info *be, int state) { @@ -437,6 +578,15 @@ static int xen_blkbk_probe(struct xenbus_device *dev, } be->dev = dev; dev_set_drvdata(&dev->dev, be); + atomic_set(&be->refcnt, 1); + + be->nodename = kasprintf(GFP_KERNEL, "%s", dev->nodename); + if (!be->nodename) { + xenbus_dev_fatal(dev, -ENOMEM, + "allocating backend structure"); + kfree(be); + return -ENOMEM; + } be->blkif = xen_blkif_alloc(dev->otherend_id); if (IS_ERR(be->blkif)) { @@ -454,6 +604,11 @@ static int xen_blkbk_probe(struct xenbus_device *dev, if (err) goto fail; + err = xenbus_watch_pathfmt(dev, &be->shutdown_watch, xenvbd_start_shutdown, + "%s/%s", dev->nodename, "shutdown-request"); + if (err) + goto fail; + err = xenbus_switch_state(dev, XenbusStateInitWait); if (err) goto fail; @@ -567,13 +722,17 @@ static void frontend_changed(struct xenbus_device *dev, struct backend_info *be = dev_get_drvdata(&dev->dev); int err; - DPRINTK("%s", xenbus_strstate(frontend_state)); + DPRINTK("%s: %s", dev->nodename, xenbus_strstate(frontend_state)); switch (frontend_state) { case XenbusStateInitialising: if (dev->state == XenbusStateClosed) { pr_info(DRV_PFX "%s: prepare for reconnect\n", dev->nodename); + + xenbus_rm(XBT_NIL, dev->nodename, "shutdown-done"); + be->shutdown_signalled = 0; + xenbus_switch_state(dev, XenbusStateInitWait); } break; @@ -590,7 +749,7 @@ static void frontend_changed(struct xenbus_device *dev, /* * Enforce precondition before potential leak point. - * blkif_disconnect() is idempotent. + * xen_blkif_disconnect() is idempotent. */ xen_blkif_disconnect(be->blkif); @@ -601,17 +760,16 @@ static void frontend_changed(struct xenbus_device *dev, break; case XenbusStateClosing: - xen_blkif_disconnect(be->blkif); xenbus_switch_state(dev, XenbusStateClosing); break; case XenbusStateClosed: - xenbus_switch_state(dev, XenbusStateClosed); - if (xenbus_dev_is_online(dev)) - break; - /* fall through if not online */ + if (!xenvbd_kthread_remove(be)) + xenvbd_signal_shutdown(be); + break; + case XenbusStateUnknown: - /* implies blkif_disconnect() via blkback_remove() */ + /* implies xen_blkif_disconnect() via blkback_remove() */ device_unregister(&dev->dev); break; @@ -620,6 +778,8 @@ static void frontend_changed(struct xenbus_device *dev, frontend_state); break; } + + DPRINTK("%s: %s", dev->nodename, xenbus_strstate(dev->state)); } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Aug-03 05:06 UTC
[Xen-devel] Re: [PATCH 1/3] xen-blkback: add remove_requested to xen_blkif and some declares
On Wed, Aug 03, 2011 at 10:12:01AM +0800, Joe Jin wrote:> Add remove_requested to xen_blkif and some declares. > > Signed-off-by: Joe Jin <joe.jin@oracle.com> > Cc: Daniel Stodden <daniel.stodden@citrix.com> > Cc: Jens Axboe <jaxboe@fusionio.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Annie Li <annie.li@oracle.com> > Cc: Ian Campbell <Ian.Campbell@eu.citrix.com> > --- > drivers/block/xen-blkback/common.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h > index 9e40b28..453fecf 100644 > --- a/drivers/block/xen-blkback/common.h > +++ b/drivers/block/xen-blkback/common.h > @@ -49,6 +49,7 @@ > pr_debug(DRV_PFX "(%s:%d) " fmt ".\n", \ > __func__, __LINE__, ##args) > > +#define WPRINTK(fmt, args...) printk(KERN_WARNING "xen-blkback: " fmt, ##args) > > /* Not a real protocol. Used to generate ring structs which contain > * the elements common to all protocols only. This way we get a > @@ -145,6 +146,7 @@ struct xen_blkif { > /* Back pointer to the backend_info. */ > struct backend_info *be; > /* Private fields. */ > + int remove_requested;bool?> spinlock_t blk_ring_lock; > atomic_t refcnt; > > @@ -198,6 +200,9 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt, > > struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be); > > +void xen_vbd_sync(struct xen_vbd *vbd); > +void xen_blkback_close(struct xen_blkif *blkif); > + > static inline void blkif_get_x86_32_req(struct blkif_request *dst, > struct blkif_x86_32_request *src) > { >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Aug-03 05:14 UTC
[Xen-devel] Re: [PATCH 3/3] xen-blkback: refactor vbd remove/disconnect.
On Wed, Aug 03, 2011 at 10:14:29AM +0800, Joe Jin wrote:> This patch refactor vbd remove/disconnect. > 1. Add blkback shutdown watch for the remove/disconnect. > 2. Don''t disconnect backend when frontend state is XenbusStateClosing > until frontend state changed to XenbusStateClosed.Please do run this through checkpath.pl and fix the issues I''ve mentioned below.> > Signed-off-by: Joe Jin <joe.jin@oracle.com> > Cc: Daniel Stodden <daniel.stodden@citrix.com> > Cc: Jens Axboe <jaxboe@fusionio.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Annie Li <annie.li@oracle.com> > Cc: Ian Campbell <Ian.Campbell@eu.citrix.com> > --- > drivers/block/xen-blkback/xenbus.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 181 insertions(+), 21 deletions(-) > > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c > index 3f129b4..2bb9727 100644 > --- a/drivers/block/xen-blkback/xenbus.c > +++ b/drivers/block/xen-blkback/xenbus.c > @@ -25,16 +25,25 @@ struct backend_info { > struct xenbus_device *dev; > struct xen_blkif *blkif; > struct xenbus_watch backend_watch; > + struct xenbus_watch shutdown_watch; > unsigned major; > unsigned minor; > char *mode; > + int group_added;Grrr.. please do make this bool> + char *nodename; > + atomic_t refcnt; > + pid_t kthread_pid; > + int shutdown_signalled;And also this one> }; > > +DEFINE_SEMAPHORE(blkback_dev_sem); > + > static struct kmem_cache *xen_blkif_cachep; > static void connect(struct backend_info *); > static int connect_ring(struct backend_info *); > static void backend_changed(struct xenbus_watch *, const char **, > unsigned int); > +static void xen_vbd_free(struct xen_vbd *vbd); > > struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be) > { > @@ -99,6 +108,16 @@ static void xen_update_blkif_status(struct xen_blkif *blkif) > blkif->xenblkd = NULL; > xenbus_dev_error(blkif->be->dev, err, "start xenblkd"); > } > +^ You relly need to remove the space there. I believe if you run this through scripts/checkpath.pl it will complain.> + blkif->be->kthread_pid = blkif->xenblkd->pid; > + atomic_inc(&blkif->be->refcnt); > + > + err = xenbus_printf(XBT_NIL, blkif->be->dev->nodename, "kthread-pid", > + "%d", blkif->xenblkd->pid);Hm, not sure if that is my editor - but it looks like a big space. If the checkpatch.pl does not complain about it, then it has to be my editor.> + if (err) { > + xenbus_dev_error(blkif->be->dev, err, "write kthread-pid"); > + return; > + } > } > > static struct xen_blkif *xen_blkif_alloc(domid_t domid) > @@ -213,11 +232,6 @@ static int xen_blkif_map(struct xen_blkif *blkif, unsigned long shared_page, > > static void xen_blkif_disconnect(struct xen_blkif *blkif) > { > - if (blkif->xenblkd) { > - kthread_stop(blkif->xenblkd); > - blkif->xenblkd = NULL; > - } > - > atomic_dec(&blkif->refcnt); > wait_event(blkif->waiting_to_free, atomic_read(&blkif->refcnt) == 0); > atomic_inc(&blkif->refcnt); > @@ -296,6 +310,7 @@ VBD_SHOW(mode, "%s\n", be->mode); > int xenvbd_sysfs_addif(struct xenbus_device *dev) > { > int error; > + struct backend_info *be = dev_get_drvdata(&dev->dev); > > error = device_create_file(&dev->dev, &dev_attr_physical_device); > if (error) > @@ -309,6 +324,8 @@ int xenvbd_sysfs_addif(struct xenbus_device *dev) > if (error) > goto fail3; > > + be->group_added = 1; > + > return 0; > > fail3: sysfs_remove_group(&dev->dev.kobj, &xen_vbdstat_group); > @@ -319,11 +336,73 @@ fail1: device_remove_file(&dev->dev, &dev_attr_physical_device); > > void xenvbd_sysfs_delif(struct xenbus_device *dev) > { > + struct backend_info *be = dev_get_drvdata(&dev->dev); > + if (be->group_added == 0) > + return; > sysfs_remove_group(&dev->dev.kobj, &xen_vbdstat_group); > device_remove_file(&dev->dev, &dev_attr_mode); > device_remove_file(&dev->dev, &dev_attr_physical_device); > + be->group_added = 0; > +} > + > +static int xenvbd_kthread_remove(struct backend_info *be) > +{ > + struct xen_blkif *blkif = be->blkif; > + > + if (!blkif || !blkif->xenblkd) > + return 0; > + > + blkif->remove_requested = 1; > + wake_up_process(blkif->xenblkd); > + > + return -EBUSY; > +} > + > +static void xenvbd_signal_shutdown(struct backend_info *be) > +{ > + int err; > + > + down(&blkback_dev_sem); > + > + if (be->shutdown_signalled) > + goto out; > + > + err = xenbus_write(XBT_NIL, be->nodename, "shutdown-done", ""); > + if (err) > + WPRINTK("Error writing shutdown-done for %s: %d\n", > + be->nodename, err); ^remove that space.> + > + if (be->dev) > + xenbus_switch_state(be->dev, XenbusStateClosed); > + > + be->shutdown_signalled = 1; > + > + out: > + up(&blkback_dev_sem); > } > > +static void backend_release(struct backend_info *be) > +{ > + struct xen_blkif *blkif = be->blkif; > + > + if (current->pid == be->kthread_pid) > + xenvbd_signal_shutdown(be); > + > + if (!atomic_dec_and_test(&be->refcnt)) > + return; > + > + xenvbd_signal_shutdown(be); > + > + if (blkif) { > + xen_blkif_disconnect(blkif); > + xen_vbd_free(&blkif->vbd); > + xen_blkif_free(blkif); > + be->blkif = NULL; > + } > + > + kfree(be->nodename); > + kfree(be); > + } > > static void xen_vbd_free(struct xen_vbd *vbd) > { > @@ -332,6 +411,12 @@ static void xen_vbd_free(struct xen_vbd *vbd) > vbd->bdev = NULL; > } > > +void xen_vbd_sync(struct xen_vbd *vbd) > +{ > + if (vbd->bdev) > + fsync_bdev(vbd->bdev); > +} > + > static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle, > unsigned major, unsigned minor, int readonly, > int cdrom) > @@ -384,8 +469,9 @@ static int xen_blkbk_remove(struct xenbus_device *dev) > > DPRINTK(""); > > - if (be->major || be->minor) > - xenvbd_sysfs_delif(dev); > + down(&blkback_dev_sem); > + be->dev = NULL; > + up(&blkback_dev_sem); > > if (be->backend_watch.node) { > unregister_xenbus_watch(&be->backend_watch); > @@ -393,18 +479,73 @@ static int xen_blkbk_remove(struct xenbus_device *dev) > be->backend_watch.node = NULL; > } > > - if (be->blkif) { > - xen_blkif_disconnect(be->blkif); > - xen_vbd_free(&be->blkif->vbd); > - xen_blkif_free(be->blkif); > - be->blkif = NULL; > + if (be->shutdown_watch.node) { > + unregister_xenbus_watch(&be->shutdown_watch); > + kfree(be->shutdown_watch.node); > + be->shutdown_watch.node = NULL; > } > > - kfree(be); > + if (xenvbd_kthread_remove(be)) > + WPRINTK("BAD REMOVE REQUEST for %s\n", be->nodename); > + > + xenvbd_sysfs_delif(dev); > + backend_release(be); > + > dev_set_drvdata(&dev->dev, NULL); > + > return 0; > } > > +/* > + * called by kthread when closing > + */ > +void xen_blkback_close(struct xen_blkif *blkif) > +{ > + xen_blkif_disconnect(blkif); > + xen_vbd_sync(&blkif->vbd); > + blkif->remove_requested = 0; > + > + down(&blkback_dev_sem); > + if (blkif->be->dev) > + xenvbd_sysfs_delif(blkif->be->dev); > + up(&blkback_dev_sem); > + > + backend_release(blkif->be); > + blkif->xenblkd = NULL; > +} > + > +static void xenvbd_start_shutdown(struct xenbus_watch *watch, > + const char **vec, unsigned int length) > +{ > + int err; > + char *type; > + unsigned int len; > + struct backend_info *be > + = container_of(watch, struct backend_info, shutdown_watch); > + struct xenbus_device *dev = be->dev; > + > + if (be->shutdown_signalled) > + return; > + > + type = xenbus_read(XBT_NIL, dev->nodename, "shutdown-request", &len); > + err = (IS_ERR(type) ? PTR_ERR(type) : 0); > + > + if (XENBUS_EXIST_ERR(err)) > + return; > + > + if (err) { > + xenbus_dev_fatal(dev, err, "reading shutdown-request"); > + return; > + } > + > + xenbus_switch_state(dev, XenbusStateClosing); > + > + if (len == sizeof("force") - 1 && !memcmp(type, "force", len)) > + if (!xenvbd_kthread_remove(be)) > + xenvbd_signal_shutdown(be); /* shutdown immediately */ > + > + kfree(type); > + } > int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt, > struct backend_info *be, int state) > { > @@ -437,6 +578,15 @@ static int xen_blkbk_probe(struct xenbus_device *dev, > } > be->dev = dev; > dev_set_drvdata(&dev->dev, be); > + atomic_set(&be->refcnt, 1); > + > + be->nodename = kasprintf(GFP_KERNEL, "%s", dev->nodename); > + if (!be->nodename) { > + xenbus_dev_fatal(dev, -ENOMEM, > + "allocating backend structure"); > + kfree(be); > + return -ENOMEM; > + } > > be->blkif = xen_blkif_alloc(dev->otherend_id); > if (IS_ERR(be->blkif)) { > @@ -454,6 +604,11 @@ static int xen_blkbk_probe(struct xenbus_device *dev, > if (err) > goto fail; > > + err = xenbus_watch_pathfmt(dev, &be->shutdown_watch, xenvbd_start_shutdown, > + "%s/%s", dev->nodename, "shutdown-request"); > + if (err) > + goto fail; > + > err = xenbus_switch_state(dev, XenbusStateInitWait); > if (err) > goto fail; > @@ -567,13 +722,17 @@ static void frontend_changed(struct xenbus_device *dev, > struct backend_info *be = dev_get_drvdata(&dev->dev); > int err; > > - DPRINTK("%s", xenbus_strstate(frontend_state)); > + DPRINTK("%s: %s", dev->nodename, xenbus_strstate(frontend_state)); > > switch (frontend_state) { > case XenbusStateInitialising: > if (dev->state == XenbusStateClosed) { > pr_info(DRV_PFX "%s: prepare for reconnect\n", > dev->nodename); > + > + xenbus_rm(XBT_NIL, dev->nodename, "shutdown-done"); > + be->shutdown_signalled = 0; > + > xenbus_switch_state(dev, XenbusStateInitWait); > } > break; > @@ -590,7 +749,7 @@ static void frontend_changed(struct xenbus_device *dev, > > /* > * Enforce precondition before potential leak point. > - * blkif_disconnect() is idempotent. > + * xen_blkif_disconnect() is idempotent. > */ > xen_blkif_disconnect(be->blkif); > > @@ -601,17 +760,16 @@ static void frontend_changed(struct xenbus_device *dev, > break; > > case XenbusStateClosing: > - xen_blkif_disconnect(be->blkif); > xenbus_switch_state(dev, XenbusStateClosing); > break; > > case XenbusStateClosed: > - xenbus_switch_state(dev, XenbusStateClosed); > - if (xenbus_dev_is_online(dev)) > - break; > - /* fall through if not online */ > + if (!xenvbd_kthread_remove(be)) > + xenvbd_signal_shutdown(be); > + break; > + > case XenbusStateUnknown: > - /* implies blkif_disconnect() via blkback_remove() */ > + /* implies xen_blkif_disconnect() via blkback_remove() */ > device_unregister(&dev->dev); > break; > > @@ -620,6 +778,8 @@ static void frontend_changed(struct xenbus_device *dev, > frontend_state); > break; > } > + > + DPRINTK("%s: %s", dev->nodename, xenbus_strstate(dev->state)); > } > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Aug-03 14:53 UTC
Re: [Xen-devel] [PATCH 0/3] xen-blkback: refactor vbd remove/disconnect.
>>> Joe Jin 08/03/11 4:10 AM >>> >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.A bogus sequence of operations - an operator in Dom0 shouldn''t remove a device that is still in use in a guest, except as an exceptionasl measure (and then other bad behavior is to be expected). What''s the use case? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel Stodden
2011-Aug-03 20:38 UTC
Re: [Xen-devel] [PATCH 0/3] xen-blkback: refactor vbd remove/disconnect.
On Wed, 2011-08-03 at 10:53 -0400, Jan Beulich wrote:> >>> Joe Jin 08/03/11 4:10 AM >>> > >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 timeoutDoesn''t this fail immediately in userland right now? The disconnect attempt (blkback switching to Closing) will be acknowledged with an error, so you''d watch backend and frontend state simultaneously.>From there on, there might be options. Could switch back to Connectedand call it a day, but that''s currently nowhere exercised, so not sure if it''s fully stable. But the behavior established in XCP is to bail out, but leave the backend at Closing. That means from the backend perspective, the VBD will unplug and get cleaned up at an unspecific later point in time. But stay operational. Note that the latter takes udev work, to finally clean up attached disk images after completion.> >5. try to unmount the disk in guest, umount hung. at here, any IOs to the > >device will hang.Yeah, not so good. That''s what --force would imply.> A bogus sequence of operations - an operator in Dom0 shouldn''t remove > a device that is still in use in a guest, except as an exceptionasl measure > (and then other bad behavior is to be expected). What''s the use case?The administrative perspective is right, but it''s a valid one. And even if you''re coordinating dom0 unplug with guest umount -- there needs a way to handly buggy coordination. We don''t have guest usage indication in the frontend record, so backends resort to try/error instead. I think try/error is the way to deal with it. Transaction stuff necessary to synch a front/back-coordinated disconnect seems over the top, and we''d probably want compat behavior anyway. Not sure what xm/xl can or wants to do about delayed detaches. If it sounds undesirable, we probably want to consider patch to blkback to abort shutdown-request and flip back to Connected. So a controller would try in sync, fail in sync, and rolls back. So it won''t have to deal with backend detach later. Sounds simpler. I don''t think there''s really a particular reason XCP chose to behave the way it does, except low hanging fruit. Not convinced it doesn''t need transactions either, because you could see the tool/blkback interaction race blkback/blkfront driven by guest umount. But it may yield a simpler UI. Please correct me where my lack of OSS toolstack understanding struck. :) Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel