WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH 3/3] xen-blkback: refactor vbd remove/disconnect.

To: Joe Jin <joe.jin@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH 3/3] xen-blkback: refactor vbd remove/disconnect.
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Wed, 3 Aug 2011 01:14:07 -0400
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Jens Axboe <jaxboe@xxxxxxxxxxxx>, Greg Marsden <greg.marsden@xxxxxxxxxx>, "linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>, Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>, Annie Li <annie.li@xxxxxxxxxx>, Kurt C Hackel <KURT.HACKEL@xxxxxxxxxx>, Daniel Stodden <daniel.stodden@xxxxxxxxxx>
Delivery-date: Tue, 02 Aug 2011 22:14:57 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4E38AF05.7050506@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4E38ADA0.4030304@xxxxxxxxxx> <4E38AF05.7050506@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxx>
> Cc: Daniel Stodden <daniel.stodden@xxxxxxxxxx>
> Cc: Jens Axboe <jaxboe@xxxxxxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Annie Li <annie.li@xxxxxxxxxx>
> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel