Re: [RFC PATCH] blk-mq: move timeout handling from queue to tagset

["jianchao.wang" <jianchao.w.wang@oracle.com>]

Hi Keith

On 07/19/2018 01:45 AM, Keith Busch wrote:
>>> +	list_for_each_entry(q, &set->tag_list, tag_set_list) {
>>>  		/*
>>>  		 * Request timeouts are handled as a forward rolling timer. If
>>>  		 * we end up here it means that no requests are pending and
>>> @@ -881,7 +868,6 @@ static void blk_mq_timeout_work(struct work_struct *work)
>>>  				blk_mq_tag_idle(hctx);
>>>  		}
>>>  	}
>>> -	blk_queue_exit(q);

The tags sharing fairness mechanism between different request_queues cannot work well here.
When timer is per-request_queue, if there is no request on one request_queue,
it could be idled. But now, with per-tagset timer, we cannot detect the idle one at all.


>   
>>> +	timer_setup(&set->timer, blk_mq_timed_out_timer, 0);
>>> +	INIT_WORK(&set->timeout_work, blk_mq_timeout_work);
>>> [ ... ]
>>> --- a/include/linux/blk-mq.h
>>> +++ b/include/linux/blk-mq.h
>>> @@ -86,6 +86,8 @@ struct blk_mq_tag_set {
>>>  
>>>  	struct blk_mq_tags	**tags;
>>>  
>>> +	struct timer_list	timer;
>>> +	struct work_struct	timeout_work;
>> Can the timer and timeout_work data structures be replaced by a single
>> delayed_work instance?
> I think so. I wanted to keep blk_add_timer relatively unchanged for this
> proposal, so I followed the existing pattern with the timer kicking the
> work. I don't see why that extra indirection is necessary, so I think
> it's a great idea. Unless anyone knows a reason not to, we can collapse
> this into a single delayed work for both mq and legacy as a prep patch
> before this one.

mod_delayed_work_on is very tricky in our scenario. It will grab the pending
work entry and queue it again.

delayed_work.timer trigger
queue_work timeout_work          delayed_work.timer not pending
                                 mod_delayed_work_on
                                   grab the pending timeout_work
                                 re-arm the timer

The timeout_work would not be run.

Thanks
Jianchao