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

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

[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 <bart.vanassche@wdc.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
---
 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(&rq->queuelist))
 		__blk_mq_debugfs_rq_show(params->m,
 					 list_entry_rq(&rq->queuelist));
 }
-- 
2.15.1

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

[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

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

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAxLTEyIGF0IDE0OjU1IC0wNzAwLCBKZW5zIEF4Ym9lIHdyb3RlOg0KPiBP
biAxLzEyLzE4IDI6NTIgUE0sIEJhcnQgVmFuIEFzc2NoZSB3cm90ZToNCj4gPiBXaGVuIGRlYnVn
Z2luZyBlLmcuIHRoZSBTQ1NJIHRpbWVvdXQgaGFuZGxlciBpdCBpcyBpbXBvcnRhbnQgdGhhdA0K
PiA+IHJlcXVlc3RzIHRoYXQgaGF2ZSBub3QgeWV0IGJlZW4gc3RhcnRlZCBvciB0aGF0IGFscmVh
ZHkgaGF2ZQ0KPiA+IGNvbXBsZXRlZCBhcmUgYWxzbyByZXBvcnRlZCB0aHJvdWdoIGRlYnVnZnMu
DQo+ID4gDQo+ID4gVGhpcyBwYXRjaCBkZXBlbmRzIG9uIGEgcGF0Y2ggdGhhdCB3ZW50IHVwc3Ry
ZWFtIHJlY2VudGx5LCBuYW1lbHkNCj4gPiBjb21taXQgMTRlMzA2MmZiMTg1ICgic2NzaTogY29y
ZTogRml4IGEgc2NzaV9zaG93X3JxKCkgTlVMTCBwb2ludGVyDQo+ID4gZGVyZWZlcmVuY2UiKS4N
Cj4gDQo+IFdoeSBkb24ndCB3ZSBqdXN0IGtpbGwgdGhlIGNoZWNrLCBhbmQgZHVtcCBhbnkgcmVx
dWVzdCB0aGF0IGhhcyBhDQo+IG1hdGNoaW5nIGhjdHg/IFdlIGFscmVhZHkga25vdyB0aGUgYml0
IHdhcyBzZXQsIHNvIGp1c3QgcHJpbnQNCj4gYWxsIG9mIHRoZW0uDQoNCkl0IGlzIHZlcnkgaGVs
cGZ1bCBkdXJpbmcgZGVidWdnaW5nIHRoYXQgcmVxdWVzdHMgb3duZWQgYnkgYSBibG9jayBkcml2
ZXIgYW5kDQpyZXF1ZXN0cyBvd25lZCBieSB0aGUgYmxvY2sgbGF5ZXIgY29yZSBzaG93IHVwIGlu
IGRpZmZlcmVudCBkZWJ1Z2ZzIGZpbGVzLg0KUmVtb3ZpbmcgdGhlIGNoZWNrIGNvbXBsZXRlbHkg
d291bGQgY2F1c2UgYWxsIHJlcXVlc3RzIHRvIHNob3cgdXAgaW4gdGhlIHNhbWUNCmRlYnVnZnMg
ZmlsZSBhbmQgd291bGQgbWFrZSBpbnRlcnByZXRpbmcgdGhlIGNvbnRlbnRzIG9mIHRoZXNlIGRl
YnVnZnMgZmlsZXMNCm11Y2ggaGFyZGVyLg0KDQpUaGFua3MsDQoNCkJhcnQu

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

[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

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAxLTEyIGF0IDE1OjA1IC0wNzAwLCBKZW5zIEF4Ym9lIHdyb3RlOg0KPiBP
biAxLzEyLzE4IDM6MDAgUE0sIEJhcnQgVmFuIEFzc2NoZSB3cm90ZToNCj4gPiBPbiBGcmksIDIw
MTgtMDEtMTIgYXQgMTQ6NTUgLTA3MDAsIEplbnMgQXhib2Ugd3JvdGU6DQo+ID4gPiBPbiAxLzEy
LzE4IDI6NTIgUE0sIEJhcnQgVmFuIEFzc2NoZSB3cm90ZToNCj4gPiA+ID4gV2hlbiBkZWJ1Z2dp
bmcgZS5nLiB0aGUgU0NTSSB0aW1lb3V0IGhhbmRsZXIgaXQgaXMgaW1wb3J0YW50IHRoYXQNCj4g
PiA+ID4gcmVxdWVzdHMgdGhhdCBoYXZlIG5vdCB5ZXQgYmVlbiBzdGFydGVkIG9yIHRoYXQgYWxy
ZWFkeSBoYXZlDQo+ID4gPiA+IGNvbXBsZXRlZCBhcmUgYWxzbyByZXBvcnRlZCB0aHJvdWdoIGRl
YnVnZnMuDQo+ID4gPiA+IA0KPiA+ID4gPiBUaGlzIHBhdGNoIGRlcGVuZHMgb24gYSBwYXRjaCB0
aGF0IHdlbnQgdXBzdHJlYW0gcmVjZW50bHksIG5hbWVseQ0KPiA+ID4gPiBjb21taXQgMTRlMzA2
MmZiMTg1ICgic2NzaTogY29yZTogRml4IGEgc2NzaV9zaG93X3JxKCkgTlVMTCBwb2ludGVyDQo+
ID4gPiA+IGRlcmVmZXJlbmNlIikuDQo+ID4gPiANCj4gPiA+IFdoeSBkb24ndCB3ZSBqdXN0IGtp
bGwgdGhlIGNoZWNrLCBhbmQgZHVtcCBhbnkgcmVxdWVzdCB0aGF0IGhhcyBhDQo+ID4gPiBtYXRj
aGluZyBoY3R4PyBXZSBhbHJlYWR5IGtub3cgdGhlIGJpdCB3YXMgc2V0LCBzbyBqdXN0IHByaW50
DQo+ID4gPiBhbGwgb2YgdGhlbS4NCj4gPiANCj4gPiBJdCBpcyB2ZXJ5IGhlbHBmdWwgZHVyaW5n
IGRlYnVnZ2luZyB0aGF0IHJlcXVlc3RzIG93bmVkIGJ5IGEgYmxvY2sgZHJpdmVyIGFuZA0KPiA+
IHJlcXVlc3RzIG93bmVkIGJ5IHRoZSBibG9jayBsYXllciBjb3JlIHNob3cgdXAgaW4gZGlmZmVy
ZW50IGRlYnVnZnMgZmlsZXMuDQo+ID4gUmVtb3ZpbmcgdGhlIGNoZWNrIGNvbXBsZXRlbHkgd291
bGQgY2F1c2UgYWxsIHJlcXVlc3RzIHRvIHNob3cgdXAgaW4gdGhlIHNhbWUNCj4gPiBkZWJ1Z2Zz
IGZpbGUgYW5kIHdvdWxkIG1ha2UgaW50ZXJwcmV0aW5nIHRoZSBjb250ZW50cyBvZiB0aGVzZSBk
ZWJ1Z2ZzIGZpbGVzDQo+ID4gbXVjaCBoYXJkZXIuDQo+IA0KPiBZZWFoLCB3ZSdkIGhhdmUgdG8g
bWFrZSBpdCBqdXN0IG9uZSBmaWxlIGF0IHRoYXQgcG9pbnQuIEknbSBub3QgaHVnZWx5DQo+IGFn
YWluc3QgdGhlIHF1ZXVlbGlzdCBjaGVjaywgYnV0IHByb2JhYmx5IHdhcnJhbnRzIGEgY29tbWVu
dCBhcyBpdCdzIG5vdA0KPiBpbW1lZGlhdGVseSBjbGVhciAoYXMgb3Bwb3NlZCB0byB0aGUgaWRs
ZSBjaGVjaywgb3IgdGhlIHByZXZpb3VzIFNUQVJUDQo+IGJpdCBjaGVjaykuDQoNCkhvdyBhYm91
dCB0aGUgYmVsb3c/DQoNCmRpZmYgLS1naXQgYS9ibG9jay9ibGstbXEtZGVidWdmcy5jIGIvYmxv
Y2svYmxrLW1xLWRlYnVnZnMuYw0KaW5kZXggMTlkYjNmNTgzYmYxLi5kYTg1OWRhYzQ0MmIgMTAw
NjQ0DQotLS0gYS9ibG9jay9ibGstbXEtZGVidWdmcy5jDQorKysgYi9ibG9jay9ibGstbXEtZGVi
dWdmcy5jDQpAQCAtMzk0LDYgKzM5NCwxMiBAQCBzdHJ1Y3Qgc2hvd19idXN5X3BhcmFtcyB7DQog
fTsNCiANCiAvKg0KKyAqIFNob3cgImJ1c3kiIHJlcXVlc3RzIC0gdGhlc2UgYXJlIHRoZSByZXF1
ZXN0cyBvd25lZCBieSB0aGUgYmxvY2sgZHJpdmVyLg0KKyAqIFRoZSB0ZXN0IGxpc3RfZW1wdHko
JnJxLT5xdWV1ZWxpc3QpIGlzIHVzZWQgdG8gZmlndXJlIG91dCB3aGV0aGVyIG9yIG5vdA0KKyAq
IGEgcmVxdWVzdCBpcyBvd25lZCBieSB0aGUgYmxvY2sgZHJpdmVyLiBUaGF0IHRlc3Qgd29ya3Mg
YmVjYXVzZSB0aGUgYmxvY2sNCisgKiBsYXllciBjb3JlIHVzZXMgbGlzdF9kZWxfaW5pdCgpIGNv
bnNpc3RlbnRseSB0byByZW1vdmUgYSByZXF1ZXN0IGZyb20gb25lDQorICogb2YgdGhlIHJlcXVl
c3QgbGlzdHMuDQorICoNCiAgKiBOb3RlOiB0aGUgc3RhdGUgb2YgYSByZXF1ZXN0IG1heSBjaGFu
Z2Ugd2hpbGUgdGhpcyBmdW5jdGlvbiBpcyBpbiBwcm9ncmVzcywNCiAgKiBlLmcuIGR1ZSB0byBh
IGNvbmN1cnJlbnQgYmxrX21xX2ZpbmlzaF9yZXF1ZXN0KCkgY2FsbC4NCiAgKi8NCkBAIC00MDIs
NyArNDA4LDcgQEAgc3RhdGljIHZvaWQgaGN0eF9zaG93X2J1c3lfcnEoc3RydWN0IHJlcXVlc3Qg
KnJxLCB2b2lkICpkYXRhLCBib29sIHJlc2VydmVkKQ0KIAljb25zdCBzdHJ1Y3Qgc2hvd19idXN5
X3BhcmFtcyAqcGFyYW1zID0gZGF0YTsNCiANCiAJaWYgKGJsa19tcV9tYXBfcXVldWUocnEtPnEs
IHJxLT5tcV9jdHgtPmNwdSkgPT0gcGFyYW1zLT5oY3R4ICYmDQotCSAgICBibGtfbXFfcnFfc3Rh
dGUocnEpICE9IE1RX1JRX0lETEUpDQorCSAgICBsaXN0X2VtcHR5KCZycS0+cXVldWVsaXN0KSkN
CiAJCV9fYmxrX21xX2RlYnVnZnNfcnFfc2hvdyhwYXJhbXMtPm0sDQogCQkJCQkgbGlzdF9lbnRy
eV9ycSgmcnEtPnF1ZXVlbGlzdCkpOw0KIH0NCg0KVGhhbmtzLA0KDQpCYXJ0Lg==

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

[Bart Van Assche]

On Fri, 2018-01-12 at 22:11 +0000, 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(&rq->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(&rq->queuelist))
>  		__blk_mq_debugfs_rq_show(params->m,
>  					 list_entry_rq(&rq->queuelist));
>  }

Hello Jens,

Can you share your opinion about the above patch?

Thanks,

Bart.

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

[Jens Axboe]

On 10/3/18 3:42 PM, Bart Van Assche wrote:
> On Fri, 2018-01-12 at 22:11 +0000, 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(&rq->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(&rq->queuelist))
>>  		__blk_mq_debugfs_rq_show(params->m,
>>  					 list_entry_rq(&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

[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 +0000, Bart Van Assche wrote:
> > >  /*
> > > + * Show "busy" requests - these are the requests owned by the block driver.
> > > + * The test list_empty(&rq->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(&rq->queuelist))
> > >  		__blk_mq_debugfs_rq_show(params->m,
> > >  					 list_entry_rq(&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:00000000abe67123 {.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

[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 +0000, Bart Van Assche wrote:
>>>>  /*
>>>> + * Show "busy" requests - these are the requests owned by the block driver.
>>>> + * The test list_empty(&rq->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(&rq->queuelist))
>>>>  		__blk_mq_debugfs_rq_show(params->m,
>>>>  					 list_entry_rq(&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:00000000abe67123 {.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

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

[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 <bart.vanassche@wdc.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
---
 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(&rq->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(&rq->queuelist))
 		__blk_mq_debugfs_rq_show(params->m,
 					 list_entry_rq(&rq->queuelist));
 }
-- 
2.16.1

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

[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 <bvanassche@acm.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
---
 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(&rq->queuelist));
 }
-- 
2.19.0.605.g01d371f741-goog

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

[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 <bvanassche@acm.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>   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(&rq->queuelist));
>   }
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@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)

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

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@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

[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 <bvanassche@acm.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>

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

[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 <bvanassche@acm.org>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Ming Lei <ming.lei@redhat.com>
> > Cc: Hannes Reinecke <hare@suse.com>
> > Cc: Johannes Thumshirn <jthumshirn@suse.de>
> > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> 
> 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

[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 <bvanassche@acm.org>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Ming Lei <ming.lei@redhat.com>
>>> Cc: Hannes Reinecke <hare@suse.com>
>>> Cc: Johannes Thumshirn <jthumshirn@suse.de>
>>> Cc: Martin K. Petersen <martin.petersen@oracle.com>
>>
>> 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

[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 <bvanassche@acm.org>
> >>> Cc: Christoph Hellwig <hch@lst.de>
> >>> Cc: Ming Lei <ming.lei@redhat.com>
> >>> Cc: Hannes Reinecke <hare@suse.com>
> >>> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> >>> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> >>
> >> 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
jthumshirn@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