Re: [PATCH] blk-mq: Export queue state through /sys/kernel/debug/block/*/state

[Jens Axboe <axboe@fb.com>]

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.

-- 
Jens Axboe