| Message ID | 20180521231131.6685-3-keith.busch@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 21, 2018 at 05:11:30PM -0600, Keith Busch wrote: > The block layer had been setting the state to in-flight prior to updating > the timer. This is the wrong order since the timeout handler could observe > the in-flight state with the older timeout, believing the request had > expired when in fact it is just getting started. > > Signed-off-by: Keith Busch <keith.busch@intel.com> > --- > block/blk-mq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 8b370ed75605..66e5c768803f 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -713,8 +713,8 @@ void blk_mq_start_request(struct request *rq) > preempt_disable(); > write_seqcount_begin(&rq->gstate_seq); > > - blk_mq_rq_update_state(rq, MQ_RQ_IN_FLIGHT); > blk_add_timer(rq); > + blk_mq_rq_update_state(rq, MQ_RQ_IN_FLIGHT); > > write_seqcount_end(&rq->gstate_seq); > preempt_enable(); > -- > 2.14.3 > Looks fine, Reviewed-by: Ming Lei <ming.lei@redhat.com> Thanks, Ming
On Mon, May 21, 2018 at 05:11:30PM -0600, Keith Busch wrote: > The block layer had been setting the state to in-flight prior to updating > the timer. This is the wrong order since the timeout handler could observe > the in-flight state with the older timeout, believing the request had > expired when in fact it is just getting started. > > Signed-off-by: Keith Busch <keith.busch@intel.com> The way I understood Barts comments to my comments on his patch we actually need the two updates to be atomic. I haven't had much time to follow up, but I'd like to hear Barts opinion. Either way we clearly need to document our assumptions here in comments.
On Tue, 2018-05-22 at 17:24 +0200, Christoph Hellwig wrote: > On Mon, May 21, 2018 at 05:11:30PM -0600, Keith Busch wrote: > > The block layer had been setting the state to in-flight prior to updating > > the timer. This is the wrong order since the timeout handler could observe > > the in-flight state with the older timeout, believing the request had > > expired when in fact it is just getting started. > > > > Signed-off-by: Keith Busch <keith.busch@intel.com> > > The way I understood Barts comments to my comments on his patch we > actually need the two updates to be atomic. I haven't had much time > to follow up, but I'd like to hear Barts opinion. Either way we > clearly need to document our assumptions here in comments. After we discussed request state updating I have been thinking further about this. I think now that it is safe to update the request deadline first because the timeout code ignores requests anyway that have another state than MQ_RQ_IN_FLIGHT. Additionally, it is unlikely that the request timer will fire before the request state has been updated. And if that would happen the request timeout will be handled the next time request timeouts are examined. Bart.
diff --git a/block/blk-mq.c b/block/blk-mq.c index 8b370ed75605..66e5c768803f 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -713,8 +713,8 @@ void blk_mq_start_request(struct request *rq) preempt_disable(); write_seqcount_begin(&rq->gstate_seq); - blk_mq_rq_update_state(rq, MQ_RQ_IN_FLIGHT); blk_add_timer(rq); + blk_mq_rq_update_state(rq, MQ_RQ_IN_FLIGHT); write_seqcount_end(&rq->gstate_seq); preempt_enable();
The block layer had been setting the state to in-flight prior to updating the timer. This is the wrong order since the timeout handler could observe the in-flight state with the older timeout, believing the request had expired when in fact it is just getting started. Signed-off-by: Keith Busch <keith.busch@intel.com> --- block/blk-mq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)