| Message ID | 1D08B61A9CF0974AA09887BE32D889DA12BA96@ULS-OP-MBXIP03.sdcorp.global.sandisk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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.
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;
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(+)