[PATCH] have udev create the device for blktap

[PATCH] have udev create the device for blktap

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

This patch makes blktap Do The Right Thing(TM).  It allows udev to 
create the /dev/xen/blktap[0-9] devices.

It creates a sysfs class called "xen".  This part may later be placed 
someplace else, but currently blktap is the only user so it is placed in 
the blktap code.

When blktap is initialized, a blktap0 sysfs class device is made.  The 
other devices blktapX (X > 0) are made when the BLKTAP_IOCTL_NEWINTF 
ioctl is called.  This way we don't flood the sysfs and /dev/xen with 
unnecessary devices.

I added a rule in the xen-backend.rules to allow for udev to create the 
blktap devices.


With this, we can really remove the code to search and create the 
/dev/xen/blktap[0-9]*, but I'll leave it in for now. With the use of 
udev, we really should remove that code as well as the code for creating 
the evtchn device.  udev works for both of these now.  But that removal 
will have to be in another patch.

-- Steve

[-- Attachment #2: xen-linux-blktap-sysfs.patch --]
[-- Type: text/x-patch, Size: 4931 bytes --]

diff -r 34b6478419b0 linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
--- a/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c	Thu Sep 28 08:01:18 2006 -0400
+++ b/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c	Thu Sep 28 09:51:45 2006 -0400
@@ -44,7 +44,6 @@
 #include <linux/kernel.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
-#include <linux/miscdevice.h>
 #include <linux/errno.h>
 #include <linux/major.h>
 #include <linux/gfp.h>
@@ -54,6 +53,30 @@
 
 #define MAX_TAP_DEV 100     /*the maximum number of tapdisk ring devices    */
 #define MAX_DEV_NAME 100    /*the max tapdisk ring device name e.g. blktap0 */
+
+
+struct class *xen_class;
+EXPORT_SYMBOL_GPL(xen_class);
+
+/*
+ * Setup the xen class.  This should probably go in another file, but
+ * since blktap is the only user of it so far, it gets to keep it.
+ */
+int setup_xen_class(void)
+{
+	int ret;
+
+	if (xen_class)
+		return 0;
+
+	xen_class = class_create(THIS_MODULE, "xen");
+	if ((ret = IS_ERR(xen_class))) {
+		xen_class = NULL;
+		return ret;
+	}
+
+	return 0;
+}
 
 /*
  * The maximum number of requests that can be outstanding at any time
@@ -100,6 +123,7 @@ typedef struct tap_blkif {
 	unsigned long *idx_map;       /*Record the user ring id to kern 
 					[req id, idx] tuple                  */
 	blkif_t *blkif;               /*Associate blkif with tapdev          */
+	int sysfs_set;                /*Set if it has a class device.        */
 } tap_blkif_t;
 
 /*Data struct handed back to userspace for tapdisk device to VBD mapping*/
@@ -304,8 +328,6 @@ static int blktap_ioctl(struct inode *in
                         unsigned int cmd, unsigned long arg);
 static unsigned int blktap_poll(struct file *file, poll_table *wait);
 
-struct miscdevice *set_misc(int minor, char *name, int dev);
-
 static struct file_operations blktap_fops = {
 	.owner   = THIS_MODULE,
 	.poll    = blktap_poll,
@@ -337,6 +359,16 @@ static int get_next_free_dev(void)
 	
 done:
 	spin_unlock_irqrestore(&pending_free_lock, flags);
+
+	/*
+	 * We are protected by having the dev_pending set.
+	 */
+	if (!tapfds[i]->sysfs_set && xen_class) {
+		class_device_create(xen_class, NULL,
+				    MKDEV(blktap_major, ret), NULL,
+				    "blktap%d", ret);
+		tapfds[i]->sysfs_set = 1;
+	}
 	return ret;
 }
 
@@ -428,7 +460,7 @@ static int blktap_release(struct inode *
 	if (!info) {
 		WPRINTK("Trying to free device that doesn't exist "
 		       "[/dev/xen/blktap%d]\n",iminor(inode) - BLKTAP_MINOR);
-		return -1;
+		return -EBADF;
 	}
 	info->dev_inuse = 0;
 	DPRINTK("Freeing device [/dev/xen/blktap%d]\n",info->minor);
@@ -602,6 +634,7 @@ static int blktap_ioctl(struct inode *in
 	case BLKTAP_IOCTL_FREEINTF:
 	{
 		unsigned long dev = arg;
+		unsigned long flags;
 
 		/* Looking at another device */
 		info = NULL;
@@ -609,8 +642,11 @@ static int blktap_ioctl(struct inode *in
 		if ( (dev > 0) && (dev < MAX_TAP_DEV) )
 			info = tapfds[dev];
 
+		spin_lock_irqsave(&pending_free_lock, flags);
 		if ( (info != NULL) && (info->dev_pending) )
 			info->dev_pending = 0;
+		spin_unlock_irqrestore(&pending_free_lock, flags);
+
 		return 0;
 	}
 	case BLKTAP_IOCTL_MINOR:
@@ -1406,7 +1442,8 @@ static int __init blkif_init(void)
 
 	for(i = 0; i < MAX_TAP_DEV; i++ ) {
 		info = tapfds[i] = kzalloc(sizeof(tap_blkif_t),GFP_KERNEL);
-		if(tapfds[i] == NULL) return -ENOMEM;
+		if(tapfds[i] == NULL)
+			return -ENOMEM;
 		info->minor = i;
 		info->pid = 0;
 		info->blkif = NULL;
@@ -1414,12 +1451,31 @@ static int __init blkif_init(void)
 		ret = devfs_mk_cdev(MKDEV(blktap_major, i),
 			S_IFCHR|S_IRUGO|S_IWUSR, "xen/blktap%d", i);
 
-		if(ret != 0) return -ENOMEM;
+		if(ret != 0)
+			return -ENOMEM;
 		info->dev_pending = info->dev_inuse = 0;
 
 		DPRINTK("Created misc_dev [/dev/xen/blktap%d]\n",i);
 	}
 	
+	/* Make sure the xen class exists */
+	if (!setup_xen_class()) {
+		/*
+		 * This will allow udev to create the blktap ctrl device.
+		 * We only want to create blktap0 first.  We don't want
+		 * to flood the sysfs system with needless blktap devices.
+		 * We only create the device when a request of a new device is
+		 * made.
+		 */
+		class_device_create(xen_class, NULL,
+				    MKDEV(blktap_major, 0), NULL,
+				    "blktap0");
+		tapfds[0]->sysfs_set = 1;
+	} else {
+		/* this is bad, but not fatal */
+		WPRINTK("blktap: sysfs xen_class not created\n");
+	}
+
 	DPRINTK("Blktap device successfully created\n");
 
 	return 0;
diff -r 34b6478419b0 tools/examples/xen-backend.rules
--- a/tools/examples/xen-backend.rules	Thu Sep 28 08:01:18 2006 -0400
+++ b/tools/examples/xen-backend.rules	Thu Sep 28 09:51:45 2006 -0400
@@ -5,3 +5,4 @@ SUBSYSTEM=="xen-backend", KERNEL=="vif*"
 SUBSYSTEM=="xen-backend", KERNEL=="vif*", ACTION=="offline", RUN+="$env{script} offline"
 SUBSYSTEM=="xen-backend", ACTION=="remove", RUN+="/etc/xen/scripts/xen-hotplug-cleanup"
 KERNEL=="evtchn", NAME="xen/%k"
+KERNEL=="blktap[0-9]*", NAME="xen/%k"

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] have udev create the device for blktap

[Andrew Warfield]

I'm wondering about this one a bit, as I'm not so familiar with the
class stuff.  This patch isn't changing any of the device creation
code -- it's just adding calls to publish the existence of the
tap-related device nodes through sysfs so that udev creates them for
us, right?

Can you explain what the new 'xen' class does -- is it just to keep
things organized within /sys?  the /dev/xen subdirectory is created
through the udev rules, and not implicitly through having the class,
isn't it?

Assuming this is true, I wonder if it's worth broadening this patch
slightly to also incorporate the event channel driver, and to pull the
xen class stuff out into drivers/xen/core/sysfs_class.c or similar.

Also, on the blocktap side, it would be nice (in a later patch) to
kill the static allocation of the data-path devices and allocate them
on demand, as you do with the associed sysfs entries.

a.

On 9/28/06, Steven Rostedt <srostedt@redhat.com> wrote:
> This patch makes blktap Do The Right Thing(TM).  It allows udev to
> create the /dev/xen/blktap[0-9] devices.
>
> It creates a sysfs class called "xen".  This part may later be placed
> someplace else, but currently blktap is the only user so it is placed in
> the blktap code.
>
> When blktap is initialized, a blktap0 sysfs class device is made.  The
> other devices blktapX (X > 0) are made when the BLKTAP_IOCTL_NEWINTF
> ioctl is called.  This way we don't flood the sysfs and /dev/xen with
> unnecessary devices.
>
> I added a rule in the xen-backend.rules to allow for udev to create the
> blktap devices.
>
>
> With this, we can really remove the code to search and create the
> /dev/xen/blktap[0-9]*, but I'll leave it in for now. With the use of
> udev, we really should remove that code as well as the code for creating
> the evtchn device.  udev works for both of these now.  But that removal
> will have to be in another patch.
>
> -- Steve
>
>
>

Re: [PATCH] have udev create the device for blktap

[Andrew Warfield]

New plan -- after looking at the sysfs stuff a bit more and chatting
to Keir, I've applied the patch as-is.  the event channel is probably
okay for the moment as a misc device, and we can abstract out the xen
class from blktap if/when there's something else that wants to use it.

Pretty much as you said in your original submission. ;)

cheers,
a.

On 9/28/06, Andrew Warfield <andrew.warfield@cl.cam.ac.uk> wrote:
> I'm wondering about this one a bit, as I'm not so familiar with the
> class stuff.  This patch isn't changing any of the device creation
> code -- it's just adding calls to publish the existence of the
> tap-related device nodes through sysfs so that udev creates them for
> us, right?
>
> Can you explain what the new 'xen' class does -- is it just to keep
> things organized within /sys?  the /dev/xen subdirectory is created
> through the udev rules, and not implicitly through having the class,
> isn't it?
>
> Assuming this is true, I wonder if it's worth broadening this patch
> slightly to also incorporate the event channel driver, and to pull the
> xen class stuff out into drivers/xen/core/sysfs_class.c or similar.
>
> Also, on the blocktap side, it would be nice (in a later patch) to
> kill the static allocation of the data-path devices and allocate them
> on demand, as you do with the associed sysfs entries.
>
> a.
>
> On 9/28/06, Steven Rostedt <srostedt@redhat.com> wrote:
> > This patch makes blktap Do The Right Thing(TM).  It allows udev to
> > create the /dev/xen/blktap[0-9] devices.
> >
> > It creates a sysfs class called "xen".  This part may later be placed
> > someplace else, but currently blktap is the only user so it is placed in
> > the blktap code.
> >
> > When blktap is initialized, a blktap0 sysfs class device is made.  The
> > other devices blktapX (X > 0) are made when the BLKTAP_IOCTL_NEWINTF
> > ioctl is called.  This way we don't flood the sysfs and /dev/xen with
> > unnecessary devices.
> >
> > I added a rule in the xen-backend.rules to allow for udev to create the
> > blktap devices.
> >
> >
> > With this, we can really remove the code to search and create the
> > /dev/xen/blktap[0-9]*, but I'll leave it in for now. With the use of
> > udev, we really should remove that code as well as the code for creating
> > the evtchn device.  udev works for both of these now.  But that removal
> > will have to be in another patch.
> >
> > -- Steve
> >
> >
> >
>

Re: [PATCH] have udev create the device for blktap

[Steven Rostedt]

Andrew Warfield wrote:
> I'm wondering about this one a bit, as I'm not so familiar with the
> class stuff.  This patch isn't changing any of the device creation
> code -- it's just adding calls to publish the existence of the
> tap-related device nodes through sysfs so that udev creates them for
> us, right?

Yep.

> 
> Can you explain what the new 'xen' class does -- is it just to keep
> things organized within /sys?  the /dev/xen subdirectory is created
> through the udev rules, and not implicitly through having the class,
> isn't it?

The class seemed the most reasonable way to organize it in sysfs.  The 
directory is still created by the udev rules, but that is still the norm.

> 
> Assuming this is true, I wonder if it's worth broadening this patch
> slightly to also incorporate the event channel driver, and to pull the
> xen  class stuff out into drivers/xen/core/sysfs_class.c or similar.

That can be done.  I was going to do something with the evtchn but since 
it was a misc device, it is automatically registered in the sysfs 
system.  And udev uses that to create it.  Although we should probably 
have the evtchn grab a dynamic minor from the misc system.  And for 
those systems without udev, we could have the daemon read the sysfs 
system to find the minor. (I should probably change the blktapctrl to 
read sysfs instead of /proc/devices).

As noted in the code, I would have put it else where (perhaps it's own 
file), but since blktap was the only user (so far) I kept it there. Hmm, 
I guess I can through that code into its own file.  Would be a small 
file though :)   Would that be the preferred method?

> 
> Also, on the blocktap side, it would be nice (in a later patch) to
> kill the static allocation of the data-path devices and allocate them
> on demand, as you do with the associed sysfs entries.

That would not be a problem.  The tap devices could be stored in a link 
list. The only time you would need to search the list to find the tap 
device, is really on open.  You would also do it to look for a free 
device, but that search is already done.

I'll take a look into that too.


-- Steve

Re: [PATCH] have udev create the device for blktap

[Andrew Warfield]

Ships in the night, and so on -- as I posted concurrently with this, I
applied the patch. ;)

Anyhow, thanks for the clarifications, and I agree with your points
about evntchn moving to a dynamic minor and the tap interrogation
moving from /proc/devices to /sys -- that would all be great.

> > Also, on the blocktap side, it would be nice (in a later patch) to
> > kill the static allocation of the data-path devices and allocate them
> > on demand, as you do with the associed sysfs entries.
>
> That would not be a problem.  The tap devices could be stored in a link
> list. The only time you would need to search the list to find the tap
> device, is really on open.  You would also do it to look for a free
> device, but that search is already done.
>
> I'll take a look into that too.

Perfect -- this would be a very welcome patch.

Thanks again!
a.