Added in user-injected messages into blk traces

PATCH In review Alan D. Brunelle 2 participants 4 messages Jun 5, 2008 – Jun 6, 2008 Block layer

Share & tools

Alan D. Brunelle

author

2008-06-05 12:21 UTC #0 lore raw

PATCH 1/1 Added in user-injected messages into blk traces

This allows a user to annotate the blk trace stream: writing a suitable message to {/sys/kernel/debug}/block/<dsf>/msg will have it propagated into the trace stream.

Signed-off-by Alan D. Brunelle <[email protected]>

---
 block/blktrace.c             |   42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/blktrace_api.h |    1 +
 2 files changed, 43 insertions(+), 0 deletions(-)

block/blktrace.c +42 -0

index 38e6b83..b9c4df2 100644
@@ -253,6 +253,7 @@ err:
 static void blk_trace_cleanup(struct blk_trace *bt)
 {
 	relay_close(bt->rchan);
+	debugfs_remove(bt->msg_file);
 	debugfs_remove(bt->dropped_file);
 	blk_remove_tree(bt->dir);
 	free_percpu(bt->sequence);

@@ -299,6 +300,41 @@ static const struct file_operations blk_dropped_fops = {
 	.read =		blk_dropped_read,
 };
 
+static int blk_msg_open(struct inode *inode, struct file *filp)
+{
+	filp->private_data = inode->i_private;
+
+	return 0;
+}
+
+static ssize_t blk_msg_write(struct file *filp, const char __user *buffer,
+				size_t count, loff_t *ppos)
+{
+	ssize_t ret = count;
+
+	if (count > BLK_TN_MAX_MSG)
+		ret = -EINVAL;
+	else {
+		char *msg = kmalloc(BLK_TN_MAX_MSG, GFP_KERNEL);
+
+		if (copy_from_user(msg, buffer, count))
+			ret = -EFAULT;
+		else {
+			struct blk_trace *bt = filp->private_data;
+			__trace_note_message(bt, "%s", msg);
+		}
+		kfree(msg);
+	}
+
+	return ret;
+}
+
+static const struct file_operations blk_msg_fops = {
+	.owner =	THIS_MODULE,
+	.open =		blk_msg_open,
+	.write =	blk_msg_write,
+};
+
 /*
  * Keep track of how many times we encountered a full subbuffer, to aid
  * the user space app in telling how many lost events there were.

@@ -384,6 +420,10 @@ int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	if (!bt->dropped_file)
 		goto err;
 
+	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
+	if (!bt->msg_file)
+		goto err;
+
 	bt->rchan = relay_open("trace", dir, buts->buf_size,
 				buts->buf_nr, &blk_relay_callbacks, bt);
 	if (!bt->rchan)

@@ -413,6 +453,8 @@ err:
 	if (dir)
 		blk_remove_tree(dir);
 	if (bt) {
+		if (bt->msg_file)
+			debugfs_remove(bt->msg_file);
 		if (bt->dropped_file)
 			debugfs_remove(bt->dropped_file);
 		free_percpu(bt->sequence);

include/linux/blktrace_api.h +1 -0

index 3228caa..41208f1 100644
@@ -128,6 +128,7 @@ struct blk_trace {
 	u32 dev;
 	struct dentry *dir;
 	struct dentry *dropped_file;
+	struct dentry *msg_file;
 	atomic_t dropped;
 };

Signature · 1 line

1.5.4.3

Jens Axboe

2008-06-06 09:24 UTC #1 lore raw

Quoted 3 lines · Alan D. Brunelle

This allows a user to annotate the blk trace stream: writing a suitable message to {/sys/kernel/debug}/block/<dsf>/msg will have it propagated into the trace stream.

Looks good to me, I think this can be useful for ease of trace annotation. Comments below.

Quoted 36 lines

Signed-off-by Alan D. Brunelle <[email protected]>

---
 block/blktrace.c             |   42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/blktrace_api.h |    1 +
 2 files changed, 43 insertions(+), 0 deletions(-)

block/blktrace.c +15 -0

index 38e6b83..b9c4df2 100644
@@ -253,6 +253,7 @@ err:
 static void blk_trace_cleanup(struct blk_trace *bt)
 {
 	relay_close(bt->rchan);
+	debugfs_remove(bt->msg_file);
 	debugfs_remove(bt->dropped_file);
 	blk_remove_tree(bt->dir);
 	free_percpu(bt->sequence);

@@ -299,6 +300,41 @@ static const struct file_operations blk_dropped_fops = {
 	.read =		blk_dropped_read,
 };
 
+static int blk_msg_open(struct inode *inode, struct file *filp)
+{
+	filp->private_data = inode->i_private;
+
+	return 0;
+}
+
+static ssize_t blk_msg_write(struct file *filp, const char __user *buffer,
+				size_t count, loff_t *ppos)
+{
+	ssize_t ret = count;
+
+	if (count > BLK_TN_MAX_MSG)
+		ret = -EINVAL;

block/blktrace.c static const struct file_operations blk_dropped_fops = { · line 326

Quoted 10 lines

block/blktrace.c +10 -0

+	else {
+		char *msg = kmalloc(BLK_TN_MAX_MSG, GFP_KERNEL);
+
+		if (copy_from_user(msg, buffer, count))
+			ret = -EFAULT;
+		else {
+			struct blk_trace *bt = filp->private_data;
+			__trace_note_message(bt, "%s", msg);
+		}
+		kfree(msg);

Would be cleaner with an explicit !msg check, though I think copy_from_user() will notice and it'll work as-is.

if (count > BLK_TN_MAX_MSG)

        return -EINVAL;

msg = kmalloc(BLK_TN_MAX_MSG, GFP_KERNEL);
if (!msg)

        return -ENOMEM;

if (copy_from_user(msg, buffer, count))

        return -EFAULT;

...

is just a lot easier to directly follow imho. Otherwise I have no problems with it, if you fix that up we can queue it for 2.6.27 inclusion.

Signature · 1 line

Jens Axboe

Alan D. Brunelle

author

2008-06-06 10:46 UTC #2 lore raw

Quoted 66 lines · Jens Axboe

Quoted 3 lines · Alan D. Brunelle
This allows a user to annotate the blk trace stream: writing a suitable message to {/sys/kernel/debug}/block/<dsf>/msg will have it propagated into the trace stream.
Looks good to me, I think this can be useful for ease of trace annotation. Comments below.
Quoted 36 lines
Signed-off-by Alan D. Brunelle <[email protected]>
---
 block/blktrace.c             |   42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/blktrace_api.h |    1 +
 2 files changed, 43 insertions(+), 0 deletions(-)
block/blktrace.c +15 -0
index 38e6b83..b9c4df2 100644
@@ -253,6 +253,7 @@ err:
 static void blk_trace_cleanup(struct blk_trace *bt)
 {
 	relay_close(bt->rchan);
+	debugfs_remove(bt->msg_file);
 	debugfs_remove(bt->dropped_file);
 	blk_remove_tree(bt->dir);
 	free_percpu(bt->sequence);
@@ -299,6 +300,41 @@ static const struct file_operations blk_dropped_fops = {
 	.read =		blk_dropped_read,
 };
 
+static int blk_msg_open(struct inode *inode, struct file *filp)
+{
+	filp->private_data = inode->i_private;
+
+	return 0;
+}
+
+static ssize_t blk_msg_write(struct file *filp, const char __user *buffer,
+				size_t count, loff_t *ppos)
+{
+	ssize_t ret = count;
+
+	if (count > BLK_TN_MAX_MSG)
+		ret = -EINVAL;
block/blktrace.c static const struct file_operations blk_dropped_fops = { · line 326
Quoted 10 lines
block/blktrace.c +10 -0
+	else {
+		char *msg = kmalloc(BLK_TN_MAX_MSG, GFP_KERNEL);
+
+		if (copy_from_user(msg, buffer, count))
+			ret = -EFAULT;
+		else {
+			struct blk_trace *bt = filp->private_data;
+			__trace_note_message(bt, "%s", msg);
+		}
+		kfree(msg);
Would be cleaner with an explicit !msg check, though I think copy_from_user() will notice and it'll work as-is.
if (count > BLK_TN_MAX_MSG)
        return -EINVAL;
msg = kmalloc(BLK_TN_MAX_MSG, GFP_KERNEL);
if (!msg)
        return -ENOMEM;

Quoted 9 lines

if (copy_from_user(msg, buffer, count))
        return -EFAULT;
...
is just a lot easier to directly follow imho. Otherwise I have no problems with it, if you fix that up we can queue it for 2.6.27 inclusion.

Good points, will clean up and resubmit.

Thanks,
Alan

Alan D. Brunelle

author

2008-06-06 12:24 UTC #3 lore raw

PATCH 1/1 Added in user-injected messages into blk traces

In reply to Jens Axboe

This allows a user to annotate the blk trace stream: writing a suitable message to {/sys/kernel/debug}/block/<dsf>/msg will have it propagated into the trace stream.

Signed-off-by Alan D. Brunelle <[email protected]>

---
 block/blktrace.c             |   45 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/blktrace_api.h |    1 +
 2 files changed, 46 insertions(+), 0 deletions(-)

block/blktrace.c +45 -0

index 7ae87cc..63ed9cf 100644
@@ -246,6 +246,7 @@ err:
 static void blk_trace_cleanup(struct blk_trace *bt)
 {
 	relay_close(bt->rchan);
+	debugfs_remove(bt->msg_file);
 	debugfs_remove(bt->dropped_file);
 	blk_remove_tree(bt->dir);
 	free_percpu(bt->sequence);

@@ -293,6 +294,44 @@ static const struct file_operations blk_dropped_fops = {
 	.read =		blk_dropped_read,
 };
 
+static int blk_msg_open(struct inode *inode, struct file *filp)
+{
+	filp->private_data = inode->i_private;
+
+	return 0;
+}
+
+static ssize_t blk_msg_write(struct file *filp, const char __user *buffer,
+				size_t count, loff_t *ppos)
+{
+	char *msg;
+	struct blk_trace *bt;
+
+	if (count > BLK_TN_MAX_MSG)
+		return -EINVAL;
+
+	msg = kmalloc(count, GFP_KERNEL);
+	if (msg == NULL)
+		return -ENOMEM;
+
+	if (copy_from_user(msg, buffer, count)) {
+		kfree(msg);
+		return -EFAULT;
+	}
+
+	bt = filp->private_data;
+	__trace_note_message(bt, "%s", msg);
+	kfree(msg);
+
+	return count;
+}
+
+static const struct file_operations blk_msg_fops = {
+	.owner =	THIS_MODULE,
+	.open =		blk_msg_open,
+	.write =	blk_msg_write,
+};
+
 /*
  * Keep track of how many times we encountered a full subbuffer, to aid
  * the user space app in telling how many lost events there were.

@@ -382,6 +421,10 @@ int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	if (!bt->dropped_file)
 		goto err;
 
+	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
+	if (!bt->msg_file)
+		goto err;
+
 	bt->rchan = relay_open("trace", dir, buts->buf_size,
 				buts->buf_nr, &blk_relay_callbacks, bt);
 	if (!bt->rchan)

@@ -411,6 +454,8 @@ err:
 	if (dir)
 		blk_remove_tree(dir);
 	if (bt) {
+		if (bt->msg_file)
+			debugfs_remove(bt->msg_file);
 		if (bt->dropped_file)
 			debugfs_remove(bt->dropped_file);
 		free_percpu(bt->sequence);

include/linux/blktrace_api.h +1 -0

index e3ef903..d084b8d 100644
@@ -129,6 +129,7 @@ struct blk_trace {
 	u32 dev;
 	struct dentry *dir;
 	struct dentry *dropped_file;
+	struct dentry *msg_file;
 	atomic_t dropped;
 };

Signature · 1 line

1.5.4.3