From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Joe Jin" Subject: Re: [PATCH] Fix blkback/blktap sysfs read bug. Date: Wed, 20 Jan 2010 18:51:36 +0800 Message-ID: <20100120105136.GA6801@joejin-pc.cn.oracle.com> References: <20100119095250.GB20691@joejin-pc.cn.oracle.com> <4B5596B2020000780002AAF1@vpn.id2.novell.com> <20100119113224.GA21348@joejin-pc.cn.oracle.com> <4B55AE58020000780002AB39@vpn.id2.novell.com> <20100119141338.GA22249@joejin-pc.cn.oracle.com> <4B55E9E7020000780002AC17@vpn.id2.novell.com> <20100120020605.GA25697@joejin-pc.cn.oracle.com> <4B56C2DB020000780002AE45@vpn.id2.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <4B56C2DB020000780002AE45@vpn.id2.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: deepak.patel@oracle.com, Keir Fraser , xen-devel@lists.xensource.com, Joe Jin , greg.marsden@oracle.com List-Id: xen-devel@lists.xenproject.org On 2010-01-20 07:46, Jan Beulich wrote: > >>> "Joe Jin" 20.01.10 03:06 >>> > >sysfs did not provide lock to handle this, not sure if developer > >think it is not necessary or they'd like to caller to handled it. > > A lock is probably not the usual way to deal with this; ref-counting > would seem more common. Nevertheless I think adding a lock will > take care of the issue here. Add refcnt could not fix the race for file open is sysfs file operations and will not touch xenbus/blk{back,tap} struct, so could not increase the refcnt. The root cause is after open sysfs file, vbd was been removed by other. For the lock type is read/write lock, so the lock almost will not slowdwon performance, isn't it? > > >--- a/drivers/xen/blktap/xenbus.c Fri Jan 08 13:07:17 2010 +0000 > >+++ b/drivers/xen/blktap/xenbus.c Wed Jan 20 10:00:53 2010 +0800 > >... > >@@ -122,10 +123,15 @@ > > struct device_attribute *attr, \ > > char *buf) \ > > { \ > >+ ssize_t ret = -ENODEV; \ > > struct xenbus_device *dev = to_xenbus_device(_dev); \ > > The use of to_xenbus_device() here makes ... > > >- struct backend_info *be = dev->dev.driver_data; \ > >+ struct backend_info *be; \ > > \ > >- return sprintf(buf, format, ##args); \ > >+ read_lock(&sysfs_read_lock); \ > >+ if (dev && (be = dev->dev.driver_data) && be->blkif) \ > > ... the checking of dev here useless (and the blkback part of the patch > doesn't do the same). Sorry I forgot get rid of it :) > > >+ ret = sprintf(buf, format, ##args); \ > >+ read_unlock(&sysfs_read_lock); \ > >+ return ret; \ > > } \ > > static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL) > > > > And btw., in both cases with the lock added there's no need to check > both 'be' and 'be->blkif', since be->blkif can't be NULL when be is > non-NULL. No we must check it, as I have methioned, root cause is vbd was been freed after open sysfs file. Below is the new patch, please review. diff -r 0bec29c94ce9 drivers/xen/blkback/xenbus.c --- a/drivers/xen/blkback/xenbus.c Mon Jan 18 14:50:43 2010 +0000 +++ b/drivers/xen/blkback/xenbus.c Wed Jan 20 18:49:50 2010 +0800 @@ -26,6 +26,8 @@ #define DPRINTK(fmt, args...) \ pr_debug("blkback/xenbus (%s:%d) " fmt ".\n", \ __FUNCTION__, __LINE__, ##args) + +static rwlock_t sysfs_read_lock = RW_LOCK_UNLOCKED; struct backend_info { @@ -104,10 +106,19 @@ struct device_attribute *attr, \ char *buf) \ { \ - struct xenbus_device *dev = to_xenbus_device(_dev); \ - struct backend_info *be = dev->dev.driver_data; \ + 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); \ + if ((be = dev->dev.driver_data) && be->blkif) \ + ret = sprintf(buf, format, ##args); \ + read_unlock(&sysfs_read_lock); \ + put_device(_dev); \ + return ret; \ } \ static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL) @@ -173,6 +184,7 @@ DPRINTK(""); + write_lock(&sysfs_read_lock); if (be->major || be->minor) xenvbd_sysfs_delif(dev); @@ -191,6 +203,7 @@ kfree(be); dev->dev.driver_data = NULL; + write_unlock(&sysfs_read_lock); return 0; } diff -r 0bec29c94ce9 drivers/xen/blktap/xenbus.c --- a/drivers/xen/blktap/xenbus.c Mon Jan 18 14:50:43 2010 +0000 +++ b/drivers/xen/blktap/xenbus.c Wed Jan 20 18:49:50 2010 +0800 @@ -50,6 +50,7 @@ int group_added; }; +static rwlock_t sysfs_read_lock = RW_LOCK_UNLOCKED; static void connect(struct backend_info *); static int connect_ring(struct backend_info *); @@ -122,10 +123,19 @@ struct device_attribute *attr, \ char *buf) \ { \ - struct xenbus_device *dev = to_xenbus_device(_dev); \ - struct backend_info *be = dev->dev.driver_data; \ + 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); \ + if ((be = dev->dev.driver_data) && be->blkif) \ + ret = sprintf(buf, format, ##args); \ + read_unlock(&sysfs_read_lock); \ + put_device(_dev); \ + return ret; \ } \ static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL) @@ -170,6 +180,7 @@ { struct backend_info *be = dev->dev.driver_data; + write_lock(&sysfs_read_lock); if (be->group_added) xentap_sysfs_delif(be->dev); if (be->backend_watch.node) { @@ -187,6 +198,7 @@ } kfree(be); dev->dev.driver_data = NULL; + write_unlock(&sysfs_read_lock); return 0; }