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
