thr3ads.net - Xen devel - [Xen-devel] [PATCH -v3 0/3] xen-blkback: refactor vbd remove/disconnect. [Aug 2011]

If this information is useful, please help other people find it:
Share via:

Joe Jin

2011-Aug-04 07:21 UTC

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

This patchset is a backport and original patch 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.

Changes:
  v3:
    - Unregister the device when backend state switch to XenbusStateClosed.

  v2:
    - Reformat code style.
    - Per Knoard suggestions, change some int defines to bool.

 drivers/block/xen-blkback/blkback.c |   10 +--
 drivers/block/xen-blkback/common.h  |    5 +
 drivers/block/xen-blkback/xenbus.c  |  206
+++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 195 insertions(+), 26 deletions(-)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Joe Jin

2011-Aug-04 07:23 UTC

head link

[Xen-devel] [PATCH -v3 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..acda757 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. */
+	bool			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-04 07:24 UTC

head link

[Xen-devel] [PATCH -v3 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-04 07:25 UTC

head link

[Xen-devel] [PATCH -v3 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 |  206
++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 185 insertions(+), 21 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c
b/drivers/block/xen-blkback/xenbus.c
index 3f129b4..1990d1c 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;
+	bool			group_added;
+	char			*nodename;
+	atomic_t		refcnt;
+	pid_t			kthread_pid;
+	bool			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 = true;
+
 	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)
+		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 = false;
+}
+
+static int xenvbd_kthread_remove(struct backend_info *be)
+{
+	struct xen_blkif *blkif = be->blkif;
+
+	if (!blkif || !blkif->xenblkd)
+		return 0;
+
+	blkif->remove_requested = true;
+	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 = true;
+
+ 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,76 @@ 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)
+{
+	struct xenbus_device *dev = blkif->be->dev;
+
+	xen_blkif_disconnect(blkif);
+	xen_vbd_sync(&blkif->vbd);
+	blkif->remove_requested = false;
+
+	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;
+	device_unregister(&dev->dev);
+}
+
+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 +581,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 +607,12 @@ 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 +726,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 = false;
+
 			xenbus_switch_state(dev, XenbusStateInitWait);
 		}
 		break;
@@ -590,7 +753,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 +764,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 +782,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-04 19:48 UTC

head link

[Xen-devel] Re: [PATCH -v3 2/3] xen-blkback: repleace check kthread_should_stop() to remove_requested in xen_blkif_schedule() loop.

On Thu, Aug 04, 2011 at 03:24:44PM +0800, Joe Jin wrote:> 
> 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);
<sigh> I can''t believe I didn''t catch this earlier, but
this a
big NO NO. Each patch _MUST_ be compile on its own. If I commit
patch 1 and this patch, the compile (actually linker) process
stops b/c xen_blkback_close is not defined:

drivers/built-in.o: In function `xen_blkif_schedule'':
/home/konrad/ssd/linux/drivers/block/xen-blkback/blkback.c:305: undefined
reference to `xen_blkback_close''
make[2]: *** [.tmp_vmlinux1] Error 1

You can move the xen_blkback_close() to the third patch
instead of having it here.

I will be nice and do it for you.
>  
>  	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

David Vrabel

2011-Aug-05 09:42 UTC

head link

Re: [Xen-devel] Re: [PATCH -v3 2/3] xen-blkback: repleace check kthread_should_stop() to remove_requested in xen_blkif_schedule() loop.

On 04/08/2011 20:48, Konrad Rzeszutek Wilk wrote:> <sigh>  I can''t believe I didn''t catch this
earlier, but this a
> big NO NO. Each patch _MUST_ be compile on its own. If I commit
> patch 1 and this patch, the compile (actually linker) process
> stops b/c xen_blkback_close is not defined:
All three patches should be merged together as patch 2 on it''s own will
break the driver as the thread will never exit as 
blkif->remove_requested isn''t set until patch 3.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Konrad Rzeszutek Wilk

2011-Aug-06 14:39 UTC

head link

[Xen-devel] Re: [PATCH -v3 1/3] xen-blkback: add remove_requested to xen_blkif and some declares

On Thu, Aug 04, 2011 at 03:23:41PM +0800, Joe Jin wrote:> 
> Add remove_requested to xen_blkif and some declares.
By itself this patch is a bit strange. If you look in
Documentation/SubmittingPatches:
"2) Describe your changes.

Describe the technical detail of the change(s) your patch includes.
"

There is no really technical details. As in, why is this patch neccessary.

That document also states:
"3) Separate your changes.

Separate _logical changes_ into a single patch file.

For example, if your changes include both bug fixes and performance
enhancements for a single driver, separate those changes into two
or more patches.  If your changes include an API update, and a new
driver which uses that new API, separate those into two patches."


This patch by itself has no logical purpose - as in it does not fix a bug,
or add a new feature - it just plops a new element in a structure, provides
two function decleration of non-existing functions and a maccro that is not
used.

Soo.  Looking at the three patches I believe some reshuffling ought to be done.
If you recall my comments:


        >  	case XenbusStateUnknown:
        > -		/* implies blkif_disconnect() via blkback_remove() */
        > +		/* implies xen_blkif_disconnect() via blkback_remove() */

        Ok, that is not part of this patch. You should create another commit
which
        does this type of cleanup and
        >  		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));

        .. also for this.
        >  }
        >  

I am not sure if it is not clear, but what I meant that those two changes
(the comment rename and adding the DPRINKT) should be as a seperate
patch. So take those changes from patch #3 out and make a new patch.

Read on..> 
> 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..acda757 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)
This is what I said in my review

        "What is ''WPRITNK'' ? Can you use the the same
type of printks
        as the rest? Also you have a space at the end. Make sure to
        run checkpath.pl"

which honeselty I should have been more specific. I meant that
you could just convert all the "WPRINTK" to what they define.

As in, sed/WPRINT/printk(KERN_WARNING/..

In essence, what I would like you to do is:

1). Read up on Documentation/SubmittingPatches
2). Squash this patch (except the declerations of the functions that are
    implemented in patch #3) into patch #2. The declerations of functions
    should be squashed in patch #3.
3). Replace in patch #3 all calls to WPRINTK with printk(KERN_WARNING.
4). Create a new patch that deals with the addition of DPRINTK
    and the update of the comment (see above).
5). The total should be three patches:
    a). This patch squashed with patch #2 (with the modification described in
2).
    b). Patch #3 modified
    c). A new patch with the debug/comments modifications.
6). Make sure each patch compiles on its own.
7). Resend - or if you want to double check with me in case I''ve
further
    comments - just send it to me privately.
>  
>  /* 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. */
> +	bool			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

Xen devel - Aug 2011 - [PATCH -v3 0/3] xen-blkback: refactor vbd remove/disconnect.

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

[Xen-devel] [PATCH -v3 1/3] xen-blkback: add remove_requested to xen_blkif and some declares

[Xen-devel] [PATCH -v3 2/3] xen-blkback: repleace check kthread_should_stop() to remove_requested in xen_blkif_schedule() loop.

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

[Xen-devel] Re: [PATCH -v3 2/3] xen-blkback: repleace check kthread_should_stop() to remove_requested in xen_blkif_schedule() loop.

Re: [Xen-devel] Re: [PATCH -v3 2/3] xen-blkback: repleace check kthread_should_stop() to remove_requested in xen_blkif_schedule() loop.

[Xen-devel] Re: [PATCH -v3 1/3] xen-blkback: add remove_requested to xen_blkif and some declares