Re: [PATCH] blk-mq: Wait for for hctx inflight requests on CPU unplug

[Ming Lei <ming.lei@redhat.com>]

On Tue, May 21, 2019 at 09:40:19AM +0200, Christoph Hellwig wrote:
> > diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> > index 7513c8eaabee..b24334f99c5d 100644
> > --- a/block/blk-mq-tag.c
> > +++ b/block/blk-mq-tag.c
> > @@ -332,7 +332,7 @@ static void bt_tags_for_each(struct blk_mq_tags *tags, struct sbitmap_queue *bt,
> >   *		true to continue iterating tags, false to stop.
> >   * @priv:	Will be passed as second argument to @fn.
> >   */
> > -static void blk_mq_all_tag_busy_iter(struct blk_mq_tags *tags,
> > +void blk_mq_all_tag_busy_iter(struct blk_mq_tags *tags,
> >  		busy_tag_iter_fn *fn, void *priv)
> 
> How about moving blk_mq_tags_inflight_rqs to blk-mq-tag.c instead?

OK.

> 
> > +	#define BLK_MQ_TAGS_DRAINED           0
> 
> Please don't indent #defines.

OK.

> 
> >  
> > +static int blk_mq_hctx_notify_prepare(unsigned int cpu, struct hlist_node *node)
> > +{
> > +	struct blk_mq_hw_ctx	*hctx;
> > +	struct blk_mq_tags	*tags;
> > +
> > +	tags = hctx->tags;
> > +
> > +	if (tags)
> > +		clear_bit(BLK_MQ_TAGS_DRAINED, &tags->flags);
> > +
> > +	return 0;
> 
> I'd write this as:
> 
> {
> 	struct blk_mq_hw_ctx	*hctx = 
> 		hlist_entry_safe(node, struct blk_mq_hw_ctx, cpuhp_dead);
> 
> 	if (hctx->tags)
> 		clear_bit(BLK_MQ_TAGS_DRAINED, &hctx->tags->flags);
> 	return 0;
> }

OK.

> 
> > +static void blk_mq_drain_inflight_rqs(struct blk_mq_tags *tags, int dead_cpu)
> > +{
> > +	unsigned long msecs_left = 1000 * 5;
> > +
> > +	if (!tags)
> > +		return;
> > +
> > +	if (test_and_set_bit(BLK_MQ_TAGS_DRAINED, &tags->flags))
> > +		return;
> > +
> > +	while (msecs_left > 0) {
> > +		if (!blk_mq_tags_inflight_rqs(tags))
> > +			break;
> > +		msleep(5);
> > +		msecs_left -= 5;
> > +	}
> > +
> > +	if (msecs_left > 0)
> > +		printk(KERN_WARNING "requests not completed from dead "
> > +				"CPU %d\n", dead_cpu);
> > +}
> 
> Isn't this condition inverted?  If we break out early msecs_left will
> be larger than 0 and we are fine.

Yeah, I saw that just after the patch was posted.

> 
> Why not:
> 
> 	for (attempts = 0; attempts < 1000; attempts++) {
> 		if (!blk_mq_tags_inflight_rqs(tags))
> 			return;
> 	}

Fine.

> 
> 	...
> 
> But more importantly I'm not sure we can just give up that easily.
> Shouldn't we at lest wait the same timeout we otherwise have for
> requests, and if the command isn't finished in time kick off error
> handling?

The default 30sec timeout is too long here, and may cause new bug
report on CPU hotplug.

Also it makes no difference by waiting 30sec, given timeout
handler will be triggered when request is really timed out.

However, one problem is that some drivers may simply return RESET_TIMER
in their timeout handler, then no simple solution for these drivers.


Thanks,
Ming