blk-mq: Export queue state through /sys/kernel/debug/block/*/state
diff mbox

Message ID 1D08B61A9CF0974AA09887BE32D889DA12BA96@ULS-OP-MBXIP03.sdcorp.global.sandisk.com
State New
Headers show

Commit Message

Bart Van Assche March 29, 2017, 9:32 p.m. UTC
Make it possible to check whether or not a block layer queue has
been stopped. Make it possible to start and to run a blk-mq queue
from user space.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>

---

Changes compared to v1:
- Constified blk_queue_flag_name.
- Left out QUEUE_FLAG_VIRT because it is a synonym of QUEUE_FLAG_NONROT.
- Check array size before reading from blk_queue_flag_name[].
- Add functionality to restart a block layer queue.

---
 block/blk-mq-debugfs.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

Comments

Jens Axboe March 30, 2017, 3:27 p.m. UTC | #1
On 03/29/2017 03:32 PM, Bart Van Assche wrote:
> Make it possible to check whether or not a block layer queue has
> been stopped. Make it possible to start and to run a blk-mq queue
> from user space.

Looks good, minor nitpicking below.

> +static int blk_queue_flags_show(struct seq_file *m, void *v)
> +{
> +	struct request_queue *q = m->private;
> +	bool sep = false;
> +	int i;
> +
> +	for (i = 0; i < sizeof(q->queue_flags) * BITS_PER_BYTE; i++) {
> +		if (!(q->queue_flags & BIT(i)))
> +			continue;
> +		if (sep)
> +			seq_puts(m, " ");
> +		sep = true;

Put that sep = true in the else branch?

> +	if (strcmp(op, "run") == 0) {
> +		blk_mq_run_hw_queues(q, true);
> +	} else if (strcmp(op, "start") == 0) {
> +		blk_mq_start_stopped_hw_queues(q, true);
> +	} else {
> +		pr_err("%s: unsupported operation %s\n", __func__, op);
> +		return -EINVAL;
> +	}

You are inconsistent with your braces. I'd prefer no braces for single
lines.

Should the error case include valid commands? It'd be nice to have this
available, and not have to resort to looking at the source to figure out
what commands are available.
Bart Van Assche March 30, 2017, 6:10 p.m. UTC | #2
On Thu, 2017-03-30 at 09:27 -0600, Jens Axboe wrote:
> On 03/29/2017 03:32 PM, Bart Van Assche wrote:
> > Make it possible to check whether or not a block layer queue has
> > been stopped. Make it possible to start and to run a blk-mq queue
> > from user space.
> 
> Looks good, minor nitpicking below.
> 
> > +static int blk_queue_flags_show(struct seq_file *m, void *v)
> > +{
> > +	struct request_queue *q = m->private;
> > +	bool sep = false;
> > +	int i;
> > +
> > +	for (i = 0; i < sizeof(q->queue_flags) * BITS_PER_BYTE; i++) {
> > +		if (!(q->queue_flags & BIT(i)))
> > +			continue;
> > +		if (sep)
> > +			seq_puts(m, " ");
> > +		sep = true;
> 
> Put that sep = true in the else branch?

I can do that, but that would result in slightly less efficient assembler
code (additional jump) while stores can be pipelined easily.

> > +	if (strcmp(op, "run") == 0) {
> > +		blk_mq_run_hw_queues(q, true);
> > +	} else if (strcmp(op, "start") == 0) {
> > +		blk_mq_start_stopped_hw_queues(q, true);
> > +	} else {
> > +		pr_err("%s: unsupported operation %s\n", __func__, op);
> > +		return -EINVAL;
> > +	}
> 
> You are inconsistent with your braces. I'd prefer no braces for single
> lines.

Sorry but if braces are used for one side of an if-then-else statement then
checkpatch wants braces to be used for the other side too, even if that other
side is only a single line. From the checkpatch source code:

# check for single line unbalanced braces
                if ($sline =~ /^.\s*\}\s*else\s*$/ ||
                    $sline =~ /^.\s*else\s*\{\s*$/) {
                        CHK("BRACES", "Unbalanced braces around else statement\n" . $herecurr);
                }

> Should the error case include valid commands? It'd be nice to have this
> available, and not have to resort to looking at the source to figure out
> what commands are available.

OK, I will update the error message.

Thanks for the review.

Bart.
Jens Axboe March 30, 2017, 6:14 p.m. UTC | #3
On 03/30/2017 12:10 PM, Bart Van Assche wrote:
> On Thu, 2017-03-30 at 09:27 -0600, Jens Axboe wrote:
>> On 03/29/2017 03:32 PM, Bart Van Assche wrote:
>>> Make it possible to check whether or not a block layer queue has
>>> been stopped. Make it possible to start and to run a blk-mq queue
>>> from user space.
>>
>> Looks good, minor nitpicking below.
>>
>>> +static int blk_queue_flags_show(struct seq_file *m, void *v)
>>> +{
>>> +	struct request_queue *q = m->private;
>>> +	bool sep = false;
>>> +	int i;
>>> +
>>> +	for (i = 0; i < sizeof(q->queue_flags) * BITS_PER_BYTE; i++) {
>>> +		if (!(q->queue_flags & BIT(i)))
>>> +			continue;
>>> +		if (sep)
>>> +			seq_puts(m, " ");
>>> +		sep = true;
>>
>> Put that sep = true in the else branch?
> 
> I can do that, but that would result in slightly less efficient assembler
> code (additional jump) while stores can be pipelined easily.

It's a sysfs show function, it's not like we care about branching. But we
can leave it as-is, I don't feel that strongly about it at all.

>>> +	if (strcmp(op, "run") == 0) {
>>> +		blk_mq_run_hw_queues(q, true);
>>> +	} else if (strcmp(op, "start") == 0) {
>>> +		blk_mq_start_stopped_hw_queues(q, true);
>>> +	} else {
>>> +		pr_err("%s: unsupported operation %s\n", __func__, op);
>>> +		return -EINVAL;
>>> +	}
>>
>> You are inconsistent with your braces. I'd prefer no braces for single
>> lines.
> 
> Sorry but if braces are used for one side of an if-then-else statement then
> checkpatch wants braces to be used for the other side too, even if that other
> side is only a single line. From the checkpatch source code:

Omar informs me that it's coding style mandated. The block layer generally
does NOT do braces, so I'd prefer to keep it consistent at least. Again, not
really a huge problem, and as I prefaced the original email with, totally
nitpicking.

Patch
diff mbox

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 4b3f962a9c7a..f8b97d6306af 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -43,6 +43,100 @@  static int blk_mq_debugfs_seq_open(struct inode *inode, struct file *file,
 	return ret;
 }
 
+static const char *const blk_queue_flag_name[] = {
+	[QUEUE_FLAG_QUEUED]	 = "QUEUED",
+	[QUEUE_FLAG_STOPPED]	 = "STOPPED",
+	[QUEUE_FLAG_SYNCFULL]	 = "SYNCFULL",
+	[QUEUE_FLAG_ASYNCFULL]	 = "ASYNCFULL",
+	[QUEUE_FLAG_DYING]	 = "DYING",
+	[QUEUE_FLAG_BYPASS]	 = "BYPASS",
+	[QUEUE_FLAG_BIDI]	 = "BIDI",
+	[QUEUE_FLAG_NOMERGES]	 = "NOMERGES",
+	[QUEUE_FLAG_SAME_COMP]	 = "SAME_COMP",
+	[QUEUE_FLAG_FAIL_IO]	 = "FAIL_IO",
+	[QUEUE_FLAG_STACKABLE]	 = "STACKABLE",
+	[QUEUE_FLAG_NONROT]	 = "NONROT",
+	[QUEUE_FLAG_IO_STAT]	 = "IO_STAT",
+	[QUEUE_FLAG_DISCARD]	 = "DISCARD",
+	[QUEUE_FLAG_NOXMERGES]	 = "NOXMERGES",
+	[QUEUE_FLAG_ADD_RANDOM]	 = "ADD_RANDOM",
+	[QUEUE_FLAG_SECERASE]	 = "SECERASE",
+	[QUEUE_FLAG_SAME_FORCE]	 = "SAME_FORCE",
+	[QUEUE_FLAG_DEAD]	 = "DEAD",
+	[QUEUE_FLAG_INIT_DONE]	 = "INIT_DONE",
+	[QUEUE_FLAG_NO_SG_MERGE] = "NO_SG_MERGE",
+	[QUEUE_FLAG_POLL]	 = "POLL",
+	[QUEUE_FLAG_WC]		 = "WC",
+	[QUEUE_FLAG_FUA]	 = "FUA",
+	[QUEUE_FLAG_FLUSH_NQ]	 = "FLUSH_NQ",
+	[QUEUE_FLAG_DAX]	 = "DAX",
+	[QUEUE_FLAG_STATS]	 = "STATS",
+	[QUEUE_FLAG_RESTART]	 = "RESTART",
+	[QUEUE_FLAG_POLL_STATS]	 = "POLL_STATS",
+};
+
+static int blk_queue_flags_show(struct seq_file *m, void *v)
+{
+	struct request_queue *q = m->private;
+	bool sep = false;
+	int i;
+
+	for (i = 0; i < sizeof(q->queue_flags) * BITS_PER_BYTE; i++) {
+		if (!(q->queue_flags & BIT(i)))
+			continue;
+		if (sep)
+			seq_puts(m, " ");
+		sep = true;
+		if (i < ARRAY_SIZE(blk_queue_flag_name) &&
+		    blk_queue_flag_name[i])
+			seq_puts(m, blk_queue_flag_name[i]);
+		else
+			seq_printf(m, "%d", i);
+	}
+	seq_puts(m, "\n");
+	return 0;
+}
+
+static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf,
+				     size_t len, loff_t *offp)
+{
+	struct request_queue *q = file_inode(file)->i_private;
+	char op[16] = { }, *s;
+
+	len = min(len, sizeof(op) - 1);
+	if (copy_from_user(op, ubuf, len))
+		return -EFAULT;
+	s = op;
+	strsep(&s, " \t\n"); /* strip trailing whitespace */
+	if (strcmp(op, "run") == 0) {
+		blk_mq_run_hw_queues(q, true);
+	} else if (strcmp(op, "start") == 0) {
+		blk_mq_start_stopped_hw_queues(q, true);
+	} else {
+		pr_err("%s: unsupported operation %s\n", __func__, op);
+		return -EINVAL;
+	}
+	return len;
+}
+
+static int blk_queue_flags_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, blk_queue_flags_show, inode->i_private);
+}
+
+static const struct file_operations blk_queue_flags_fops = {
+	.open		= blk_queue_flags_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+	.write		= blk_queue_flags_store,
+};
+
+static const struct blk_mq_debugfs_attr blk_queue_attrs[] = {
+	{"state", 0600, &blk_queue_flags_fops},
+	{},
+};
+
 static void print_stat(struct seq_file *m, struct blk_rq_stat *stat)
 {
 	if (stat->nr_samples) {
@@ -735,6 +829,9 @@  int blk_mq_debugfs_register_hctxs(struct request_queue *q)
 	if (!q->debugfs_dir)
 		return -ENOENT;
 
+	if (!debugfs_create_files(q->debugfs_dir, q, blk_queue_attrs))
+		goto err;
+
 	q->mq_debugfs_dir = debugfs_create_dir("mq", q->debugfs_dir);
 	if (!q->mq_debugfs_dir)
 		goto err;