diff mbox

[v2,3/4] blk-mq-debugfs: Show busy requests

Message ID 20170531213050.30276-4-bart.vanassche@sandisk.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bart Van Assche May 31, 2017, 9:30 p.m. UTC 
Requests that got stuck in a block driver are neither on
blk_mq_ctx.rq_list nor on any hw dispatch queue. Make these
visible in debugfs through the "busy" attribute.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-debugfs.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Eduardo Valentin May 31, 2017, 9:43 p.m. UTC | #1 
Hello,

On Wed, May 31, 2017 at 02:30:49PM -0700, Bart Van Assche wrote:
> Requests that got stuck in a block driver are neither on
> blk_mq_ctx.rq_list nor on any hw dispatch queue. Make these
> visible in debugfs through the "busy" attribute.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-debugfs.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 8b06a12c1461..fa0f624dfccd 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -370,6 +370,31 @@ static const struct seq_operations hctx_dispatch_seq_ops = {
>  	.show	= blk_mq_debugfs_rq_show,
>  };
>  
> +struct show_busy_params {
> +	struct seq_file		*m;
> +	struct blk_mq_hw_ctx	*hctx;
> +};
> +
> +static void hctx_show_busy(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 &&
> +	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> +		__blk_mq_debugfs_rq_show(params->m,
> +					 list_entry_rq(&rq->queuelist));
> +}
> +
> +static int hctx_busy_show(void *data, struct seq_file *m)
> +{
> +	struct blk_mq_hw_ctx *hctx = data;
> +	struct show_busy_params params = { .m = m, .hctx = hctx };
> +
> +	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy, &params);
> +
> +	return 0;
> +}

Why not making the two above one single function?
hctx_busy_show vs. hctx_show_busy seams a bit confusing, and I could not see
where they get reused in your patch set..


> +
>  static int hctx_ctx_map_show(void *data, struct seq_file *m)
>  {
>  	struct blk_mq_hw_ctx *hctx = data;
> @@ -705,6 +730,7 @@ static const struct blk_mq_debugfs_attr blk_mq_debugfs_hctx_attrs[] = {
>  	{"state", 0400, hctx_state_show},
>  	{"flags", 0400, hctx_flags_show},
>  	{"dispatch", 0400, .seq_ops = &hctx_dispatch_seq_ops},
> +	{"busy", 0400, hctx_busy_show},
>  	{"ctx_map", 0400, hctx_ctx_map_show},
>  	{"tags", 0400, hctx_tags_show},
>  	{"tags_bitmap", 0400, hctx_tags_bitmap_show},
> -- 
> 2.12.2
> 
>
Bart Van Assche May 31, 2017, 9:45 p.m. UTC | #2 
On Wed, 2017-05-31 at 14:43 -0700, Eduardo Valentin wrote:
> On Wed, May 31, 2017 at 02:30:49PM -0700, Bart Van Assche wrote:
> > +static void hctx_show_busy(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 &&
> > +	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> > +		__blk_mq_debugfs_rq_show(params->m,
> > +					 list_entry_rq(&rq->queuelist));
> > +}
> > +
> > +static int hctx_busy_show(void *data, struct seq_file *m)
> > +{
> > +	struct blk_mq_hw_ctx *hctx = data;
> > +	struct show_busy_params params = { .m = m, .hctx = hctx };
> > +
> > +	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy, &params);
> > +
> > +	return 0;
> > +}
> 
> Why not making the two above one single function?
> hctx_busy_show vs. hctx_show_busy seams a bit confusing, and I could not see
> where they get reused in your patch set..

Hello Eduardo,

If I would open-code blk_mq_tagset_busy_iter() then I would be able to implement
the above two functions as a single function. However, blk_mq_tagset_busy_iter()
expects a function pointer as third argument. That's why the above functionality
has been split over two functions.

Bart.
Eduardo Valentin May 31, 2017, 9:49 p.m. UTC | #3 
On Wed, May 31, 2017 at 09:45:54PM +0000, Bart Van Assche wrote:
> On Wed, 2017-05-31 at 14:43 -0700, Eduardo Valentin wrote:
> > On Wed, May 31, 2017 at 02:30:49PM -0700, Bart Van Assche wrote:
> > > +static void hctx_show_busy(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 &&
> > > +	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> > > +		__blk_mq_debugfs_rq_show(params->m,
> > > +					 list_entry_rq(&rq->queuelist));
> > > +}
> > > +
> > > +static int hctx_busy_show(void *data, struct seq_file *m)
> > > +{
> > > +	struct blk_mq_hw_ctx *hctx = data;
> > > +	struct show_busy_params params = { .m = m, .hctx = hctx };
> > > +
> > > +	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy, &params);
> > > +
> > > +	return 0;
> > > +}
> > 
> > Why not making the two above one single function?
> > hctx_busy_show vs. hctx_show_busy seams a bit confusing, and I could not see
> > where they get reused in your patch set..
> 
> Hello Eduardo,
> 
> If I would open-code blk_mq_tagset_busy_iter() then I would be able to implement
> the above two functions as a single function. However, blk_mq_tagset_busy_iter()
> expects a function pointer as third argument. That's why the above functionality
> has been split over two functions.

Yeah, my bad here. I misread the functions. But still the naming doesnt seam
too suggestive? how about s/hctx_show_busy/hctx_busy_entry/g?

> 
> Bart.
Bart Van Assche May 31, 2017, 9:54 p.m. UTC | #4 
On Wed, 2017-05-31 at 14:49 -0700, Eduardo Valentin wrote:
> On Wed, May 31, 2017 at 09:45:54PM +0000, Bart Van Assche wrote:
> > On Wed, 2017-05-31 at 14:43 -0700, Eduardo Valentin wrote:
> > > On Wed, May 31, 2017 at 02:30:49PM -0700, Bart Van Assche wrote:
> > > > +static void hctx_show_busy(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 &&
> > > > +	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> > > > +		__blk_mq_debugfs_rq_show(params->m,
> > > > +					 list_entry_rq(&rq->queuelist));
> > > > +}
> > > > +
> > > > +static int hctx_busy_show(void *data, struct seq_file *m)
> > > > +{
> > > > +	struct blk_mq_hw_ctx *hctx = data;
> > > > +	struct show_busy_params params = { .m = m, .hctx = hctx };
> > > > +
> > > > +	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy, &params);
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > Why not making the two above one single function?
> > > hctx_busy_show vs. hctx_show_busy seams a bit confusing, and I could not see
> > > where they get reused in your patch set..
> > 
> > Hello Eduardo,
> > 
> > If I would open-code blk_mq_tagset_busy_iter() then I would be able to implement
> > the above two functions as a single function. However, blk_mq_tagset_busy_iter()
> > expects a function pointer as third argument. That's why the above functionality
> > has been split over two functions.
> 
> Yeah, my bad here. I misread the functions. But still the naming doesnt seam
> too suggestive? how about s/hctx_show_busy/hctx_busy_entry/g?

Hello Eduardo,

Since that function shows information about a single request, how about
hctx_show_busy_rq()?

Bart.
Eduardo Valentin May 31, 2017, 11:27 p.m. UTC | #5 
On Wed, May 31, 2017 at 09:54:11PM +0000, Bart Van Assche wrote:
> On Wed, 2017-05-31 at 14:49 -0700, Eduardo Valentin wrote:
> > On Wed, May 31, 2017 at 09:45:54PM +0000, Bart Van Assche wrote:
> > > On Wed, 2017-05-31 at 14:43 -0700, Eduardo Valentin wrote:
> > > > On Wed, May 31, 2017 at 02:30:49PM -0700, Bart Van Assche wrote:
> > > > > +static void hctx_show_busy(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 &&
> > > > > +	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> > > > > +		__blk_mq_debugfs_rq_show(params->m,
> > > > > +					 list_entry_rq(&rq->queuelist));
> > > > > +}
> > > > > +
> > > > > +static int hctx_busy_show(void *data, struct seq_file *m)
> > > > > +{
> > > > > +	struct blk_mq_hw_ctx *hctx = data;
> > > > > +	struct show_busy_params params = { .m = m, .hctx = hctx };
> > > > > +
> > > > > +	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy, &params);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > 
> > > > Why not making the two above one single function?
> > > > hctx_busy_show vs. hctx_show_busy seams a bit confusing, and I could not see
> > > > where they get reused in your patch set..
> > > 
> > > Hello Eduardo,
> > > 
> > > If I would open-code blk_mq_tagset_busy_iter() then I would be able to implement
> > > the above two functions as a single function. However, blk_mq_tagset_busy_iter()
> > > expects a function pointer as third argument. That's why the above functionality
> > > has been split over two functions.
> > 
> > Yeah, my bad here. I misread the functions. But still the naming doesnt seam
> > too suggestive? how about s/hctx_show_busy/hctx_busy_entry/g?
> 
> Hello Eduardo,
> 
> Since that function shows information about a single request, how about
> hctx_show_busy_rq()?

Sounds good to me.

> 
> Bart.
Ming Lei June 1, 2017, 2:47 a.m. UTC | #6 
On Wed, May 31, 2017 at 02:30:49PM -0700, Bart Van Assche wrote:
> Requests that got stuck in a block driver are neither on
> blk_mq_ctx.rq_list nor on any hw dispatch queue. Make these
> visible in debugfs through the "busy" attribute.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-debugfs.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 8b06a12c1461..fa0f624dfccd 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -370,6 +370,31 @@ static const struct seq_operations hctx_dispatch_seq_ops = {
>  	.show	= blk_mq_debugfs_rq_show,
>  };
>  
> +struct show_busy_params {
> +	struct seq_file		*m;
> +	struct blk_mq_hw_ctx	*hctx;
> +};
> +
> +static void hctx_show_busy(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 &&
> +	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> +		__blk_mq_debugfs_rq_show(params->m,
> +					 list_entry_rq(&rq->queuelist));
> +}

Not like dumping requests in ctx and requeue list, the dumped requests
here may have been released and the result may not be 100% reliable,
so suggest to add comment for this fact.

Otherwise, looks fine for me.

Thanks,
Ming
Hannes Reinecke June 1, 2017, 5:48 a.m. UTC | #7 
On 05/31/2017 11:30 PM, Bart Van Assche wrote:
> Requests that got stuck in a block driver are neither on
> blk_mq_ctx.rq_list nor on any hw dispatch queue. Make these
> visible in debugfs through the "busy" attribute.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-debugfs.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
diff mbox

Patch

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 8b06a12c1461..fa0f624dfccd 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -370,6 +370,31 @@  static const struct seq_operations hctx_dispatch_seq_ops = {
 	.show	= blk_mq_debugfs_rq_show,
 };
 
+struct show_busy_params {
+	struct seq_file		*m;
+	struct blk_mq_hw_ctx	*hctx;
+};
+
+static void hctx_show_busy(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 &&
+	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
+		__blk_mq_debugfs_rq_show(params->m,
+					 list_entry_rq(&rq->queuelist));
+}
+
+static int hctx_busy_show(void *data, struct seq_file *m)
+{
+	struct blk_mq_hw_ctx *hctx = data;
+	struct show_busy_params params = { .m = m, .hctx = hctx };
+
+	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy, &params);
+
+	return 0;
+}
+
 static int hctx_ctx_map_show(void *data, struct seq_file *m)
 {
 	struct blk_mq_hw_ctx *hctx = data;
@@ -705,6 +730,7 @@  static const struct blk_mq_debugfs_attr blk_mq_debugfs_hctx_attrs[] = {
 	{"state", 0400, hctx_state_show},
 	{"flags", 0400, hctx_flags_show},
 	{"dispatch", 0400, .seq_ops = &hctx_dispatch_seq_ops},
+	{"busy", 0400, hctx_busy_show},
 	{"ctx_map", 0400, hctx_ctx_map_show},
 	{"tags", 0400, hctx_tags_show},
 	{"tags_bitmap", 0400, hctx_tags_bitmap_show},