Virtualization - Oct 2022 - [PATCH V2 1/1] blk-mq: avoid double ->queue_rq() because of early timeout

On Tue, Oct 25, 2022 at 09:11:37PM -0600, Keith Busch
wrote:
> On Wed, Oct 26, 2022 at 09:55:21AM +0800, Ming Lei wrote:
> > @@ -1564,8 +1571,13 @@ static bool blk_mq_check_expired(struct request
*rq, void *priv)
> >  	 * it was completed and reallocated as a new request after returning
> >  	 * from blk_mq_check_expired().
> >  	 */
> > -	if (blk_mq_req_expired(rq, next))
> > +	if (blk_mq_req_expired(rq, expired)) {
> > +		if (expired->check_only) {
> > +			expired->has_timedout_rq = true;
> > +			return false;
> > +		}
> >  		blk_mq_rq_timed_out(rq);
> > +	}
> >  	return true;
> >  }
> >  
> > @@ -1573,7 +1585,10 @@ static void blk_mq_timeout_work(struct
work_struct *work)
> >  {
> >  	struct request_queue *q > >  		container_of(work, struct
request_queue, timeout_work);
> > -	unsigned long next = 0;
> > +	struct blk_expired_data expired = {
> > +		.check_only = true,
> > +		.timeout_start = jiffies,
> > +	};
> >  	struct blk_mq_hw_ctx *hctx;
> >  	unsigned long i;
> >  
> > @@ -1593,10 +1608,24 @@ static void blk_mq_timeout_work(struct
work_struct *work)
> >  	if (!percpu_ref_tryget(&q->q_usage_counter))
> >  		return;
> >  
> > -	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &next);
> > +	/* check if there is any timed-out request */
> > +	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &expired);
> > +	if (expired.has_timedout_rq) {
> > +		/*
> > +		 * Before walking tags, we must ensure any submit started
> > +		 * before the current time has finished. Since the submit
> > +		 * uses srcu or rcu, wait for a synchronization point to
> > +		 * ensure all running submits have finished
> > +		 */
> > +		blk_mq_wait_quiesce_done(q);
> > +
> > +		expired.check_only = false;
> > +		expired.next = 0;
> > +		blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &expired);
> 
> I think it would be easier to follow with separate callbacks instead of
> special casing for 'check_only'. One callback for checking
timeouts, and
> a different one for handling them?
Both two are basically same, with two callbacks, just .check_only is saved,
nothing else, meantime with one extra similar callback added.

If you or anyone think it is one big deal, I can switch to two callback version.


Thanks,
Ming