[RFC] [PATCH] add locking in statistics sysfs show functions

[RFC] [PATCH] add locking in statistics sysfs show functions

[Matt Wilson]

The blkback code in the 2.6.18 tree has a patch to avoid a race condition
in the statistics sysfs routines. Is it no longer needed? The following
patch ports the change to 3.x.

Matt Wilson (1):
  xen/blkback: add locking in statistics sysfs show functions

 drivers/block/xen-blkback/xenbus.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

-- 
1.7.2.5

[RFC] [PATCH] xen/blkback: add locking in statistics sysfs show functions

[Matt Wilson]

This is a port of a patch in the XenoLinux 2.6.18 tree:
http://xenbits.xen.org/hg/linux-2.6.18-xen.hg/rev/f47c07325a56

Fix blkback sysfs race

Read blkback statistics info after remove vbd device(s) kernel
will crash.

Signed-off-by: Joe Jin <joe.jin@oracle.com>
Acked-by: Jan Beulich <jbeulich@novell.com>
[Ported to Linux 3.x]
Signed-off-by: Matt Wilson <msw@amazon.com>
---
 drivers/block/xen-blkback/xenbus.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index da19506..74d4810 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -21,6 +21,8 @@
 #include <xen/grant_table.h>
 #include "common.h"
 
+static DEFINE_RWLOCK(sysfs_read_lock);
+
 struct backend_info {
 	struct xenbus_device	*dev;
 	struct xen_blkif	*blkif;
@@ -225,10 +227,20 @@ int __init xen_blkif_interface_init(void)
 				   struct device_attribute *attr,	\
 				   char *buf)				\
 	{								\
-		struct xenbus_device *dev = to_xenbus_device(_dev);	\
-		struct backend_info *be = dev_get_drvdata(&dev->dev);	\
+		ssize_t ret = -ENODEV;					\
+		struct xenbus_device *dev;				\
+		struct backend_info *be;				\
 									\
-		return sprintf(buf, format, ##args);			\
+		if (!get_device(_dev))					\
+			return ret;					\
+		dev = to_xenbus_device(_dev);				\
+		read_lock(&sysfs_read_lock);				\
+		be = (struct backend_info *) dev_get_drvdata(&dev->dev);\
+		if (be != NULL)						\
+			ret = sprintf(buf, format, ##args);		\
+		read_unlock(&sysfs_read_lock);				\
+		put_device(_dev);					\
+		return ret;						\
 	}								\
 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
 
@@ -353,6 +365,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
 
 	DPRINTK("");
 
+	write_lock(&sysfs_read_lock);
 	if (be->major || be->minor)
 		xenvbd_sysfs_delif(dev);
 
@@ -371,6 +384,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
 
 	kfree(be);
 	dev_set_drvdata(&dev->dev, NULL);
+	write_unlock(&sysfs_read_lock);
 	return 0;
 }
 
-- 
1.7.2.5

Re: [RFC] [PATCH] xen/blkback: add locking in statistics sysfs show functions

[Konrad Rzeszutek Wilk]

On Fri, Apr 20, 2012 at 07:21:13PM +0000, Matt Wilson wrote:
> This is a port of a patch in the XenoLinux 2.6.18 tree:
> http://xenbits.xen.org/hg/linux-2.6.18-xen.hg/rev/f47c07325a56
> 
> Fix blkback sysfs race
> 
> Read blkback statistics info after remove vbd device(s) kernel
> will crash.

Looks pretty reasonable.

> 
> Signed-off-by: Joe Jin <joe.jin@oracle.com>
> Acked-by: Jan Beulich <jbeulich@novell.com>
> [Ported to Linux 3.x]
> Signed-off-by: Matt Wilson <msw@amazon.com>
> ---
>  drivers/block/xen-blkback/xenbus.c |   20 +++++++++++++++++---
>  1 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index da19506..74d4810 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -21,6 +21,8 @@
>  #include <xen/grant_table.h>
>  #include "common.h"
>  
> +static DEFINE_RWLOCK(sysfs_read_lock);
> +
>  struct backend_info {
>  	struct xenbus_device	*dev;
>  	struct xen_blkif	*blkif;
> @@ -225,10 +227,20 @@ int __init xen_blkif_interface_init(void)
>  				   struct device_attribute *attr,	\
>  				   char *buf)				\
>  	{								\
> -		struct xenbus_device *dev = to_xenbus_device(_dev);	\
> -		struct backend_info *be = dev_get_drvdata(&dev->dev);	\
> +		ssize_t ret = -ENODEV;					\
> +		struct xenbus_device *dev;				\
> +		struct backend_info *be;				\
>  									\
> -		return sprintf(buf, format, ##args);			\
> +		if (!get_device(_dev))					\
> +			return ret;					\
> +		dev = to_xenbus_device(_dev);				\
> +		read_lock(&sysfs_read_lock);				\
> +		be = (struct backend_info *) dev_get_drvdata(&dev->dev);\
> +		if (be != NULL)						\
> +			ret = sprintf(buf, format, ##args);		\
> +		read_unlock(&sysfs_read_lock);				\
> +		put_device(_dev);					\
> +		return ret;						\
>  	}								\
>  	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
>  
> @@ -353,6 +365,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
>  
>  	DPRINTK("");
>  
> +	write_lock(&sysfs_read_lock);
>  	if (be->major || be->minor)
>  		xenvbd_sysfs_delif(dev);
>  
> @@ -371,6 +384,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
>  
>  	kfree(be);
>  	dev_set_drvdata(&dev->dev, NULL);
> +	write_unlock(&sysfs_read_lock);
>  	return 0;
>  }
>  
> -- 
> 1.7.2.5

Re: [RFC] [PATCH] xen/blkback: add locking in statistics sysfs show functions

[Konrad Rzeszutek Wilk]

On Fri, Apr 20, 2012 at 07:21:13PM +0000, Matt Wilson wrote:
> This is a port of a patch in the XenoLinux 2.6.18 tree:
> http://xenbits.xen.org/hg/linux-2.6.18-xen.hg/rev/f47c07325a56
> 
> Fix blkback sysfs race

So with this patch I keep on getting this:

[  128.274290] switch: port 2(vif1.0) entered disabled state
[  128.855442] BUG: sleeping function called from invalid context at /home/konrad/ssd/linux/kernel/mutex.c:85
[  128.855628] in_atomic(): 1, irqs_disabled(): 0, pid: 46, name: xenwatch
[  128.855757] Pid: 46, comm: xenwatch Tainted: G           O 3.4.0-rc4upstream-00103-g7e4325c-dirty #1
[  128.855917] Call Trace:
[  128.856062]  [<ffffffff810983ea>] __might_sleep+0xda/0x100
[  128.856215]  [<ffffffff815a9267>] mutex_lock+0x27/0x50
[  128.856351]  [<ffffffff811b3039>] sysfs_get_dirent+0x29/0x80
[  128.856495]  [<ffffffff811b4d7b>] sysfs_remove_group+0x2b/0x100
[  128.856626]  [<ffffffff81393a20>] xenvbd_sysfs_delif+0x20/0x50
[  128.856756]  [<ffffffff81394518>] xen_blkbk_remove+0xe8/0xf0
[  128.856895]  [<ffffffff8131c768>] xenbus_dev_remove+0x38/0x70
[  128.857041]  [<ffffffff81381601>] __device_release_driver+0x61/0xc0
[  128.857177]  [<ffffffff81381778>] device_release_driver+0x28/0x40
[  128.857323]  [<ffffffff81380896>] bus_remove_device+0x106/0x140
[  128.857459]  [<ffffffff8137e868>] device_del+0x118/0x1c0
[  128.857580]  [<ffffffff8137e921>] device_unregister+0x11/0x20
[  128.857712]  [<ffffffff8139376a>] frontend_changed+0xba/0x350
[  128.857851]  [<ffffffff810337f9>] ? xen_irq_enable_direct_reloc+0x4/0x4
[  128.857982]  [<ffffffff8109928d>] ? finish_task_switch+0x4d/0x100
[  128.858106]  [<ffffffff8131ca18>] xenbus_otherend_changed+0xa8/0xb0
[  128.858236]  [<ffffffff815ab629>] ? _raw_spin_unlock_irqrestore+0x19/0x30
[  128.858368]  [<ffffffff8131cc2b>] frontend_changed+0xb/0x10
[  128.858522]  [<ffffffff8131ad3b>] xenwatch_thread+0xcb/0x190
[  128.858655]  [<ffffffff8108ec40>] ? wake_up_bit+0x40/0x40
[  128.858801]  [<ffffffff8131ac70>] ? xenbus_thread+0x320/0x320
[  128.858965]  [<ffffffff8108e716>] kthread+0x96/0xa0
[  128.859097]  [<ffffffff815b3564>] kernel_thread_helper+0x4/0x10
[  128.859235]  [<ffffffff815abb78>] ? retint_restore_args+0x5/0x6
[  128.859371]  [<ffffffff815b3560>] ? gs_change+0x13/0x13
[  128.859542] BUG: scheduling while atomic: xenwatch/46/0x00000002

whenever a guest is powered off.

Re: [RFC] [PATCH] xen/blkback: add locking in statistics sysfs show functions

[Matt Wilson]

On Thu, Apr 26, 2012 at 07:24:56PM -0700, Konrad Rzeszutek Wilk wrote:
> On Fri, Apr 20, 2012 at 07:21:13PM +0000, Matt Wilson wrote:
> > This is a port of a patch in the XenoLinux 2.6.18 tree:
> > http://xenbits.xen.org/hg/linux-2.6.18-xen.hg/rev/f47c07325a56
> > 
> > Fix blkback sysfs race
> 
> So with this patch I keep on getting this:
> 
> [  128.274290] switch: port 2(vif1.0) entered disabled state
...
> [  128.859542] BUG: scheduling while atomic: xenwatch/46/0x00000002
> 
> whenever a guest is powered off.

Oops. I hadn't had a chance to do testing of the patch, I just noticed
that the upstream version didn't have it.

I'll tweak it a bit, do some testing, and resubmit when it works.
--
msw