Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-10-08 Thread Johannes Thumshirn
On Fri, Oct 05, 2018 at 07:13:32PM -0600, Jens Axboe wrote:
> On 10/5/18 4:37 PM, Omar Sandoval wrote:
> > On Fri, Oct 05, 2018 at 08:18:00AM -0600, Jens Axboe wrote:
> >> On 10/4/18 11:35 AM, Bart Van Assche wrote:
> >>> When debugging e.g. the SCSI timeout handler it is important that
> >>> requests that have not yet been started or that already have
> >>> completed are also reported through debugfs.
> >>
> >> Thanks, I like this better - applied. BTW, what's up with the
> >> reverse ordering on this:
> >>
> >>> Signed-off-by: Bart Van Assche 
> >>> Cc: Christoph Hellwig 
> >>> Cc: Ming Lei 
> >>> Cc: Hannes Reinecke 
> >>> Cc: Johannes Thumshirn 
> >>> Cc: Martin K. Petersen 
> >>
> >> For some reason that really annoys me, and I see it in various
> >> patches these days. IMHO the SOB should be last, with whatever
> >> acks, reviews, CC, before that.
> > 
> > I could've sworn that this guideline was even documented somewhere, but
> > I can't find it now ¯\_(ツ)_/¯
> 
> My guess is that it's some newer git thing - but if it is, it's really
> annoying and should be reverted. I end up fixing these up by hand.

Isn't it the way patchwork adds the tags? And yes I also thought this is the
correct way of adding the Ccs, etc..

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850


Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-10-05 Thread Jens Axboe
On 10/5/18 4:37 PM, Omar Sandoval wrote:
> On Fri, Oct 05, 2018 at 08:18:00AM -0600, Jens Axboe wrote:
>> On 10/4/18 11:35 AM, Bart Van Assche wrote:
>>> When debugging e.g. the SCSI timeout handler it is important that
>>> requests that have not yet been started or that already have
>>> completed are also reported through debugfs.
>>
>> Thanks, I like this better - applied. BTW, what's up with the
>> reverse ordering on this:
>>
>>> Signed-off-by: Bart Van Assche 
>>> Cc: Christoph Hellwig 
>>> Cc: Ming Lei 
>>> Cc: Hannes Reinecke 
>>> Cc: Johannes Thumshirn 
>>> Cc: Martin K. Petersen 
>>
>> For some reason that really annoys me, and I see it in various
>> patches these days. IMHO the SOB should be last, with whatever
>> acks, reviews, CC, before that.
> 
> I could've sworn that this guideline was even documented somewhere, but
> I can't find it now ¯\_(ツ)_/¯

My guess is that it's some newer git thing - but if it is, it's really
annoying and should be reverted. I end up fixing these up by hand.

-- 
Jens Axboe



Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-10-05 Thread Omar Sandoval
On Fri, Oct 05, 2018 at 08:18:00AM -0600, Jens Axboe wrote:
> On 10/4/18 11:35 AM, Bart Van Assche wrote:
> > When debugging e.g. the SCSI timeout handler it is important that
> > requests that have not yet been started or that already have
> > completed are also reported through debugfs.
> 
> Thanks, I like this better - applied. BTW, what's up with the
> reverse ordering on this:
> 
> > Signed-off-by: Bart Van Assche 
> > Cc: Christoph Hellwig 
> > Cc: Ming Lei 
> > Cc: Hannes Reinecke 
> > Cc: Johannes Thumshirn 
> > Cc: Martin K. Petersen 
> 
> For some reason that really annoys me, and I see it in various
> patches these days. IMHO the SOB should be last, with whatever
> acks, reviews, CC, before that.

I could've sworn that this guideline was even documented somewhere, but
I can't find it now ¯\_(ツ)_/¯


Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-10-05 Thread Jens Axboe
On 10/4/18 11:35 AM, Bart Van Assche wrote:
> When debugging e.g. the SCSI timeout handler it is important that
> requests that have not yet been started or that already have
> completed are also reported through debugfs.

Thanks, I like this better - applied. BTW, what's up with the
reverse ordering on this:

> Signed-off-by: Bart Van Assche 
> Cc: Christoph Hellwig 
> Cc: Ming Lei 
> Cc: Hannes Reinecke 
> Cc: Johannes Thumshirn 
> Cc: Martin K. Petersen 

For some reason that really annoys me, and I see it in various
patches these days. IMHO the SOB should be last, with whatever
acks, reviews, CC, before that.

-- 
Jens Axboe



Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-10-05 Thread Johannes Thumshirn
Looks good,
Reviewed-by: Johannes Thumshirn 
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850


Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-10-04 Thread Hannes Reinecke

On 10/4/18 7:35 PM, Bart Van Assche wrote:

When debugging e.g. the SCSI timeout handler it is important that
requests that have not yet been started or that already have
completed are also reported through debugfs.

Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
Cc: Ming Lei 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Martin K. Petersen 
---
  block/blk-mq-debugfs.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index a5ea86835fcb..41b86f50d126 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -431,8 +431,7 @@ static void hctx_show_busy_rq(struct request *rq, void 
*data, bool reserved)
  {
const struct show_busy_params *params = data;
  
-	if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx &&

-   blk_mq_rq_state(rq) != MQ_RQ_IDLE)
+   if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx)
__blk_mq_debugfs_rq_show(params->m,
 list_entry_rq(>queuelist));
  }


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)


[PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-10-04 Thread Bart Van Assche
When debugging e.g. the SCSI timeout handler it is important that
requests that have not yet been started or that already have
completed are also reported through debugfs.

Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
Cc: Ming Lei 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Martin K. Petersen 
---
 block/blk-mq-debugfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index a5ea86835fcb..41b86f50d126 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -431,8 +431,7 @@ static void hctx_show_busy_rq(struct request *rq, void 
*data, bool reserved)
 {
const struct show_busy_params *params = data;
 
-   if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx &&
-   blk_mq_rq_state(rq) != MQ_RQ_IDLE)
+   if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx)
__blk_mq_debugfs_rq_show(params->m,
 list_entry_rq(>queuelist));
 }
-- 
2.19.0.605.g01d371f741-goog



Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-10-03 Thread Jens Axboe
On 10/3/18 4:51 PM, Bart Van Assche wrote:
> On Wed, 2018-10-03 at 16:12 -0600, Jens Axboe wrote:
>> On 10/3/18 3:42 PM, Bart Van Assche wrote:
>>> On Fri, 2018-01-12 at 22:11 +, Bart Van Assche wrote:
  /*
 + * Show "busy" requests - these are the requests owned by the block 
 driver.
 + * The test list_empty(>queuelist) is used to figure out whether or 
 not
 + * a request is owned by the block driver. That test works because the 
 block
 + * layer core uses list_del_init() consistently to remove a request from 
 one
 + * of the request lists.
 + *
   * Note: the state of a request may change while this function is in 
 progress,
   * e.g. due to a concurrent blk_mq_finish_request() call.
   */
 @@ -402,7 +408,7 @@ static void hctx_show_busy_rq(struct request *rq, void 
 *data, bool reserved)
const struct show_busy_params *params = data;
  
if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx &&
 -  blk_mq_rq_state(rq) != MQ_RQ_IDLE)
 +  list_empty(>queuelist))
__blk_mq_debugfs_rq_show(params->m,
 list_entry_rq(>queuelist));
  }
>>>
>>> Hello Jens,
>>>
>>> Can you share your opinion about the above patch?
>>
>> I just convince myself that the list check is super useful. The request
>> could be on any number of lists, either not yet seen by the driver, or
>> maybe sitting in dispatch. You're only going to be finding these
>> requests if the tag is allocated, which means that it's already in some
>> sort of non-idle state.
>>
>> So enlighten me why we need the list check at all? Wouldn't it be better
>> to simply remove it, and ensure that the debug print includes things
>> like list state, rq state, etc?
> 
> Hello Jens,
> 
> I have tried to leave the list_empty() check out but if I do that then
> requests that have the state "idle" (allocated but not yet started) also
> show up. I think these should be left out from the output produced by
> reading the "busy" attribute because such requests are not interesting
> when analyzing an I/O lockup:
> 
> nullb0/hctx1/busy:abe67123 {.op=READ, .cmd_flags=, 
> .rq_flags=IO_STAT|STATS, .s
> tate=idle, .tag=63, .internal_tag=-1}

They might be, if we have allocated tags that aren't going anywhere.
Like in a tag leak, or something. Besides, requests should exist in that
state very shortly, so it's not like it should cloud the output that
much.

-- 
Jens Axboe



Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-10-03 Thread Bart Van Assche
On Wed, 2018-10-03 at 16:12 -0600, Jens Axboe wrote:
> On 10/3/18 3:42 PM, Bart Van Assche wrote:
> > On Fri, 2018-01-12 at 22:11 +, Bart Van Assche wrote:
> > >  /*
> > > + * Show "busy" requests - these are the requests owned by the block 
> > > driver.
> > > + * The test list_empty(>queuelist) is used to figure out whether or 
> > > not
> > > + * a request is owned by the block driver. That test works because the 
> > > block
> > > + * layer core uses list_del_init() consistently to remove a request from 
> > > one
> > > + * of the request lists.
> > > + *
> > >   * Note: the state of a request may change while this function is in 
> > > progress,
> > >   * e.g. due to a concurrent blk_mq_finish_request() call.
> > >   */
> > > @@ -402,7 +408,7 @@ static void hctx_show_busy_rq(struct request *rq, 
> > > void *data, bool reserved)
> > >   const struct show_busy_params *params = data;
> > >  
> > >   if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx &&
> > > - blk_mq_rq_state(rq) != MQ_RQ_IDLE)
> > > + list_empty(>queuelist))
> > >   __blk_mq_debugfs_rq_show(params->m,
> > >list_entry_rq(>queuelist));
> > >  }
> > 
> > Hello Jens,
> > 
> > Can you share your opinion about the above patch?
> 
> I just convince myself that the list check is super useful. The request
> could be on any number of lists, either not yet seen by the driver, or
> maybe sitting in dispatch. You're only going to be finding these
> requests if the tag is allocated, which means that it's already in some
> sort of non-idle state.
> 
> So enlighten me why we need the list check at all? Wouldn't it be better
> to simply remove it, and ensure that the debug print includes things
> like list state, rq state, etc?

Hello Jens,

I have tried to leave the list_empty() check out but if I do that then
requests that have the state "idle" (allocated but not yet started) also
show up. I think these should be left out from the output produced by
reading the "busy" attribute because such requests are not interesting
when analyzing an I/O lockup:

nullb0/hctx1/busy:abe67123 {.op=READ, .cmd_flags=, 
.rq_flags=IO_STAT|STATS, .s
tate=idle, .tag=63, .internal_tag=-1}

Thanks,

Bart.


Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-10-03 Thread Jens Axboe
On 10/3/18 3:42 PM, Bart Van Assche wrote:
> On Fri, 2018-01-12 at 22:11 +, Bart Van Assche wrote:
>> On Fri, 2018-01-12 at 15:05 -0700, Jens Axboe wrote:
>>> On 1/12/18 3:00 PM, Bart Van Assche wrote:
 On Fri, 2018-01-12 at 14:55 -0700, Jens Axboe wrote:
> On 1/12/18 2:52 PM, Bart Van Assche wrote:
>> When debugging e.g. the SCSI timeout handler it is important that
>> requests that have not yet been started or that already have
>> completed are also reported through debugfs.
>>
>> This patch depends on a patch that went upstream recently, namely
>> commit 14e3062fb185 ("scsi: core: Fix a scsi_show_rq() NULL pointer
>> dereference").
>
> Why don't we just kill the check, and dump any request that has a
> matching hctx? We already know the bit was set, so just print
> all of them.

 It is very helpful during debugging that requests owned by a block driver 
 and
 requests owned by the block layer core show up in different debugfs files.
 Removing the check completely would cause all requests to show up in the 
 same
 debugfs file and would make interpreting the contents of these debugfs 
 files
 much harder.
>>>
>>> Yeah, we'd have to make it just one file at that point. I'm not hugely
>>> against the queuelist check, but probably warrants a comment as it's not
>>> immediately clear (as opposed to the idle check, or the previous START
>>> bit check).
>>
>> How about the below?
>>
>> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
>> index 19db3f583bf1..da859dac442b 100644
>> --- a/block/blk-mq-debugfs.c
>> +++ b/block/blk-mq-debugfs.c
>> @@ -394,6 +394,12 @@ struct show_busy_params {
>>  };
>>  
>>  /*
>> + * Show "busy" requests - these are the requests owned by the block driver.
>> + * The test list_empty(>queuelist) is used to figure out whether or not
>> + * a request is owned by the block driver. That test works because the block
>> + * layer core uses list_del_init() consistently to remove a request from one
>> + * of the request lists.
>> + *
>>   * Note: the state of a request may change while this function is in 
>> progress,
>>   * e.g. due to a concurrent blk_mq_finish_request() call.
>>   */
>> @@ -402,7 +408,7 @@ static void hctx_show_busy_rq(struct request *rq, void 
>> *data, bool reserved)
>>  const struct show_busy_params *params = data;
>>  
>>  if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx &&
>> -blk_mq_rq_state(rq) != MQ_RQ_IDLE)
>> +list_empty(>queuelist))
>>  __blk_mq_debugfs_rq_show(params->m,
>>   list_entry_rq(>queuelist));
>>  }
> 
> Hello Jens,
> 
> Can you share your opinion about the above patch?

I just convince myself that the list check is super useful. The request
could be on any number of lists, either not yet seen by the driver, or
maybe sitting in dispatch. You're only going to be finding these
requests if the tag is allocated, which means that it's already in some
sort of non-idle state.

So enlighten me why we need the list check at all? Wouldn't it be better
to simply remove it, and ensure that the debug print includes things
like list state, rq state, etc?

-- 
Jens Axboe



Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-10-03 Thread Bart Van Assche
On Fri, 2018-01-12 at 22:11 +, Bart Van Assche wrote:
> On Fri, 2018-01-12 at 15:05 -0700, Jens Axboe wrote:
> > On 1/12/18 3:00 PM, Bart Van Assche wrote:
> > > On Fri, 2018-01-12 at 14:55 -0700, Jens Axboe wrote:
> > > > On 1/12/18 2:52 PM, Bart Van Assche wrote:
> > > > > When debugging e.g. the SCSI timeout handler it is important that
> > > > > requests that have not yet been started or that already have
> > > > > completed are also reported through debugfs.
> > > > > 
> > > > > This patch depends on a patch that went upstream recently, namely
> > > > > commit 14e3062fb185 ("scsi: core: Fix a scsi_show_rq() NULL pointer
> > > > > dereference").
> > > > 
> > > > Why don't we just kill the check, and dump any request that has a
> > > > matching hctx? We already know the bit was set, so just print
> > > > all of them.
> > > 
> > > It is very helpful during debugging that requests owned by a block driver 
> > > and
> > > requests owned by the block layer core show up in different debugfs files.
> > > Removing the check completely would cause all requests to show up in the 
> > > same
> > > debugfs file and would make interpreting the contents of these debugfs 
> > > files
> > > much harder.
> > 
> > Yeah, we'd have to make it just one file at that point. I'm not hugely
> > against the queuelist check, but probably warrants a comment as it's not
> > immediately clear (as opposed to the idle check, or the previous START
> > bit check).
> 
> How about the below?
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 19db3f583bf1..da859dac442b 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -394,6 +394,12 @@ struct show_busy_params {
>  };
>  
>  /*
> + * Show "busy" requests - these are the requests owned by the block driver.
> + * The test list_empty(>queuelist) is used to figure out whether or not
> + * a request is owned by the block driver. That test works because the block
> + * layer core uses list_del_init() consistently to remove a request from one
> + * of the request lists.
> + *
>   * Note: the state of a request may change while this function is in 
> progress,
>   * e.g. due to a concurrent blk_mq_finish_request() call.
>   */
> @@ -402,7 +408,7 @@ static void hctx_show_busy_rq(struct request *rq, void 
> *data, bool reserved)
>   const struct show_busy_params *params = data;
>  
>   if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx &&
> - blk_mq_rq_state(rq) != MQ_RQ_IDLE)
> + list_empty(>queuelist))
>   __blk_mq_debugfs_rq_show(params->m,
>list_entry_rq(>queuelist));
>  }

Hello Jens,

Can you share your opinion about the above patch?

Thanks,

Bart.



[PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-02-12 Thread Bart Van Assche
When debugging e.g. the SCSI timeout handler it is important that
requests that have not yet been started or that already have
completed are also reported through debugfs.

This patch depends on a patch that went upstream in kernel v4.15,
namely commit 14e3062fb185 ("scsi: core: Fix a scsi_show_rq() NULL
pointer dereference").

Signed-off-by: Bart Van Assche 
Cc: Ming Lei 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Martin K. Petersen 
---
 block/blk-mq-debugfs.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index c62a3704515b..bcb64b2f8090 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -401,6 +401,12 @@ struct show_busy_params {
 };
 
 /*
+ * Show "busy" requests - these are the requests owned by the block driver.
+ * The test list_empty(>queuelist) is used to figure out whether or not
+ * a request is owned by the block driver. That test works because the block
+ * layer core uses list_del_init() consistently to remove a request from one
+ * of the request lists.
+ *
  * Note: the state of a request may change while this function is in progress,
  * e.g. due to a concurrent blk_mq_finish_request() call.
  */
@@ -409,7 +415,7 @@ static void hctx_show_busy_rq(struct request *rq, void 
*data, bool reserved)
const struct show_busy_params *params = data;
 
if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx &&
-   blk_mq_rq_state(rq) != MQ_RQ_IDLE)
+   list_empty(>queuelist))
__blk_mq_debugfs_rq_show(params->m,
 list_entry_rq(>queuelist));
 }
-- 
2.16.1



Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-01-12 Thread Bart Van Assche
On Fri, 2018-01-12 at 15:05 -0700, Jens Axboe wrote:
> On 1/12/18 3:00 PM, Bart Van Assche wrote:
> > On Fri, 2018-01-12 at 14:55 -0700, Jens Axboe wrote:
> > > On 1/12/18 2:52 PM, Bart Van Assche wrote:
> > > > When debugging e.g. the SCSI timeout handler it is important that
> > > > requests that have not yet been started or that already have
> > > > completed are also reported through debugfs.
> > > > 
> > > > This patch depends on a patch that went upstream recently, namely
> > > > commit 14e3062fb185 ("scsi: core: Fix a scsi_show_rq() NULL pointer
> > > > dereference").
> > > 
> > > Why don't we just kill the check, and dump any request that has a
> > > matching hctx? We already know the bit was set, so just print
> > > all of them.
> > 
> > It is very helpful during debugging that requests owned by a block driver 
> > and
> > requests owned by the block layer core show up in different debugfs files.
> > Removing the check completely would cause all requests to show up in the 
> > same
> > debugfs file and would make interpreting the contents of these debugfs files
> > much harder.
> 
> Yeah, we'd have to make it just one file at that point. I'm not hugely
> against the queuelist check, but probably warrants a comment as it's not
> immediately clear (as opposed to the idle check, or the previous START
> bit check).

How about the below?

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 19db3f583bf1..da859dac442b 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -394,6 +394,12 @@ struct show_busy_params {
 };
 
 /*
+ * Show "busy" requests - these are the requests owned by the block driver.
+ * The test list_empty(>queuelist) is used to figure out whether or not
+ * a request is owned by the block driver. That test works because the block
+ * layer core uses list_del_init() consistently to remove a request from one
+ * of the request lists.
+ *
  * Note: the state of a request may change while this function is in progress,
  * e.g. due to a concurrent blk_mq_finish_request() call.
  */
@@ -402,7 +408,7 @@ static void hctx_show_busy_rq(struct request *rq, void 
*data, bool reserved)
const struct show_busy_params *params = data;
 
if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx &&
-   blk_mq_rq_state(rq) != MQ_RQ_IDLE)
+   list_empty(>queuelist))
__blk_mq_debugfs_rq_show(params->m,
 list_entry_rq(>queuelist));
 }

Thanks,

Bart.

Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-01-12 Thread Jens Axboe
On 1/12/18 3:00 PM, Bart Van Assche wrote:
> On Fri, 2018-01-12 at 14:55 -0700, Jens Axboe wrote:
>> On 1/12/18 2:52 PM, Bart Van Assche wrote:
>>> When debugging e.g. the SCSI timeout handler it is important that
>>> requests that have not yet been started or that already have
>>> completed are also reported through debugfs.
>>>
>>> This patch depends on a patch that went upstream recently, namely
>>> commit 14e3062fb185 ("scsi: core: Fix a scsi_show_rq() NULL pointer
>>> dereference").
>>
>> Why don't we just kill the check, and dump any request that has a
>> matching hctx? We already know the bit was set, so just print
>> all of them.
> 
> It is very helpful during debugging that requests owned by a block driver and
> requests owned by the block layer core show up in different debugfs files.
> Removing the check completely would cause all requests to show up in the same
> debugfs file and would make interpreting the contents of these debugfs files
> much harder.

Yeah, we'd have to make it just one file at that point. I'm not hugely
against the queuelist check, but probably warrants a comment as it's not
immediately clear (as opposed to the idle check, or the previous START
bit check).

-- 
Jens Axboe



Re: [PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-01-12 Thread Jens Axboe
On 1/12/18 2:52 PM, Bart Van Assche wrote:
> When debugging e.g. the SCSI timeout handler it is important that
> requests that have not yet been started or that already have
> completed are also reported through debugfs.
> 
> This patch depends on a patch that went upstream recently, namely
> commit 14e3062fb185 ("scsi: core: Fix a scsi_show_rq() NULL pointer
> dereference").

Why don't we just kill the check, and dump any request that has a
matching hctx? We already know the bit was set, so just print
all of them.

-- 
Jens Axboe



[PATCH] blk-mq-debugfs: Also show requests that have not yet been started

2018-01-12 Thread Bart Van Assche
When debugging e.g. the SCSI timeout handler it is important that
requests that have not yet been started or that already have
completed are also reported through debugfs.

This patch depends on a patch that went upstream recently, namely
commit 14e3062fb185 ("scsi: core: Fix a scsi_show_rq() NULL pointer
dereference").

Signed-off-by: Bart Van Assche 
Cc: Ming Lei 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
Cc: Martin K. Petersen 
---
 block/blk-mq-debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 19db3f583bf1..ccd5ef08c3f0 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -402,7 +402,7 @@ static void hctx_show_busy_rq(struct request *rq, void 
*data, bool reserved)
const struct show_busy_params *params = data;
 
if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == params->hctx &&
-   blk_mq_rq_state(rq) != MQ_RQ_IDLE)
+   list_empty(>queuelist))
__blk_mq_debugfs_rq_show(params->m,
 list_entry_rq(>queuelist));
 }
-- 
2.15.1