[PATCH 0/2] clean up blktap and change to use a dynamic major

[PATCH 0/2] clean up blktap and change to use a dynamic major

[Steven Rostedt]

The following two patches are to clean up blktap.c and to make it use a 
dynamic major instead of a hardcoded one.

The first patch does two things:

1. cleans up the style to be more compatible with the Linux Style

2. gets rid of the private data structure used in filp->private_data

The private data structure only has one item and that is the idx (index 
into the tapfds descriptors).  This is allocated on open of the device 
and freed on close.  The idx element always is the same as the minor 
number.  Instead of using this (which would not be accepted into the 
kernel), I got rid of the private structure completely and instead I 
have filp->private_data point to the descriptor itself.  This cleans up 
the code a bit.


The second patch changes the blktap to use a dynamic major.  Instead of 
a hardcoded 254, the number is dynamic, and the blktapctrl now reads 
/proc/devices to find the number to use to create the node.

NOTE:  Currently and after this patch, the blktapctrl daemon creates the 
device node. This is not the way Linux should work, and this needs to be 
done by udev.  That update should be next (after I learn how udev works :)

-- Steve

[PATCH 1/2] clean up blktap and remove private structure

[Steven Rostedt]

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

This patch cleans up the blktap.c code to make it form to the Linux 
coding style a little better.

It also removes the private data structure that is only used to store 
the index of the tabfds descriptor.  Instead the filp->private_data now 
points to the descriptor itself.

-- Steve

Signed-off-by: Steven Rostedt <srostedt@redhat.com>


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

diff -r c4f3f719d997 linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
--- a/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c	Sat Sep 23 14:54:58 2006 +0100
+++ b/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c	Wed Sep 27 10:37:03 2006 -0400
@@ -102,17 +102,11 @@ typedef struct tap_blkif {
 	blkif_t *blkif;               /*Associate blkif with tapdev          */
 } tap_blkif_t;
 
-/*Private data struct associated with the inode*/
-typedef struct private_info {
-	int idx;
-} private_info_t;
-
 /*Data struct handed back to userspace for tapdisk device to VBD mapping*/
 typedef struct domid_translate {
 	unsigned short domid;
 	unsigned short busid;
 } domid_translate_t ;
-
 
 static domid_translate_t  translate_domid[MAX_TAP_DEV];
 static tap_blkif_t *tapfds[MAX_TAP_DEV];
@@ -200,7 +194,7 @@ static struct grant_handle_pair
     + (_i)])
 
 
-static int blktap_read_ufe_ring(int idx); /*local prototypes*/
+static int blktap_read_ufe_ring(tap_blkif_t *info); /*local prototypes*/
 
 #define BLKTAP_MINOR 0  /*/dev/xen/blktap resides at device number
 			  major=254, minor numbers begin at 0            */ 
@@ -264,7 +258,8 @@ static inline int GET_NEXT_REQ(unsigned 
 {
 	int i;
 	for (i = 0; i < MAX_PENDING_REQS; i++)
-		if (idx_map[i] == INVALID_REQ) return i;
+		if (idx_map[i] == INVALID_REQ)
+			return i;
 
 	return INVALID_REQ;
 }
@@ -369,9 +364,8 @@ void signal_tapdisk(int idx)
 	info = tapfds[idx];
 	if ( (idx > 0) && (idx < MAX_TAP_DEV) && (info->pid > 0) ) {
 		ptask = find_task_by_pid(info->pid);
-		if (ptask) { 
+		if (ptask)
 			info->status = CLEANSHUTDOWN;
- 		}
 	}
 	info->blkif = NULL;
 	return;
@@ -382,7 +376,6 @@ static int blktap_open(struct inode *ino
 	blkif_sring_t *sring;
 	int idx = iminor(inode) - BLKTAP_MINOR;
 	tap_blkif_t *info;
-	private_info_t *prv;
 	int i;
 	
 	if (tapfds[idx] == NULL) {
@@ -410,9 +403,7 @@ static int blktap_open(struct inode *ino
 	SHARED_RING_INIT(sring);
 	FRONT_RING_INIT(&info->ufe_ring, sring, PAGE_SIZE);
 	
-	prv = kzalloc(sizeof(private_info_t),GFP_KERNEL);
-	prv->idx = idx;
-	filp->private_data = prv;
+	filp->private_data = info;
 	info->vma = NULL;
 
 	info->idx_map = kmalloc(sizeof(unsigned long) * MAX_PENDING_REQS, 
@@ -433,17 +424,16 @@ static int blktap_open(struct inode *ino
 
 static int blktap_release(struct inode *inode, struct file *filp)
 {
-	int idx = iminor(inode) - BLKTAP_MINOR;
-	tap_blkif_t *info;
-	
-	if (tapfds[idx] == NULL) {
+	tap_blkif_t *info = filp->private_data;
+	
+	/* can this ever happen? - sdr */
+	if (!info) {
 		WPRINTK("Trying to free device that doesn't exist "
-		       "[/dev/xen/blktap%d]\n",idx);
+		       "[/dev/xen/blktap%d]\n",iminor(inode) - BLKTAP_MINOR);
 		return -1;
 	}
-	info = tapfds[idx];
 	info->dev_inuse = 0;
-	DPRINTK("Freeing device [/dev/xen/blktap%d]\n",idx);
+	DPRINTK("Freeing device [/dev/xen/blktap%d]\n",info->minor);
 
 	/* Free the ring page. */
 	ClearPageReserved(virt_to_page(info->ufe_ring.sring));
@@ -457,8 +447,6 @@ static int blktap_release(struct inode *
 		info->vma = NULL;
 	}
 	
-	if (filp->private_data) kfree(filp->private_data);
-
 	if ( (info->status != CLEANSHUTDOWN) && (info->blkif != NULL) ) {
 		kthread_stop(info->blkif->xenblkd);
 		info->blkif->xenblkd = NULL;
@@ -491,16 +479,12 @@ static int blktap_mmap(struct file *filp
 	int size;
 	struct page **map;
 	int i;
-	private_info_t *prv;
-	tap_blkif_t *info;
-
-	/*Retrieve the dev info*/
-	prv = (private_info_t *)filp->private_data;
-	if (prv == NULL) {
+	tap_blkif_t *info = filp->private_data;
+
+	if (info == NULL) {
 		WPRINTK("blktap: mmap, retrieving idx failed\n");
 		return -ENOMEM;
 	}
-	info = tapfds[prv->idx];
 	
 	vma->vm_flags |= VM_RESERVED;
 	vma->vm_ops = &blktap_vm_ops;
@@ -556,20 +540,17 @@ static int blktap_ioctl(struct inode *in
 static int blktap_ioctl(struct inode *inode, struct file *filp,
                         unsigned int cmd, unsigned long arg)
 {
-	int idx = iminor(inode) - BLKTAP_MINOR;
+	tap_blkif_t *info = filp->private_data;
+
 	switch(cmd) {
 	case BLKTAP_IOCTL_KICK_FE: 
 	{
 		/* There are fe messages to process. */
-		return blktap_read_ufe_ring(idx);
+		return blktap_read_ufe_ring(info);
 	}
 	case BLKTAP_IOCTL_SETMODE:
 	{
-		tap_blkif_t *info = tapfds[idx];
-		
-		if ( (idx > 0) && (idx < MAX_TAP_DEV) 
-		     && (tapfds[idx] != NULL) ) 
-		{
+		if (info) {
 			if (BLKTAP_MODE_VALID(arg)) {
 				info->mode = arg;
 				/* XXX: may need to flush rings here. */
@@ -582,11 +563,7 @@ static int blktap_ioctl(struct inode *in
 	}
 	case BLKTAP_IOCTL_PRINT_IDXS:
         {
-		tap_blkif_t *info = tapfds[idx];
-		
-		if ( (idx > 0) && (idx < MAX_TAP_DEV) 
-		     && (tapfds[idx] != NULL) ) 
-		{
+		if (info) {
 			printk("User Rings: \n-----------\n");
 			printk("UF: rsp_cons: %2d, req_prod_prv: %2d "
 				"| req_prod: %2d, rsp_prod: %2d\n",
@@ -599,11 +576,7 @@ static int blktap_ioctl(struct inode *in
         }
 	case BLKTAP_IOCTL_SENDPID:
 	{
-		tap_blkif_t *info = tapfds[idx];
-		
-		if ( (idx > 0) && (idx < MAX_TAP_DEV) 
-		     && (tapfds[idx] != NULL) ) 
-		{
+		if (info) {
 			info->pid = (pid_t)arg;
 			DPRINTK("blktap: pid received %d\n", 
 			       info->pid);
@@ -631,9 +604,12 @@ static int blktap_ioctl(struct inode *in
 	case BLKTAP_IOCTL_FREEINTF:
 	{
 		unsigned long dev = arg;
-		tap_blkif_t *info = NULL;
-
-		if ( (dev > 0) && (dev < MAX_TAP_DEV) ) info = tapfds[dev];
+
+		/* Looking at another device */
+		info = NULL;
+
+		if ( (dev > 0) && (dev < MAX_TAP_DEV) )
+			info = tapfds[dev];
 
 		if ( (info != NULL) && (info->dev_pending) )
 			info->dev_pending = 0;
@@ -642,12 +618,17 @@ static int blktap_ioctl(struct inode *in
 	case BLKTAP_IOCTL_MINOR:
 	{
 		unsigned long dev = arg;
-		tap_blkif_t *info = NULL;
+
+		/* Looking at another device */
+		info = NULL;
 		
-		if ( (dev > 0) && (dev < MAX_TAP_DEV) ) info = tapfds[dev];
+		if ( (dev > 0) && (dev < MAX_TAP_DEV) )
+			info = tapfds[dev];
 		
-		if (info != NULL) return info->minor;
-		else return -1;
+		if (info != NULL)
+			return info->minor;
+		else
+			return -1;
 	}
 	case BLKTAP_IOCTL_MAJOR:
 		return BLKTAP_DEV_MAJOR;
@@ -662,23 +643,20 @@ static int blktap_ioctl(struct inode *in
 	return -ENOIOCTLCMD;
 }
 
-static unsigned int blktap_poll(struct file *file, poll_table *wait)
-{
-	private_info_t *prv;
-	tap_blkif_t *info;
-	
-	/*Retrieve the dev info*/
-	prv = (private_info_t *)file->private_data;
-	if (prv == NULL) {
+static unsigned int blktap_poll(struct file *filp, poll_table *wait)
+{
+	tap_blkif_t *info = filp->private_data;
+	
+	if (!info) {
 		WPRINTK(" poll, retrieving idx failed\n");
 		return 0;
 	}
-	
-	if (prv->idx == 0) return 0;
-	
-	info = tapfds[prv->idx];
-	
-	poll_wait(file, &info->wait, wait);
+
+	/* do not work on the control device */
+	if (!info->minor)
+		return 0;
+
+	poll_wait(filp, &info->wait, wait);
 	if (info->ufe_ring.req_prod_pvt != info->ufe_ring.sring->req_prod) {
 		flush_tlb_all();
 		RING_PUSH_REQUESTS(&info->ufe_ring);
@@ -691,11 +669,14 @@ void blktap_kick_user(int idx)
 {
 	tap_blkif_t *info;
 
-	if (idx == 0) return;
+	if (idx == 0)
+		return;
 	
 	info = tapfds[idx];
 	
-	if (info != NULL) wake_up_interruptible(&info->wait);
+	if (info != NULL)
+		wake_up_interruptible(&info->wait);
+
 	return;
 }
 
@@ -837,7 +818,8 @@ static void req_decrease(void)
 			mmap_inuse--;
 		}
 	}
-	if (mmap_inuse == 0) mmap_req_del(mmap_alloc-1);
+	if (mmap_inuse == 0)
+		mmap_req_del(mmap_alloc-1);
  done:
 	spin_unlock_irqrestore(&pending_free_lock, flags);
 	return;
@@ -1002,7 +984,7 @@ int tap_blkif_schedule(void *arg)
  * COMPLETION CALLBACK -- Called by user level ioctl()
  */
 
-static int blktap_read_ufe_ring(int idx)
+static int blktap_read_ufe_ring(tap_blkif_t *info)
 {
 	/* This is called to read responses from the UFE ring. */
 	RING_IDX i, j, rp;
@@ -1010,12 +992,9 @@ static int blktap_read_ufe_ring(int idx)
 	blkif_t *blkif=NULL;
 	int pending_idx, usr_idx, mmap_idx;
 	pending_req_t *pending_req;
-	tap_blkif_t *info;
-	
-	info = tapfds[idx];
-	if (info == NULL) {
+	
+	if (!info)
 		return 0;
-	}
 
 	/* We currently only forward packets in INTERCEPT_FE mode. */
 	if (!(info->mode & BLKTAP_MODE_INTERCEPT_FE))
@@ -1063,7 +1042,7 @@ static int blktap_read_ufe_ring(int idx)
 				>> PAGE_SHIFT;
 			map[offset] = NULL;
 		}
-		fast_flush_area(pending_req, pending_idx, usr_idx, idx);
+		fast_flush_area(pending_req, pending_idx, usr_idx, info->minor);
 		make_response(blkif, pending_req->id, resp->operation,
 			      resp->status);
 		info->idx_map[usr_idx] = INVALID_REQ;

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

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

[PATCH 2/2] have blktap use a dynamic major

[Steven Rostedt]

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

blktap currently uses a hardcoded major of 254 for the device.  This is 
not robust in anyway and needs to be dynamic.

Note: it is better not to have the daemon create the node, and have udev 
create it instead. But since the daemon currently creates the node 
anyway, it is still the way this is done.  That change needs to be made 
at another time.

-- Steve

Signed-off-by: Steven Rostedt <srostedt@redhat.com>


[-- Attachment #2: xen-linux-blktap-dynamic-major.patch --]
[-- Type: text/x-patch, Size: 4753 bytes --]

diff -r 0b662eb8ad52 linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
--- a/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c	Wed Sep 27 10:40:59 2006 -0400
+++ b/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c	Wed Sep 27 10:43:58 2006 -0400
@@ -196,12 +196,10 @@ static struct grant_handle_pair
 
 static int blktap_read_ufe_ring(tap_blkif_t *info); /*local prototypes*/
 
-#define BLKTAP_MINOR 0  /*/dev/xen/blktap resides at device number
-			  major=254, minor numbers begin at 0            */ 
-#define BLKTAP_DEV_MAJOR 254         /* TODO: Make major number dynamic  *
-                                      * and create devices in the kernel *
-				      */
+#define BLKTAP_MINOR 0  /*/dev/xen/blktap has a dynamic major */
 #define BLKTAP_DEV_DIR  "/dev/xen"
+
+static int blktap_major;
 
 /* blktap IOCTLs: */
 #define BLKTAP_IOCTL_KICK_FE         1
@@ -631,7 +629,7 @@ static int blktap_ioctl(struct inode *in
 			return -1;
 	}
 	case BLKTAP_IOCTL_MAJOR:
-		return BLKTAP_DEV_MAJOR;
+		return blktap_major;
 
 	case BLKTAP_QUERY_ALLOC_REQS:
 	{
@@ -1395,7 +1393,8 @@ static int __init blkif_init(void)
 	/*Create the blktap devices, but do not map memory or waitqueue*/
 	for(i = 0; i < MAX_TAP_DEV; i++) translate_domid[i].domid = 0xFFFF;
 
-	ret = register_chrdev(BLKTAP_DEV_MAJOR,"blktap",&blktap_fops);
+	/* Dynamically allocate a major for this device */
+	ret = register_chrdev(0, "blktap", &blktap_fops);
 	blktap_dir = devfs_mk_dir(NULL, "xen", 0, NULL);
 
 	if ( (ret < 0)||(blktap_dir < 0) ) {
@@ -1403,6 +1402,8 @@ static int __init blkif_init(void)
 		return -ENOMEM;
 	}	
 	
+	blktap_major = ret;
+
 	for(i = 0; i < MAX_TAP_DEV; i++ ) {
 		info = tapfds[i] = kzalloc(sizeof(tap_blkif_t),GFP_KERNEL);
 		if(tapfds[i] == NULL) return -ENOMEM;
@@ -1410,7 +1411,7 @@ static int __init blkif_init(void)
 		info->pid = 0;
 		info->blkif = NULL;
 
-		ret = devfs_mk_cdev(MKDEV(BLKTAP_DEV_MAJOR, i),
+		ret = devfs_mk_cdev(MKDEV(blktap_major, i),
 			S_IFCHR|S_IRUGO|S_IWUSR, "xen/blktap%d", i);
 
 		if(ret != 0) return -ENOMEM;
diff -r 0b662eb8ad52 tools/blktap/drivers/blktapctrl.c
--- a/tools/blktap/drivers/blktapctrl.c	Wed Sep 27 10:40:59 2006 -0400
+++ b/tools/blktap/drivers/blktapctrl.c	Wed Sep 27 10:43:58 2006 -0400
@@ -67,6 +67,8 @@ int max_timeout = MAX_TIMEOUT;
 int max_timeout = MAX_TIMEOUT;
 int ctlfd = 0;
 
+int blktap_major;
+
 static int open_ctrl_socket(char *devname);
 static int write_msg(int fd, int msgtype, void *ptr, void *ptr2);
 static int read_msg(int fd, int msgtype, void *ptr);
@@ -108,7 +110,18 @@ static void make_blktap_dev(char *devnam
 		if (mknod(devname, S_IFCHR|0600,
                 	makedev(major, minor)) == 0)
 			DPRINTF("Created %s device\n",devname);
-	} else DPRINTF("%s device already exists\n",devname);
+	} else {
+		DPRINTF("%s device already exists\n",devname);
+		/* it already exists, but is it the same major number */
+		if (((st.st_rdev>>8) & 0xff) != major) {
+			DPRINTF("%s has old major %d\n",
+				devname,
+				(unsigned int)((st.st_rdev >> 8) & 0xff));
+			/* only try again if we succed in deleting it */
+			if (!unlink(devname))
+				make_blktap_dev(devname, major, minor);
+		}
+	}
 }
 
 static int get_new_dev(int *major, int *minor, blkif_t *blkif)
@@ -623,6 +636,30 @@ static void print_drivers(void)
 		DPRINTF("Found driver: [%s]\n",dtypes[i]->name);
 } 
 
+static int find_blktap_major(void)
+{
+	FILE *fp;
+	int major;
+	char device[256];
+
+	if ((fp = fopen("/proc/devices", "r")) == NULL)
+		return -1;
+
+	/* Skip title */
+	fscanf(fp,"%*s %*s\n");
+	while (fscanf(fp, "%d %255s\n", &major, device) == 2) {
+		if (strncmp("blktap", device, 6) == 0)
+			break;
+	}
+
+	fclose(fp);
+
+	if (strncmp("blktap", device, 6) == 0)
+		return major;
+
+	return -1;
+}
+
 int main(int argc, char *argv[])
 {
 	char *devname;
@@ -646,7 +683,10 @@ int main(int argc, char *argv[])
 
 	/*Attach to blktap0 */	
 	asprintf(&devname,"%s/%s0", BLKTAP_DEV_DIR, BLKTAP_DEV_NAME);
-	make_blktap_dev(devname,254,0);
+	blktap_major = find_blktap_major();
+	if (blktap_major < 0)
+		goto open_failed;
+	make_blktap_dev(devname,blktap_major,0);
 	ctlfd = open(devname, O_RDWR);
 	if (ctlfd == -1) {
 		DPRINTF("blktap0 open failed\n");
diff -r 0b662eb8ad52 tools/blktap/lib/blktaplib.h
--- a/tools/blktap/lib/blktaplib.h	Wed Sep 27 10:40:59 2006 -0400
+++ b/tools/blktap/lib/blktaplib.h	Wed Sep 27 10:43:58 2006 -0400
@@ -80,8 +80,9 @@ static inline int BLKTAP_MODE_VALID(unsi
 #define MAX_PENDING_REQS 64
 #define BLKTAP_DEV_DIR   "/dev/xen"
 #define BLKTAP_DEV_NAME  "blktap"
-#define BLKTAP_DEV_MAJOR 254
 #define BLKTAP_DEV_MINOR 0
+
+extern int blktap_major;
 
 #define BLKTAP_RING_PAGES       1 /* Front */
 #define BLKTAP_MMAP_REGION_SIZE (BLKTAP_RING_PAGES + MMAP_PAGES)

[-- 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 1/2] clean up blktap and remove private structure

[Andrew Warfield]

Applied, thank you.  Great clean-up.

a.

On 9/27/06, Steven Rostedt <srostedt@redhat.com> wrote:
> This patch cleans up the blktap.c code to make it form to the Linux
> coding style a little better.
>
> It also removes the private data structure that is only used to store
> the index of the tabfds descriptor.  Instead the filp->private_data now
> points to the descriptor itself.
>
> -- Steve
>
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
>
>
>
>

Re: [PATCH 2/2] have blktap use a dynamic major

[Andrew Warfield]

Applied, thank you.

On 9/27/06, Steven Rostedt <srostedt@redhat.com> wrote:
> blktap currently uses a hardcoded major of 254 for the device.  This is
> not robust in anyway and needs to be dynamic.
>
> Note: it is better not to have the daemon create the node, and have udev
> create it instead. But since the daemon currently creates the node
> anyway, it is still the way this is done.  That change needs to be made
> at another time.
>
> -- Steve
>
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
>
>
>
>