From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 23 May 2018 08:34:48 +0800 From: Ming Lei To: Christoph Hellwig Cc: Keith Busch , Jens Axboe , linux-nvme@lists.infradead.org, linux-block@vger.kernel.org, Bart Van Assche Subject: Re: [RFC PATCH 3/3] blk-mq: Remove generation seqeunce Message-ID: <20180523003446.GC31196@ming.t460p> References: <20180521231131.6685-1-keith.busch@intel.com> <20180521231131.6685-4-keith.busch@intel.com> <20180522161704.GA20000@lst.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <20180522161704.GA20000@lst.de> List-ID: 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: ming.lei@redhat.com (Ming Lei) Date: Wed, 23 May 2018 08:34:48 +0800 Subject: [RFC PATCH 3/3] blk-mq: Remove generation seqeunce In-Reply-To: <20180522161704.GA20000@lst.de> References: <20180521231131.6685-1-keith.busch@intel.com> <20180521231131.6685-4-keith.busch@intel.com> <20180522161704.GA20000@lst.de> Message-ID: <20180523003446.GC31196@ming.t460p> On Tue, May 22, 2018@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