[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4797ba1736ce53e799a799846704a53af05d2035.camel@wdc.com>
Date: Wed, 16 May 2018 18:06:16 +0000
From: Bart Van Assche <Bart.VanAssche@....com>
To: "hch@....de" <hch@....de>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
"israelr@...lanox.com" <israelr@...lanox.com>,
"sagi@...mberg.me" <sagi@...mberg.me>,
"sebott@...ux.ibm.com" <sebott@...ux.ibm.com>,
"ming.lei@...hat.com" <ming.lei@...hat.com>,
"axboe@...nel.dk" <axboe@...nel.dk>,
"jianchao.w.wang@...cle.com" <jianchao.w.wang@...cle.com>,
"maxg@...lanox.com" <maxg@...lanox.com>,
"tj@...nel.org" <tj@...nel.org>
Subject: Re: [PATCH v9 2/2] blk-mq: Rework blk-mq timeout handling again
On Wed, 2018-05-16 at 19:31 +0200, hch@....de wrote:
> On Wed, May 16, 2018 at 04:47:54PM +0000, Bart Van Assche wrote:
> > I think your patch changes the order of changing the request state and
> > calling mod_timer(). In my patch the request state and the deadline are
> > updated first and mod_timer() is called afterwards. I think your patch
> > changes the order of these operations into the following:
> > (1) __blk_mq_start_request() sets the request deadline.
> > (2) __blk_mq_start_request() calls __blk_add_timer() which in turn calls
> > mod_timer().
> > (3) __blk_mq_start_request() changes the request state into MQ_RQ_IN_FLIGHT.
> >
> > In the unlikely event of a significant delay between (2) and (3) it can
> > happen that the timer fires and examines and ignores the request because
> > its state differs from MQ_RQ_IN_FLIGHT. If the request for which this
> > happened times out its timeout will only be handled the next time
> > blk_mq_timeout_work() is called. Is this the behavior you intended?
>
> We can move the timer manipulation after the change easily I think.
> It would make sense to add comments explaining the ordering.
Hello Christoph,
I'm afraid that could lead to mod_timer() being called in another order than
intended. If e.g. the code that handles BLK_EH_RESET_TIMER changes the request
state first to in-flight and next calls mod_timer() then it can happen that
another context completes and restarts the request, resulting in a concurrent
mod_timer() call. I'm not sure reordering of the mod_timer() calls would result
in correct behavior.
Bart.
Powered by blists - more mailing lists