From: Bart Van Assche <Bart.VanAssche@sandisk.com>
To: "axboe@fb.com" <axboe@fb.com>
Cc: "osandov@fb.com" <osandov@fb.com>, "hare@suse.de" <hare@suse.de>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>
Subject: Re: [PATCH] blk-mq: Export queue state through /sys/kernel/debug/block/*/state
Date: Thu, 30 Mar 2017 18:10:57 +0000 [thread overview]
Message-ID: <1490897421.2753.12.camel@sandisk.com> (raw)
In-Reply-To: <f80153aa-3e7d-0a20-e426-7e58cd7c249d@fb.com>
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.
>=20
> Looks good, minor nitpicking below.
>=20
> > +static int blk_queue_flags_show(struct seq_file *m, void *v)
> > +{
> > + struct request_queue *q =3D m->private;
> > + bool sep =3D false;
> > + int i;
> > +
> > + for (i =3D 0; i < sizeof(q->queue_flags) * BITS_PER_BYTE; i++) {
> > + if (!(q->queue_flags & BIT(i)))
> > + continue;
> > + if (sep)
> > + seq_puts(m, " ");
> > + sep =3D true;
>=20
> Put that sep =3D 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") =3D=3D 0) {
> > + blk_mq_run_hw_queues(q, true);
> > + } else if (strcmp(op, "start") =3D=3D 0) {
> > + blk_mq_start_stopped_hw_queues(q, true);
> > + } else {
> > + pr_err("%s: unsupported operation %s\n", __func__, op);
> > + return -EINVAL;
> > + }
>=20
> 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 oth=
er
side is only a single line. From the checkpatch source code:
# check for single line unbalanced braces
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0if ($sline =3D~ /^.\s*\}\s*=
else\s*$/ ||
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0$sline =3D~ /^.=
\s*else\s*\{\s*$/) {
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0CHK=
("BRACES", "Unbalanced braces around else statement\n" . $herecurr);
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0}
> 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.=
next prev parent reply other threads:[~2017-03-30 18:11 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-29 21:32 [PATCH] blk-mq: Export queue state through /sys/kernel/debug/block/*/state Bart Van Assche
2017-03-30 15:27 ` Jens Axboe
2017-03-30 18:10 ` Bart Van Assche [this message]
2017-03-30 18:14 ` Jens Axboe
-- strict thread matches above, loose matches on Subject: below --
2017-03-29 20:20 Bart Van Assche
2017-03-29 20:31 ` Jens Axboe
2017-03-30 5:50 ` Hannes Reinecke
2017-03-30 15:16 ` Bart Van Assche
2017-03-30 15:19 ` Jens Axboe
2017-03-30 15:23 ` Bart Van Assche
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1490897421.2753.12.camel@sandisk.com \
--to=bart.vanassche@sandisk.com \
--cc=axboe@fb.com \
--cc=hare@suse.de \
--cc=linux-block@vger.kernel.org \
--cc=osandov@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).