On Tue, May 22, 2018 at 10:46 PM, Keith Busch <keith.busch@xxxxxxxxxxxxxxx> wrote: > On Tue, May 22, 2018 at 10:37:32PM +0800, Ming Lei wrote: >> On Tue, May 22, 2018 at 10:20 PM, Keith Busch >> <keith.busch@xxxxxxxxxxxxxxx> wrote: >> > In the event the driver requests a normal completion, the timeout work >> > releasing the last reference doesn't do a second completion: it only >> >> The reference only covers request's lifetime, not related with completion. >> >> It isn't the last reference. When driver returns EH_HANDLED, blk-mq >> will complete this request on extra time. >> >> Yes, if driver's timeout code and normal completion code can sync >> about this completion, that should be fine, but the current behaviour >> doesn't depend driver's sync since the req is always completed atomically >> via the following way: >> >> 1) timeout >> >> if (mark_completed(rq)) >> timed_out(rq) >> >> 2) normal completion >> if (mark_completed(rq)) >> complete(rq) >> >> For example, before nvme_timeout() is trying to run nvme_dev_disable(), >> irq comes and this req is completed from normal completion path, but >> nvme_timeout() still returns EH_HANDLED, and blk-mq may complete >> the req one extra time since the normal completion path may not update >> req's state yet. > > nvme_dev_disable tears down irq's, meaing their handling is already > sync'ed before nvme_dev_disable can proceed. Whether the completion > comes through nvme_irq, or through nvme_dev_disable, there is no way > possible for nvme's timeout to return EH_HANDLED if the state was not > updated prior to returning that status. OK, that still depends on driver's behaviour, even though it is true for NVMe, you still have to audit other drivers about this sync because it is required by your patch. thanks, Ming Lei
