Re: [RFC PATCH 3/3] blk-mq: Remove generation seqeunce

Ming Lei
2018-05-23T00:34:48Z

On Tue, May 22, 2018 at 06:17:04PM +0200, Christoph Hellwig wrote:
> Hi Keith,
>
> I like this series a lot. One comment that is probably close
> to the big discussion in the thread:
>
> > switch (ret) {
> > case BLK_EH_HANDLED:
> > /*
> > + * If the request is still in flight, the driver is requesting
> > + * blk-mq complete it.
> > */
> > + if (blk_mq_rq_state(req) == MQ_RQ_IN_FLIGHT)
> > + __blk_mq_complete_request(req);
> > + break;
>
> The state check here really irked me, and from the thread it seems like
> I'm not the only one. At least for the NVMe case I think it is perfectly
> safe, although I agree I'd rather audit what other drivers do carefully.

Let's consider the normal NVMe timeout code path:

1) one request is timed out;

2) controller is shutdown, this timed-out request is requeued from
nvme_cancel_request(), but can't dispatch because queues are quiesced

3) reset is done from another context, and this request is dispatched
again, and completed exactly before returning EH_HANDLED to blk-mq, but
its state isn't updated to COMPLETE yet.

4) then double completions are done from both normal completion and timeout
path.

Seems same issue exists on poll path.

Thanks,
Ming