[RFC] blktrace interface for sg devices

[RFC] blktrace interface for sg devices

[Christof Schmitt]

I am referring to the discussion of introducing statistics in the SCSI
layer and the conclusion that blktrace already provides the data:
http://lkml.org/lkml/2006/10/21/72
http://lkml.org/lkml/2006/11/2/141

While blktrace works fine for disk devices, it currently does not
provide data for non-disk devices like tape drives. To close this gap,
i am looking for a way to get the same trace data also from other SCSI
devices.

Since the SCSI layer internally uses the same request queuest for all
devices and the queues already use the blktrace interface, the main
missing part is the interface to enable the tracing for all SCSI
devices.

Attached is a patch that adds the ioctl interface for blktrace to the
sg generic scsi interface. This already allows to get some trace data
for SCSI tape drives, although i have to do more testing.

For testing, any sg device file can be passed to blktrace, e.g.:
# blktrace -d /dev/sg1 -o - | blkparse -i -

I am seeking input in this approach: Is this approach worth pursuing
to enable blktrace to trace SCSI tape drives? Would there be a better
approach to get this trace data?

Christof Schmitt

---
 block/blktrace.c       |   19 +++++++++++--------
 drivers/scsi/sg.c      |   12 ++++++++++++
 include/linux/blkdev.h |   10 ++++++++++
 3 files changed, 33 insertions(+), 8 deletions(-)

--- a/block/blktrace.c	2007-12-13 08:48:23.000000000 +0100
+++ b/block/blktrace.c	2007-12-13 08:48:25.000000000 +0100
@@ -231,7 +231,7 @@ static void blk_trace_cleanup(struct blk
 	kfree(bt);
 }
 
-static int blk_trace_remove(struct request_queue *q)
+int blk_trace_remove(struct request_queue *q)
 {
 	struct blk_trace *bt;
 
@@ -245,6 +245,7 @@ static int blk_trace_remove(struct reque
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(blk_trace_remove);
 
 static int blk_dropped_open(struct inode *inode, struct file *filp)
 {
@@ -312,13 +313,11 @@ static struct rchan_callbacks blk_relay_
 /*
  * Setup everything required to start tracing
  */
-static int blk_trace_setup(struct request_queue *q, struct block_device *bdev,
-			   char __user *arg)
+int blk_trace_setup(struct request_queue *q, char *name, dev_t dev, char __user *arg)
 {
 	struct blk_user_trace_setup buts;
 	struct blk_trace *old_bt, *bt = NULL;
 	struct dentry *dir = NULL;
-	char b[BDEVNAME_SIZE];
 	int ret, i;
 
 	if (copy_from_user(&buts, arg, sizeof(buts)))
@@ -327,7 +326,7 @@ static int blk_trace_setup(struct reques
 	if (!buts.buf_size || !buts.buf_nr)
 		return -EINVAL;
 
-	strcpy(buts.name, bdevname(bdev, b));
+	strcpy(buts.name, name);
 
 	/*
 	 * some device names have larger paths - convert the slashes
@@ -355,7 +354,7 @@ static int blk_trace_setup(struct reques
 		goto err;
 
 	bt->dir = dir;
-	bt->dev = bdev->bd_dev;
+	bt->dev = dev;
 	atomic_set(&bt->dropped, 0);
 
 	ret = -EIO;
@@ -400,8 +399,9 @@ err:
 	}
 	return ret;
 }
+EXPORT_SYMBOL_GPL(blk_trace_setup);
 
-static int blk_trace_startstop(struct request_queue *q, int start)
+int blk_trace_startstop(struct request_queue *q, int start)
 {
 	struct blk_trace *bt;
 	int ret;
@@ -434,6 +434,7 @@ static int blk_trace_startstop(struct re
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(blk_trace_startstop);
 
 /**
  * blk_trace_ioctl: - handle the ioctls associated with tracing
@@ -446,6 +447,7 @@ int blk_trace_ioctl(struct block_device 
 {
 	struct request_queue *q;
 	int ret, start = 0;
+	char b[BDEVNAME_SIZE];
 
 	q = bdev_get_queue(bdev);
 	if (!q)
@@ -455,7 +457,8 @@ int blk_trace_ioctl(struct block_device 
 
 	switch (cmd) {
 	case BLKTRACESETUP:
-		ret = blk_trace_setup(q, bdev, arg);
+		strcpy(b, bdevname(bdev, b));
+		ret = blk_trace_setup(q, b, bdev->bd_dev, arg);
 		break;
 	case BLKTRACESTART:
 		start = 1;
--- a/drivers/scsi/sg.c	2007-12-13 08:48:23.000000000 +0100
+++ b/drivers/scsi/sg.c	2007-12-13 08:48:25.000000000 +0100
@@ -55,6 +55,8 @@ static int sg_version_num = 30534;	/* 2 
 #include <scsi/scsi_ioctl.h>
 #include <scsi/sg.h>
 
+#include <linux/blktrace_api.h>
+
 #include "scsi_logging.h"
 
 #ifdef CONFIG_SCSI_PROC_FS
@@ -1066,6 +1068,16 @@ sg_ioctl(struct inode *inode, struct fil
 	case BLKSECTGET:
 		return put_user(sdp->device->request_queue->max_sectors * 512,
 				ip);
+	case BLKTRACESETUP:
+	{
+		return blk_trace_setup(sdp->device->request_queue , sdp->device->sdev_gendev.bus_id, &sdp->device->sdev_gendev, arg);
+	}
+	case BLKTRACESTART:
+		return blk_trace_startstop(sdp->device->request_queue, 1);
+	case BLKTRACESTOP:
+		return blk_trace_startstop(sdp->device->request_queue, 0);
+	case BLKTRACETEARDOWN:
+		return blk_trace_remove(sdp->device->request_queue);
 	default:
 		if (read_only)
 			return -EPERM;	/* don't know so take safe approach */
--- a/include/linux/blkdev.h	2007-12-13 08:48:23.000000000 +0100
+++ b/include/linux/blkdev.h	2007-12-13 08:48:25.000000000 +0100
@@ -747,6 +747,16 @@ static inline void blkdev_dequeue_reques
 	elv_dequeue_request(req->q, req);
 }
 
+#ifdef CONFIG_BLK_DEV_IO_TRACE
+extern int blk_trace_setup(request_queue_t *q,  char * name, dev_t dev, char __user *arg);
+extern int blk_trace_startstop(request_queue_t *q, int start);
+extern int blk_trace_remove(request_queue_t *q);
+#else
+#define blk_trace_setup(q, name, dev, arg) do { } while(0)
+#define blk_trace_startstop(q, start) do { } while(0)
+#define blk_trace_remove(q) do { } while(0)
+#endif
+
 /*
  * Access functions for manipulating queue properties
  */

Re: [RFC] blktrace interface for sg devices

[Jens Axboe]

On Thu, Dec 13 2007, Christof Schmitt wrote:
> I am referring to the discussion of introducing statistics in the SCSI
> layer and the conclusion that blktrace already provides the data:
> http://lkml.org/lkml/2006/10/21/72
> http://lkml.org/lkml/2006/11/2/141
> 
> While blktrace works fine for disk devices, it currently does not
> provide data for non-disk devices like tape drives. To close this gap,
> i am looking for a way to get the same trace data also from other SCSI
> devices.
> 
> Since the SCSI layer internally uses the same request queuest for all
> devices and the queues already use the blktrace interface, the main
> missing part is the interface to enable the tracing for all SCSI
> devices.
> 
> Attached is a patch that adds the ioctl interface for blktrace to the
> sg generic scsi interface. This already allows to get some trace data
> for SCSI tape drives, although i have to do more testing.
> 
> For testing, any sg device file can be passed to blktrace, e.g.:
> # blktrace -d /dev/sg1 -o - | blkparse -i -
> 
> I am seeking input in this approach: Is this approach worth pursuing
> to enable blktrace to trace SCSI tape drives? Would there be a better
> approach to get this trace data?

I think this approach is the simplest and right way to do it. Tracing is
really just tied to the "transport" (transport here meaning how we
transport commands to the device), and even character scsi devices use
the block layer queue for this operation, as you note.

Let me know when you are happy with the patch, and I'll queue it up for
2.6.25.
> @@ -1066,6 +1068,16 @@ sg_ioctl(struct inode *inode, struct fil
>  	case BLKSECTGET:
>  		return put_user(sdp->device->request_queue->max_sectors * 512,
>  				ip);
> +	case BLKTRACESETUP:
> +	{
> +		return blk_trace_setup(sdp->device->request_queue , sdp->device->sdev_gendev.bus_id, &sdp->device->sdev_gendev, arg);
> +	}

Don't need those braces, some other space and long line style issues as
well.

> --- a/include/linux/blkdev.h	2007-12-13 08:48:23.000000000 +0100
> +++ b/include/linux/blkdev.h	2007-12-13 08:48:25.000000000 +0100
> @@ -747,6 +747,16 @@ static inline void blkdev_dequeue_reques
>  	elv_dequeue_request(req->q, req);
>  }
>  
> +#ifdef CONFIG_BLK_DEV_IO_TRACE
> +extern int blk_trace_setup(request_queue_t *q,  char * name, dev_t dev, char __user *arg);
> +extern int blk_trace_startstop(request_queue_t *q, int start);
> +extern int blk_trace_remove(request_queue_t *q);
> +#else
> +#define blk_trace_setup(q, name, dev, arg) do { } while(0)
> +#define blk_trace_startstop(q, start) do { } while(0)
> +#define blk_trace_remove(q) do { } while(0)
> +#endif
> +

Put these in the blktrace include file.

-- 
Jens Axboe

Re: [RFC] blktrace interface for sg devices

[Christof Schmitt]

On Thu, Dec 13, 2007 at 10:19:42AM +0100, Jens Axboe wrote:
[...]
> I think this approach is the simplest and right way to do it. Tracing is
> really just tied to the "transport" (transport here meaning how we
> transport commands to the device), and even character scsi devices use
> the block layer queue for this operation, as you note.
> 
> Let me know when you are happy with the patch, and I'll queue it up for
> 2.6.25.
> > @@ -1066,6 +1068,16 @@ sg_ioctl(struct inode *inode, struct fil
> >  	case BLKSECTGET:
> >  		return put_user(sdp->device->request_queue->max_sectors * 512,
> >  				ip);
> > +	case BLKTRACESETUP:
> > +	{
> > +		return blk_trace_setup(sdp->device->request_queue , sdp->device->sdev_gendev.bus_id, &sdp->device->sdev_gendev, arg);
> > +	}
> 
> Don't need those braces, some other space and long line style issues as
> well.
> 
> > --- a/include/linux/blkdev.h	2007-12-13 08:48:23.000000000 +0100
> > +++ b/include/linux/blkdev.h	2007-12-13 08:48:25.000000000 +0100
> > @@ -747,6 +747,16 @@ static inline void blkdev_dequeue_reques
> >  	elv_dequeue_request(req->q, req);
> >  }
> >  
> > +#ifdef CONFIG_BLK_DEV_IO_TRACE
> > +extern int blk_trace_setup(request_queue_t *q,  char * name, dev_t dev, char __user *arg);
> > +extern int blk_trace_startstop(request_queue_t *q, int start);
> > +extern int blk_trace_remove(request_queue_t *q);
> > +#else
> > +#define blk_trace_setup(q, name, dev, arg) do { } while(0)
> > +#define blk_trace_startstop(q, start) do { } while(0)
> > +#define blk_trace_remove(q) do { } while(0)
> > +#endif
> > +
> 
> Put these in the blktrace include file.

Thanks for your input. I will prepare and send an updated version of
the patch. I also want to do some more testing, especially to see how
i can get the sizes of read and write requests and latencies for SCSI
tape drives.

Christof Schmitt

[PATCH] blktrace: Add blktrace ioctls to SCSI generic devices

[Christof Schmitt]

Since the SCSI layer uses the request queues from the block layer,
blktrace can also be used to trace the requests to all SCSI devices
(like SCSI tape drives), not only disks. The only missing part is the
ioctl interface to start and stop tracing.

This patch adds the SETUP, START, STOP and TEARDOWN ioctls from
blktrace to the sg device files. With this change, blktrace can be
used for SCSI devices like for disks, e.g.:
blktrace -d /dev/sg1 -o - | blkparse -i -

Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
---
 block/blktrace.c             |   24 ++++++++++++++----------
 block/compat_ioctl.c         |    5 ++++-
 drivers/scsi/sg.c            |   12 ++++++++++++
 include/linux/blktrace_api.h |   12 ++++++++++--
 4 files changed, 40 insertions(+), 13 deletions(-)

--- a/block/blktrace.c	2007-12-17 21:51:45.000000000 +0100
+++ b/block/blktrace.c	2007-12-17 21:51:45.000000000 +0100
@@ -236,7 +236,7 @@ static void blk_trace_cleanup(struct blk
 	kfree(bt);
 }
 
-static int blk_trace_remove(struct request_queue *q)
+int blk_trace_remove(struct request_queue *q)
 {
 	struct blk_trace *bt;
 
@@ -250,6 +250,7 @@ static int blk_trace_remove(struct reque
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(blk_trace_remove);
 
 static int blk_dropped_open(struct inode *inode, struct file *filp)
 {
@@ -317,18 +318,17 @@ static struct rchan_callbacks blk_relay_
 /*
  * Setup everything required to start tracing
  */
-int do_blk_trace_setup(struct request_queue *q, struct block_device *bdev,
+int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 			struct blk_user_trace_setup *buts)
 {
 	struct blk_trace *old_bt, *bt = NULL;
 	struct dentry *dir = NULL;
-	char b[BDEVNAME_SIZE];
 	int ret, i;
 
 	if (!buts->buf_size || !buts->buf_nr)
 		return -EINVAL;
 
-	strcpy(buts->name, bdevname(bdev, b));
+	strcpy(buts->name, name);
 
 	/*
 	 * some device names have larger paths - convert the slashes
@@ -353,7 +353,7 @@ int do_blk_trace_setup(struct request_qu
 		goto err;
 
 	bt->dir = dir;
-	bt->dev = bdev->bd_dev;
+	bt->dev = dev;
 	atomic_set(&bt->dropped, 0);
 
 	ret = -EIO;
@@ -400,8 +400,8 @@ err:
 	return ret;
 }
 
-static int blk_trace_setup(struct request_queue *q, struct block_device *bdev,
-			   char __user *arg)
+int blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
+		    char __user *arg)
 {
 	struct blk_user_trace_setup buts;
 	int ret;
@@ -410,7 +410,7 @@ static int blk_trace_setup(struct reques
 	if (ret)
 		return -EFAULT;
 
-	ret = do_blk_trace_setup(q, bdev, &buts);
+	ret = do_blk_trace_setup(q, name, dev, &buts);
 	if (ret)
 		return ret;
 
@@ -419,8 +419,9 @@ static int blk_trace_setup(struct reques
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(blk_trace_setup);
 
-static int blk_trace_startstop(struct request_queue *q, int start)
+int blk_trace_startstop(struct request_queue *q, int start)
 {
 	struct blk_trace *bt;
 	int ret;
@@ -453,6 +454,7 @@ static int blk_trace_startstop(struct re
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(blk_trace_startstop);
 
 /**
  * blk_trace_ioctl: - handle the ioctls associated with tracing
@@ -465,6 +467,7 @@ int blk_trace_ioctl(struct block_device 
 {
 	struct request_queue *q;
 	int ret, start = 0;
+	char b[BDEVNAME_SIZE];
 
 	q = bdev_get_queue(bdev);
 	if (!q)
@@ -474,7 +477,8 @@ int blk_trace_ioctl(struct block_device 
 
 	switch (cmd) {
 	case BLKTRACESETUP:
-		ret = blk_trace_setup(q, bdev, arg);
+		strcpy(b, bdevname(bdev, b));
+		ret = blk_trace_setup(q, b, bdev->bd_dev, arg);
 		break;
 	case BLKTRACESTART:
 		start = 1;
--- a/drivers/scsi/sg.c	2007-12-17 21:51:45.000000000 +0100
+++ b/drivers/scsi/sg.c	2007-12-17 22:36:30.000000000 +0100
@@ -48,6 +48,7 @@ static int sg_version_num = 30534;	/* 2 
 #include <linux/blkdev.h>
 #include <linux/delay.h>
 #include <linux/scatterlist.h>
+#include <linux/blktrace_api.h>
 
 #include "scsi.h"
 #include <scsi/scsi_dbg.h>
@@ -1063,6 +1064,17 @@ sg_ioctl(struct inode *inode, struct fil
 	case BLKSECTGET:
 		return put_user(sdp->device->request_queue->max_sectors * 512,
 				ip);
+	case BLKTRACESETUP:
+		return blk_trace_setup(sdp->device->request_queue,
+				       sdp->disk->disk_name,
+				       sdp->device->sdev_gendev.devt,
+				       (char *)arg);
+	case BLKTRACESTART:
+		return blk_trace_startstop(sdp->device->request_queue, 1);
+	case BLKTRACESTOP:
+		return blk_trace_startstop(sdp->device->request_queue, 0);
+	case BLKTRACETEARDOWN:
+		return blk_trace_remove(sdp->device->request_queue);
 	default:
 		if (read_only)
 			return -EPERM;	/* don't know so take safe approach */
--- a/include/linux/blktrace_api.h	2007-12-17 21:51:45.000000000 +0100
+++ b/include/linux/blktrace_api.h	2007-12-17 21:53:59.000000000 +0100
@@ -148,7 +148,7 @@ extern int blk_trace_ioctl(struct block_
 extern void blk_trace_shutdown(struct request_queue *);
 extern void __blk_add_trace(struct blk_trace *, sector_t, int, int, u32, int, int, void *);
 extern int do_blk_trace_setup(struct request_queue *q,
-	struct block_device *bdev, struct blk_user_trace_setup *buts);
+	char *name, dev_t dev, struct blk_user_trace_setup *buts);
 
 
 /**
@@ -282,6 +282,11 @@ static inline void blk_add_trace_remap(s
 	__blk_add_trace(bt, from, bio->bi_size, bio->bi_rw, BLK_TA_REMAP, !bio_flagged(bio, BIO_UPTODATE), sizeof(r), &r);
 }
 
+extern int blk_trace_setup(request_queue_t *q, char *name, dev_t dev,
+			   char __user *arg);
+extern int blk_trace_startstop(request_queue_t *q, int start);
+extern int blk_trace_remove(request_queue_t *q);
+
 #else /* !CONFIG_BLK_DEV_IO_TRACE */
 #define blk_trace_ioctl(bdev, cmd, arg)		(-ENOTTY)
 #define blk_trace_shutdown(q)			do { } while (0)
@@ -290,7 +295,10 @@ static inline void blk_add_trace_remap(s
 #define blk_add_trace_generic(q, rq, rw, what)	do { } while (0)
 #define blk_add_trace_pdu_int(q, what, bio, pdu)	do { } while (0)
 #define blk_add_trace_remap(q, bio, dev, f, t)	do {} while (0)
-#define do_blk_trace_setup(q, bdev, buts)	(-ENOTTY)
+#define do_blk_trace_setup(q, name, dev, buts)	(-ENOTTY)
+#define blk_trace_setup(q, name, dev, arg)	(-ENOTTY)
+#define blk_trace_startstop(q, start)		(-ENOTTY)
+#define blk_trace_remove(q)			(-ENOTTY)
 #endif /* CONFIG_BLK_DEV_IO_TRACE */
 #endif /* __KERNEL__ */
 #endif
--- a/block/compat_ioctl.c	2007-12-17 21:51:45.000000000 +0100
+++ b/block/compat_ioctl.c	2007-12-17 21:51:44.000000000 +0100
@@ -545,6 +545,7 @@ static int compat_blk_trace_setup(struct
 	struct blk_user_trace_setup buts;
 	struct compat_blk_user_trace_setup cbuts;
 	struct request_queue *q;
+	char b[BDEVNAME_SIZE];
 	int ret;
 
 	q = bdev_get_queue(bdev);
@@ -554,6 +555,8 @@ static int compat_blk_trace_setup(struct
 	if (copy_from_user(&cbuts, arg, sizeof(cbuts)))
 		return -EFAULT;
 
+	strcpy(b, bdevname(bdev, b));
+
 	buts = (struct blk_user_trace_setup) {
 		.act_mask = cbuts.act_mask,
 		.buf_size = cbuts.buf_size,
@@ -565,7 +568,7 @@ static int compat_blk_trace_setup(struct
 	memcpy(&buts.name, &cbuts.name, 32);
 
 	mutex_lock(&bdev->bd_mutex);
-	ret = do_blk_trace_setup(q, bdev, &buts);
+	ret = do_blk_trace_setup(q, b, bdev->bd_dev, &buts);
 	mutex_unlock(&bdev->bd_mutex);
 	if (ret)
 		return ret;