[v4,09/10] blk-mq: Add blk_mq_ops.show_rq()
diff mbox

Message ID 20170421234026.18970-10-bart.vanassche@sandisk.com
State New
Headers show

Commit Message

Bart Van Assche April 21, 2017, 11:40 p.m. UTC
This new callback function will be used in the next patch to show
more information about SCSI requests.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
---
 block/blk-mq-debugfs.c | 6 +++++-
 include/linux/blk-mq.h | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

Comments

Hannes Reinecke April 24, 2017, 7:32 a.m. UTC | #1
On 04/22/2017 01:40 AM, Bart Van Assche wrote:
> This new callback function will be used in the next patch to show
> more information about SCSI requests.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Reviewed-by: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-mq-debugfs.c | 6 +++++-
>  include/linux/blk-mq.h | 6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 3a99146ece39..c5eca9245459 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -311,6 +311,7 @@ static const char *const rqf_name[] = {
>  static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
>  {
>  	struct request *rq = list_entry_rq(v);
> +	const struct blk_mq_ops *const mq_ops = rq->q->mq_ops;
>  	const unsigned int op = rq->cmd_flags & REQ_OP_MASK;
>  
>  	seq_printf(m, "%p {.op=", rq);
> @@ -324,8 +325,11 @@ static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
>  	seq_puts(m, ", .rq_flags=");
>  	blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
>  		       ARRAY_SIZE(rqf_name));
> -	seq_printf(m, ", .tag=%d, .internal_tag=%d}\n", rq->tag,
> +	seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
>  		   rq->internal_tag);
> +	if (mq_ops->show_rq)
> +		mq_ops->show_rq(m, rq);
> +	seq_puts(m, "}\n");
>  	return 0;
>  }
>  
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 0c4dadb85f62..b7bf11c05568 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -121,6 +121,12 @@ struct blk_mq_ops {
>  	softirq_done_fn		*complete;
>  
>  	/*
> +	 * Used by the debugfs implementation to show driver-specific
> +	 * information about a request.
> +	 */
> +	void (*show_rq)(struct seq_file *m, struct request *rq);
> +
> +	/*
>  	 * Called when the block layer side of a hardware queue has been
>  	 * set up, allowing the driver to allocate/init matching structures.
>  	 * Ditto for exit/teardown.
> 
I don't really like this; what does happen if someone disabled
CONFIG_BLK_DEBUGFS?
Won't we end up with a stale callback?

Cheers,

Hannes
Bart Van Assche April 24, 2017, 9:51 p.m. UTC | #2
On Mon, 2017-04-24 at 09:32 +0200, Hannes Reinecke wrote:
> On 04/22/2017 01:40 AM, Bart Van Assche wrote:
> > --- a/include/linux/blk-mq.h
> > +++ b/include/linux/blk-mq.h
> > @@ -121,6 +121,12 @@ struct blk_mq_ops {
> >  	softirq_done_fn		*complete;
> >  
> >  	/*
> > +	 * Used by the debugfs implementation to show driver-specific
> > +	 * information about a request.
> > +	 */
> > +	void (*show_rq)(struct seq_file *m, struct request *rq);
> > +
> > +	/*
> >  	 * Called when the block layer side of a hardware queue has been
> >  	 * set up, allowing the driver to allocate/init matching structures.
> >  	 * Ditto for exit/teardown.
> > 
> 
> I don't really like this; what does happen if someone disabled
> CONFIG_BLK_DEBUGFS? Won't we end up with a stale callback?

Hello Hannes,

How about surrounding (*show_rq)() function pointer with #ifdef CONFIG_BLK_DEBUGFS /
#endif?

Bart.
Hannes Reinecke April 25, 2017, 3:16 p.m. UTC | #3
On 04/24/2017 11:51 PM, Bart Van Assche wrote:
> On Mon, 2017-04-24 at 09:32 +0200, Hannes Reinecke wrote:
>> On 04/22/2017 01:40 AM, Bart Van Assche wrote:
>>> --- a/include/linux/blk-mq.h
>>> +++ b/include/linux/blk-mq.h
>>> @@ -121,6 +121,12 @@ struct blk_mq_ops {
>>>  	softirq_done_fn		*complete;
>>>  
>>>  	/*
>>> +	 * Used by the debugfs implementation to show driver-specific
>>> +	 * information about a request.
>>> +	 */
>>> +	void (*show_rq)(struct seq_file *m, struct request *rq);
>>> +
>>> +	/*
>>>  	 * Called when the block layer side of a hardware queue has been
>>>  	 * set up, allowing the driver to allocate/init matching structures.
>>>  	 * Ditto for exit/teardown.
>>>
>>
>> I don't really like this; what does happen if someone disabled
>> CONFIG_BLK_DEBUGFS? Won't we end up with a stale callback?
> 
> Hello Hannes,
> 
> How about surrounding (*show_rq)() function pointer with #ifdef CONFIG_BLK_DEBUGFS /
> #endif?
> 
Nope.

Then you'll end up with different offsets in the structures, depending
on how the kernel is compiled. Making debugging a nightmare.

Not sure what would be the best way here ...

Cheers,

Hannes
Bart Van Assche April 25, 2017, 3:35 p.m. UTC | #4
On Tue, 2017-04-25 at 17:16 +0200, Hannes Reinecke wrote:
> On 04/24/2017 11:51 PM, Bart Van Assche wrote:
> > On Mon, 2017-04-24 at 09:32 +0200, Hannes Reinecke wrote:
> > > On 04/22/2017 01:40 AM, Bart Van Assche wrote:
> > > > --- a/include/linux/blk-mq.h
> > > > +++ b/include/linux/blk-mq.h
> > > > @@ -121,6 +121,12 @@ struct blk_mq_ops {
> > > >  	softirq_done_fn		*complete;
> > > >  
> > > >  	/*
> > > > +	 * Used by the debugfs implementation to show driver-specific
> > > > +	 * information about a request.
> > > > +	 */
> > > > +	void (*show_rq)(struct seq_file *m, struct request *rq);
> > > > +
> > > > +	/*
> > > >  	 * Called when the block layer side of a hardware queue has been
> > > >  	 * set up, allowing the driver to allocate/init matching structures.
> > > >  	 * Ditto for exit/teardown.
> > > > 
> > > 
> > > I don't really like this; what does happen if someone disabled
> > > CONFIG_BLK_DEBUGFS? Won't we end up with a stale callback?
> > 
> > How about surrounding (*show_rq)() function pointer with #ifdef CONFIG_BLK_DEBUGFS /
> > #endif?
> 
> Then you'll end up with different offsets in the structures, depending
> on how the kernel is compiled. Making debugging a nightmare.

Hello Hannes,

How about moving the .show_rq function pointer to the end such that the
offset of other members of struct blk_mq_ops does not depend on whether or
not CONFIG_BLK_DEBUGFS has been defined?

Bart.
Jens Axboe April 25, 2017, 4:34 p.m. UTC | #5
On 04/25/2017 08:16 AM, Hannes Reinecke wrote:
> On 04/24/2017 11:51 PM, Bart Van Assche wrote:
>> On Mon, 2017-04-24 at 09:32 +0200, Hannes Reinecke wrote:
>>> On 04/22/2017 01:40 AM, Bart Van Assche wrote:
>>>> --- a/include/linux/blk-mq.h
>>>> +++ b/include/linux/blk-mq.h
>>>> @@ -121,6 +121,12 @@ struct blk_mq_ops {
>>>>  	softirq_done_fn		*complete;
>>>>  
>>>>  	/*
>>>> +	 * Used by the debugfs implementation to show driver-specific
>>>> +	 * information about a request.
>>>> +	 */
>>>> +	void (*show_rq)(struct seq_file *m, struct request *rq);
>>>> +
>>>> +	/*
>>>>  	 * Called when the block layer side of a hardware queue has been
>>>>  	 * set up, allowing the driver to allocate/init matching structures.
>>>>  	 * Ditto for exit/teardown.
>>>>
>>>
>>> I don't really like this; what does happen if someone disabled
>>> CONFIG_BLK_DEBUGFS? Won't we end up with a stale callback?
>>
>> Hello Hannes,
>>
>> How about surrounding (*show_rq)() function pointer with #ifdef CONFIG_BLK_DEBUGFS /
>> #endif?
>>
> Nope.
> 
> Then you'll end up with different offsets in the structures, depending
> on how the kernel is compiled. Making debugging a nightmare.

That's nonsense, we deal with this all the time already.

Patch
diff mbox

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 3a99146ece39..c5eca9245459 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -311,6 +311,7 @@  static const char *const rqf_name[] = {
 static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
 {
 	struct request *rq = list_entry_rq(v);
+	const struct blk_mq_ops *const mq_ops = rq->q->mq_ops;
 	const unsigned int op = rq->cmd_flags & REQ_OP_MASK;
 
 	seq_printf(m, "%p {.op=", rq);
@@ -324,8 +325,11 @@  static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
 	seq_puts(m, ", .rq_flags=");
 	blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
 		       ARRAY_SIZE(rqf_name));
-	seq_printf(m, ", .tag=%d, .internal_tag=%d}\n", rq->tag,
+	seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
 		   rq->internal_tag);
+	if (mq_ops->show_rq)
+		mq_ops->show_rq(m, rq);
+	seq_puts(m, "}\n");
 	return 0;
 }
 
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 0c4dadb85f62..b7bf11c05568 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -121,6 +121,12 @@  struct blk_mq_ops {
 	softirq_done_fn		*complete;
 
 	/*
+	 * Used by the debugfs implementation to show driver-specific
+	 * information about a request.
+	 */
+	void (*show_rq)(struct seq_file *m, struct request *rq);
+
+	/*
 	 * Called when the block layer side of a hardware queue has been
 	 * set up, allowing the driver to allocate/init matching structures.
 	 * Ditto for exit/teardown.