Re: [PATCH] CDRW packet writing support for 2.6.7-bk13

From: Peter Osterlund (petero2_at_telia.com)
Date: 07/05/04

  • Next message: Peter Osterlund: "Re: [PATCH] CDRW packet writing support for 2.6.7-bk13" 
    To: Christoph Hellwig <hch@infradead.org>
Date:	05 Jul 2004 01:49:43 +0200

    Hi!

    Thanks a lot for reviewing the patch.

    Christoph Hellwig <hch@infradead.org> writes:

    > > + * - Generic interface for UDF to submit large packets for variable length
    > > + * packet writing
    >
    > Huh, what's bad about bios?

    Nothing, that comment is a leftover from before bios existed. Fixed.

    > > +#include <linux/buffer_head.h>
    >
    > Where do you need buffer_head.h?

    For fsync_bdev(), which you say is not needed anyway. Also for the
    invalidate_bdev() call in pkt_remove_dev(). The loop driver is also
    calling invalidate_bdev() in its loop_clr_fd() function, so I guess
    that call is needed.

    > > +#define SCSI_IOCTL_SEND_COMMAND 1
    >
    > Please include scsi_ioctl.h instead of duplicating it.

    Done.

    > > +static struct gendisk *disks[MAX_WRITERS];
    >
    > Please add a pointer to the gendisk to struct pktcdvd_device instead
    > of a global array

    Done.

    > > +
    > > +static struct pktcdvd_device *pkt_find_dev(request_queue_t *q)
    > > +{
    > > + int i;
    > > +
    > > + for (i = 0; i < MAX_WRITERS; i++)
    > > + if (pkt_devs[i].bdev && bdev_get_queue(pkt_devs[i].bdev) == q)
    > > + return &pkt_devs[i];
    > > +
    > > + return NULL;
    > > +}
    >
    > Please just store the pktcdvd_device * in q->queuedata.

    I don't think that's possible, because q is the queue for the device
    this device is attached to, and the driver for that device might use
    queuedata for something else.

    > > + sprintf(current->comm, pd->name);
    >
    > not needed, saemonize does it for you.

    Fixed.

    > > +static int pkt_get_minor(struct pktcdvd_device *pd)
    > > +{
    > > + int minor;
    > > + for (minor = 0; minor < MAX_WRITERS; minor++)
    > > + if (pd == &pkt_devs[minor])
    > > + break;
    > > + BUG_ON(minor == MAX_WRITERS);
    > > + return minor;
    > > +}
    >
    > Shouldn't be needed at all.

    Removed.

    > > + pd->cdrw.elv_merge_fn = q->elevator.elevator_merge_fn;
    > > + pd->cdrw.elv_completed_req_fn = q->elevator.elevator_completed_req_fn;
    > > + pd->cdrw.merge_requests_fn = q->merge_requests_fn;
    > > + q->elevator.elevator_merge_fn = pkt_lowlevel_elv_merge_fn;
    > > + q->elevator.elevator_completed_req_fn = pkt_lowlevel_elv_completed_req_fn;
    >
    > This looks really really fishy. Playing with other drivers' elevator settings
    > can't be safe. I'd say wait for the runtime selectable I/O scheduler that's
    > planned for a while and add a special packetwriting scheduler that you switch
    > to.

    OK, I'll wait. ;)

    > > +/*
    > > + * called when the device is closed. makes sure that the device flushes
    > > + * the internal cache before we close.
    > > + */
    > > +static void pkt_release_dev(struct pktcdvd_device *pd, int flush)
    > > +{
    > > + struct block_device *bdev;
    > > +
    > > + atomic_dec(&pd->refcnt);
    > > + if (atomic_read(&pd->refcnt) > 0)
    > > + return;
    > > +
    > > + bdev = bdget(pd->pkt_dev);
    > > + if (bdev) {
    >
    > You reallu should keep a reference to the underlying bdev as long as you use
    > it, aka bdev_get + blkdev_get in ->open, blkdev_put in ->release

    I'll work on this tomorrow.

    > > + fsync_bdev(bdev);
    >
    > fs/block_dev.c already does a sync_blockdev() on last close, that should
    > be enough.

    OK, removed.

    > > +static int pkt_proc_device(struct pktcdvd_device *pd, char *buf)
    > > +{
    >
    > seq_file interface please, or even better a one value per file sysfs
    > interface.

    I'll work on this tomorrow too.

    > > + pd->cdrw.pid = kernel_thread(kcdrwd, pd, CLONE_FS | CLONE_FILES | CLONE_SIGHAND);
    >
    > please use the kernel/ktread.c infastructure.

    Fixed.

    > > +static int pkt_setup_dev(struct pktcdvd_device *pd, unsigned int arg)
    > > +{
    > > + struct inode *inode;
    > > + struct file *file;
    > > + int ret;
    > > +
    > > + if ((file = fget(arg)) == NULL) {
    > > + printk("pktcdvd: bad file descriptor passed\n");
    > > + return -EBADF;
    > > + }
    > > +
    > > + ret = -EINVAL;
    > > + if ((inode = file->f_dentry->d_inode) == NULL) {
    > > + printk("pktcdvd: huh? file descriptor contains no inode?\n");
    > > + goto out;
    > > + }
    >
    > If fget is successfull file->f_dentry->d_inode can't be NULL.

    Fixed.

    > > + case BLKROSET:
    > > + if (capable(CAP_SYS_ADMIN))
    > > + clear_bit(PACKET_WRITABLE, &pd->flags);
    > > + case BLKROGET:
    > > + case BLKSSZGET:
    > > + case BLKFLSBUF:
    > > + if (!pd->bdev)
    > > + return -ENXIO;
    > > + return -EINVAL; /* Handled by blkdev layer */
    > > +
    >
    > These aren't handled by drivers anyore in 2.6

    OK, removed.

    > > + for (i = 0; i < MAX_WRITERS; i++) {
    > > + disks[i] = alloc_disk(1);
    > > + if (!disks[i])
    > > + goto out_mem2;
    > > + }
    > > +
    > > + for (i = 0; i < MAX_WRITERS; i++) {
    > > + struct pktcdvd_device *pd = &pkt_devs[i];
    > > + struct gendisk *disk = disks[i];
    > > + disk->major = PACKET_MAJOR;
    > > + disk->first_minor = i;
    > > + disk->fops = &pktcdvd_ops;
    > > + disk->flags = GENHD_FL_REMOVABLE;
    > > + sprintf(disk->disk_name, "pktcdvd%d", i);
    > > + sprintf(disk->devfs_name, "pktcdvd/%d", i);
    > > + disk->private_data = pd;
    > > + disk->queue = blk_alloc_queue(GFP_KERNEL);
    > > + if (!disk->queue)
    > > + goto out_mem3;
    > > + add_disk(disk);
    > > + }
    >
    > Please allocate all these on demand only when you actually attach
    > a device.

    But I need to open the device to be able to perform the ioctl to
    attach a device, and I can't open the device until add_disk() has been
    called. The loop device does the same thing.

    > All in all I really wonder whether a separate driver is really that
    > a good fit for the functionality or whether it should be more
    > integrated with the block layer, ala drivers/block/scsi_ioctl.c

    Jens is probably better suited than me to comment on that.

    Here is an incremental patch that goes on top all patches I've posted
    previously.

    ---------------

    Various cleanups in the pktcdvd driver suggested by Christoph Hellwig.

    - Removed obsolete comment.
    - Don't redefine SCSI_IOCTL_SEND_COMMAND locally, use the proper
      #include instead.
    - Removed the disks[] gendisk* array and added a gendisk pointer to
      struct pktcdvd_device instead.
    - No need to set current->comm in kcdrwd, since daemonize does that by
      itself.
    - No need to call fsync_bdev() in pkt_release_dev(), because it is
      handled by fs/block_dev.c.
    - After a successful fget(), file->f_dentry->d_inode can't be NULL, so
      kill the useless check.
    - The BLKROSET, BLKROGET, BLKSSZGET and BLKFLSBUF ioctls aren't
      handled by drivers any more in 2.6.
    - Removed no longer needed function pkt_get_minor().
    - Use the kernel/kthread.c infrastructure in the pktcdvd driver.

    Signed-off-by: Peter Osterlund <petero2@telia.com> 

    ---
 linux-petero/drivers/block/pktcdvd.c |  117 ++++++++---------------------------
 linux-petero/include/linux/pktcdvd.h |    5 -
 2 files changed, 31 insertions(+), 91 deletions(-)
diff -puN drivers/block/pktcdvd.c~packet-cleanup drivers/block/pktcdvd.c
--- linux/drivers/block/pktcdvd.c~packet-cleanup	2004-07-05 01:38:40.535610008 +0200
+++ linux-petero/drivers/block/pktcdvd.c	2004-07-05 01:39:06.680635360 +0200
@@ -12,8 +12,6 @@
  * TODO: (circa order of when I will fix it)
  * - Only able to write on CD-RW media right now.
  * - check host application code on media and set it in write page
- * - Generic interface for UDF to submit large packets for variable length
- *   packet writing
  * - interface for UDF <-> packet to negotiate a new location when a write
  *   fails.
  * - handle OPC, especially for -RW media
@@ -40,24 +38,23 @@
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
+#include <linux/kthread.h>
 #include <linux/errno.h>
 #include <linux/spinlock.h>
 #include <linux/file.h>
 #include <linux/proc_fs.h>
-#include <linux/buffer_head.h>
+#include <linux/buffer_head.h>		/* for invalidate_bdev() */
 #include <linux/devfs_fs_kernel.h>
 #include <linux/suspend.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_ioctl.h>
 
 #include <asm/uaccess.h>
 
-#define SCSI_IOCTL_SEND_COMMAND	1
-
 #define ZONE(sector, pd) (((sector) + (pd)->offset) & ~((pd)->settings.size - 1))
 
 static struct pktcdvd_device *pkt_devs;
 static struct proc_dir_entry *pkt_proc;
-static struct gendisk *disks[MAX_WRITERS];
 
 static struct pktcdvd_device *pkt_find_dev(request_queue_t *q)
 {
@@ -1170,19 +1167,7 @@ static int kcdrwd(void *foobar)
 	struct packet_data *pkt;
 	long min_sleep_time, residue;
 
-	/*
-	 * exit_files, mm (move to lazy-tlb, so context switches are come
-	 * extremely cheap) etc
-	 */
-	daemonize(pd->name);
-
 	set_user_nice(current, -20);
-	sprintf(current->comm, pd->name);
-
-	siginitsetinv(&current->blocked, sigmask(SIGKILL));
-	flush_signals(current);
-
-	up(&pd->cdrw.thr_sem);
 
 	for (;;) {
 		DECLARE_WAITQUEUE(wait, current);
@@ -1250,14 +1235,14 @@ static int kcdrwd(void *foobar)
 			if (signal_pending(current)) {
 				flush_signals(current);
 			}
-			if (pd->cdrw.time_to_die)
+			if (kthread_should_stop())
 				break;
 		}
 work_to_do:
 		set_current_state(TASK_RUNNING);
 		remove_wait_queue(&pd->wqueue, &wait);
 
-		if (pd->cdrw.time_to_die)
+		if (kthread_should_stop())
 			break;
 
 		/*
@@ -1278,7 +1263,6 @@ work_to_do:
 		pkt_iosched_process_queue(pd);
 	}
 
-	up(&pd->cdrw.thr_sem);
 	return 0;
 }
 
@@ -1898,16 +1882,6 @@ static int pkt_open_write(struct pktcdvd
 	return 0;
 }
 
-static int pkt_get_minor(struct pktcdvd_device *pd)
-{
-	int minor;
-	for (minor = 0; minor < MAX_WRITERS; minor++)
-		if (pd == &pkt_devs[minor])
-			break;
-	BUG_ON(minor == MAX_WRITERS);
-	return minor;
-}
-
 /*
  * called at open time.
  */
@@ -1938,7 +1912,7 @@ static int pkt_open_dev(struct pktcdvd_d
 		return ret;
 	}
 
-	set_capacity(disks[pkt_get_minor(pd)], lba << 2);
+	set_capacity(pd->disk, lba << 2);
 
 	/*
 	 * The underlying block device needs to have its merge logic
@@ -1996,14 +1970,6 @@ restore_queue:
  */
 static void pkt_release_dev(struct pktcdvd_device *pd, int flush)
 {
-	struct block_device *bdev;
-
-	bdev = bdget(pd->pkt_dev);
-	if (bdev) {
-		fsync_bdev(bdev);
-		bdput(bdev);
-	}
-
 	if (flush && pkt_flush_cache(pd))
 		DPRINTK("pktcdvd: %s not flushing cache\n", pd->name);
 
@@ -2300,7 +2266,7 @@ static int pkt_merge_bvec(request_queue_
 
 static void pkt_init_queue(struct pktcdvd_device *pd)
 {
-	request_queue_t *q = disks[pkt_get_minor(pd)]->queue;
+	request_queue_t *q = pd->disk->queue;
 
 	blk_queue_make_request(q, pkt_make_request);
 	blk_queue_hardsect_size(q, CD_FRAMESIZE);
@@ -2390,19 +2356,17 @@ static int pkt_read_proc(char *page, cha
 static int pkt_new_dev(struct pktcdvd_device *pd, struct block_device *bdev)
 {
 	dev_t dev = bdev->bd_dev;
-	int minor;
+	int i;
 	int ret = 0;
 	char b[BDEVNAME_SIZE];
 
-	for (minor = 0; minor < MAX_WRITERS; minor++) {
-		if (pkt_devs[minor].dev == dev) {
+	for (i = 0; i < MAX_WRITERS; i++) {
+		if (pkt_devs[i].dev == dev) {
 			printk("pktcdvd: %s already setup\n", bdevname(bdev, b));
 			return -EBUSY;
 		}
 	}
 
-	minor = pkt_get_minor(pd);
-
 	/* This is safe, since we have a reference from open(). */
 	__module_get(THIS_MODULE);
 
@@ -2420,17 +2384,15 @@ static int pkt_new_dev(struct pktcdvd_de
 
 	pkt_init_queue(pd);
 
-	pd->cdrw.time_to_die = 0;
-	pd->cdrw.pid = kernel_thread(kcdrwd, pd, CLONE_FS | CLONE_FILES | CLONE_SIGHAND);
-	if (pd->cdrw.pid < 0) {
+	pd->cdrw.thread = kthread_run(kcdrwd, pd, "%s", pd->name);
+	if (IS_ERR(pd->cdrw.thread)) {
 		printk("pktcdvd: can't start kernel thread\n");
-		ret = -EBUSY;
+		ret = -ENOMEM;
 		goto out_thread;
 	}
-	down(&pd->cdrw.thr_sem);
 
 	create_proc_read_entry(pd->name, 0, pkt_proc, pkt_read_proc, pd);
-	DPRINTK("pktcdvd: writer %d mapped to %s\n", minor, bdevname(bdev, b));
+	DPRINTK("pktcdvd: writer %s mapped to %s\n", pd->name, bdevname(bdev, b));
 	return 0;
 
 out_thread:
@@ -2455,11 +2417,7 @@ static int pkt_setup_dev(struct pktcdvd_
 		return -EBADF;
 	}
 
-	ret = -EINVAL;
-	if ((inode = file->f_dentry->d_inode) == NULL) {
-		printk("pktcdvd: huh? file descriptor contains no inode?\n");
-		goto out;
-	}
+	inode = file->f_dentry->d_inode;
 	ret = -ENOTBLK;
 	if (!S_ISBLK(inode->i_mode)) {
 		printk("pktcdvd: device is not a block device (duh)\n");
@@ -2480,18 +2438,9 @@ out:
 static int pkt_remove_dev(struct pktcdvd_device *pd)
 {
 	struct block_device *bdev;
-	int ret;
 
-	if (pd->cdrw.pid >= 0) {
-		pd->cdrw.time_to_die = 1;
-		wmb();
-		ret = kill_proc(pd->cdrw.pid, SIGKILL, 1);
-		if (ret) {
-			printk("pkt_remove_dev: can't kill kernel thread\n");
-			return ret;
-		}
-		down(&pd->cdrw.thr_sem);
-	}
+	if (!IS_ERR(pd->cdrw.thread))
+		kthread_stop(pd->cdrw.thread);
 
 	/*
 	 * will also invalidate buffers for CD-ROM
@@ -2506,7 +2455,7 @@ static int pkt_remove_dev(struct pktcdvd
 
 	remove_proc_entry(pd->name, pkt_proc);
 	pd->dev = 0;
-	DPRINTK("pktcdvd: writer %d unmapped\n", pkt_get_minor(pd));
+	DPRINTK("pktcdvd: writer %s unmapped\n", pd->name);
 	/* This is safe: open() is still holding a reference. */
 	module_put(THIS_MODULE);
 	return 0;
@@ -2566,16 +2515,6 @@ static int pkt_ioctl(struct inode *inode
 		up(&pd->ctl_mutex);
 		return ret;
 
-	case BLKROSET:
-		if (capable(CAP_SYS_ADMIN))
-			clear_bit(PACKET_WRITABLE, &pd->flags);
-	case BLKROGET:
-	case BLKSSZGET:
-	case BLKFLSBUF:
-		if (!pd->bdev)
-			return -ENXIO;
-		return -EINVAL;		    /* Handled by blkdev layer */
-
 	/*
 	 * forward selected CDROM ioctls to CD-ROM, for UDF
 	 */
@@ -2630,21 +2569,22 @@ int pkt_init(void)
 	memset(pkt_devs, 0, MAX_WRITERS * sizeof(struct pktcdvd_device));
 
 	for (i = 0; i < MAX_WRITERS; i++) {
-		disks[i] = alloc_disk(1);
-		if (!disks[i])
+		struct pktcdvd_device *pd = &pkt_devs[i];
+
+		pd->disk = alloc_disk(1);
+		if (!pd->disk)
 			goto out_mem2;
 	}
 
 	for (i = 0; i < MAX_WRITERS; i++) {
 		struct pktcdvd_device *pd = &pkt_devs[i];
-		struct gendisk *disk = disks[i];
+		struct gendisk *disk = pd->disk;
 
 		spin_lock_init(&pd->lock);
 		spin_lock_init(&pd->iosched.lock);
 		pd->pkt_dev = MKDEV(PACKET_MAJOR, i);
 		sprintf(pd->name, "pktcdvd%d", i);
 		init_waitqueue_head(&pd->wqueue);
-		init_MUTEX_LOCKED(&pd->cdrw.thr_sem);
 		init_MUTEX(&pd->ctl_mutex);
 
 		disk->major = PACKET_MAJOR;
@@ -2667,11 +2607,11 @@ int pkt_init(void)
 
 out_mem3:
 	while (i--)
-		blk_put_queue(disks[i]->queue);
+		blk_put_queue(pkt_devs[i].disk->queue);
 	i = MAX_WRITERS;
 out_mem2:
 	while (i--)
-		put_disk(disks[i]);
+		put_disk(pkt_devs[i].disk);
 	kfree(pkt_devs);
 out_mem:
 	printk("pktcdvd: out of memory\n");
@@ -2684,9 +2624,10 @@ void pkt_exit(void)
 {
 	int i;
 	for (i = 0; i < MAX_WRITERS; i++) {
-		del_gendisk(disks[i]);
-		blk_put_queue(disks[i]->queue);
-		put_disk(disks[i]);
+		struct gendisk *disk = pkt_devs[i].disk;
+		del_gendisk(disk);
+		blk_put_queue(disk->queue);
+		put_disk(disk);
 	}
 
 	devfs_remove("pktcdvd");
diff -puN include/linux/pktcdvd.h~packet-cleanup include/linux/pktcdvd.h
--- linux/include/linux/pktcdvd.h~packet-cleanup	2004-07-05 01:38:40.538609552 +0200
+++ linux-petero/include/linux/pktcdvd.h	2004-07-05 01:39:06.681635208 +0200
@@ -138,9 +138,7 @@ struct packet_cdrw
 	struct list_head	pkt_free_list;
 	struct list_head	pkt_active_list;
 	spinlock_t		active_list_lock; /* Serialize access to pkt_active_list */
-	pid_t			pid;
-	int			time_to_die;
-	struct semaphore	thr_sem;
+	struct task_struct	*thread;
 	elevator_merge_fn	*elv_merge_fn;
 	elevator_completed_req_fn *elv_completed_req_fn;
 	merge_requests_fn	*merge_requests_fn;
@@ -256,6 +254,7 @@ struct pktcdvd_device
 	atomic_t		scan_queue;	/* Set to non-zero when pkt_handle_queue */
 						/* needs to be run. */
 	struct packet_iosched   iosched;
+	struct gendisk		*disk;
 };
 
 #endif /* __KERNEL__ */
_
-- 
Peter Osterlund - petero2@telia.com
http://w1.894.telia.com/~u89404340
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
  • Next message: Peter Osterlund: "Re: [PATCH] CDRW packet writing support for 2.6.7-bk13"

    Relevant Pages