nvme/pci: Poll CQ on timeout
PATCH 1/1 nvme/pci: Poll CQ on timeout
If an IO timeout occurs, it's helpful to know if the controller did not post a completion or the driver missed an interrupt. While we never expect the latter, this patch will make it possible to tell the difference so we don't have to guess.
--- drivers/nvme/host/pci.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
drivers/nvme/host/pci.c +18 -3
@@ -745,10 +745,8 @@ static irqreturn_t nvme_irq_check(int irq, void *data)return IRQ_NONE ; } -static int nvme_poll (struct blk_mq_hw_ctx *hctx,unsigned int tag) +static int __nvme_poll (struct nvme_queue *nvmeq,unsigned int tag) { -struct nvme_queue *nvmeq = hctx->driver_data; -if (nvme_cqe_valid (nvmeq, nvmeq->cq_head, nvmeq->cq_phase)) {spin_lock_irq (&nvmeq->q_lock);__nvme_process_cq (nvmeq, &tag);
@@ -761,6 +759,13 @@ static int nvme_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)return 0 ; } +static int nvme_poll (struct blk_mq_hw_ctx *hctx,unsigned int tag) +{ +struct nvme_queue *nvmeq = hctx->driver_data; + +return __nvme_poll (nvmeq, tag); +} +static void nvme_pci_submit_async_event (struct nvme_ctrl *ctrl,int aer_idx) {struct nvme_dev *dev =to_nvme_dev (ctrl);
@@ -859,6 +864,16 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)struct nvme_command cmd;/* +* Did we miss an interrupt? +*/ +if (__nvme_poll (nvmeq, req->tag)) { +dev_warn (dev->ctrl.device, +"I/O %d QID %d timeout, completion polled\n" , + req->tag, nvmeq->qid); +return BLK_EH_HANDLED ; + } + +/* * Shutdown immediately if controller times out while starting. The * reset work will see the pci device disabled when it gets the forced * cancellation error. All outstanding requests are completed on
Signature · 1 line
2.5.5
Sagi Grimberg
maintainerPATCH 1/1 nvme/pci: Poll CQ on timeout
Quoted 4 lines
If an IO timeout occurs, it's helpful to know if the controller did not post a completion or the driver missed an interrupt. While we never expect the latter, this patch will make it possible to tell the difference so we don't have to guess.
Did you really got a missed interrupt?
It seems reasonable, I'm just curious...
Christoph Hellwig
maintainerPATCH 1/1 nvme/pci: Poll CQ on timeout
In reply to Keith BuschQuoted 4 lines · Keith Busch
If an IO timeout occurs, it's helpful to know if the controller did not post a completion or the driver missed an interrupt. While we never expect the latter, this patch will make it possible to tell the difference so we don't have to guess.
Do you have any good real use case for it? I mostly don't like it becuase it ties us to polling for a specific tag, something I'd like to change in the ->poll API.
PATCH 1/1 nvme/pci: Poll CQ on timeout
Quoted 9 lines · Christoph Hellwig
Quoted 4 lines · Keith Busch
If an IO timeout occurs, it's helpful to know if the controller did not post a completion or the driver missed an interrupt. While we never expect the latter, this patch will make it possible to tell the difference so we don't have to guess.
Do you have any good real use case for it? I mostly don't like it becuase it ties us to polling for a specific tag, something I'd like to change in the ->poll API.
I don't expect this to often catch anything, but this is a cheap way of constraining the problem just by having this in place: the "Timeout" message definitely means the device did not post an entry on that command's completion queue, so either the device is broken or we messed up the queue mapping.
In the event it does trigger (I've seen this only a handlful of times on new platforms and devices, as well as legacy IRQs), we know a whole lot more about the problem, compared to just seeing "Timeout".
If you want to remove the tag specific polling, I can look into an alternative.
PATCH 1/1 nvme/pci: Poll CQ on timeout
Quoted 17 lines · Keith Busch
Quoted 9 lines · Christoph Hellwig
Quoted 4 lines · Keith Busch
If an IO timeout occurs, it's helpful to know if the controller did not post a completion or the driver missed an interrupt. While we never expect the latter, this patch will make it possible to tell the difference so we don't have to guess.
Do you have any good real use case for it? I mostly don't like it becuase it ties us to polling for a specific tag, something I'd like to change in the ->poll API.
I don't expect this to often catch anything, but this is a cheap way of constraining the problem just by having this in place: the "Timeout" message definitely means the device did not post an entry on that command's completion queue, so either the device is broken or we messed up the queue mapping.
Quoted 6 lines
In the event it does trigger (I've seen this only a handlful of times on new platforms and devices, as well as legacy IRQs), we know a whole lot more about the problem, compared to just seeing "Timeout".
If you want to remove the tag specific polling, I can look into an alternative.
IMHO that can go at a later time, if we do remove polling for specific entries. For now it's fine.
And I do think this is a nice addition - it's free, and it provides us extra info for debugging an issue. That's a big deal, especially if it's a user report.
Signature · 1 line
Jens Axboe
PATCH 1/1 nvme/pci: Poll CQ on timeout
Quoted 6 lines · Jens Axboe
IMHO that can go at a later time, if we do remove polling for specific entries. For now it's fine.
And I do think this is a nice addition - it's free, and it provides us extra info for debugging an issue. That's a big deal, especially if it's a user report.
JFYI, we did have I/O timeout issues on one of our internal systems. Keith suggested to give this patch a shot and indeed it cought missed IRQs, which after being polled at least didn't result in filesystem errors.
So if you want you can have my:
Tested-by: Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 1/1 nvme/pci: Poll CQ on timeout
Quoted 8 lines · Johannes Thumshirn
Quoted 6 lines · Jens Axboe
IMHO that can go at a later time, if we do remove polling for specific entries. For now it's fine.
And I do think this is a nice addition - it's free, and it provides us extra info for debugging an issue. That's a big deal, especially if it's a user report.
Quoted 7 lines
JFYI, we did have I/O timeout issues on one of our internal systems. Keith suggested to give this patch a shot and indeed it cought missed IRQs, which after being polled at least didn't result in filesystem errors.
So if you want you can have my:
Tested-by: Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
Thanks, I forgot about this one, but still think it's a good idea. Applied it to for-next.
PATCH 1/1 nvme/pci: Poll CQ on timeout
Quoted 13 lines · Keith Busch
Quoted 12 lines · Johannes Thumshirn
Quoted 6 lines · Jens Axboe
IMHO that can go at a later time, if we do remove polling for specific entries. For now it's fine.
And I do think this is a nice addition - it's free, and it provides us extra info for debugging an issue. That's a big deal, especially if it's a user report.
JFYI, we did have I/O timeout issues on one of our internal systems. Keith suggested to give this patch a shot and indeed it cought missed IRQs, which after being polled at least didn't result in filesystem errors.
Quoted 6 lines
Quoted 3 lines
So if you want you can have my:
Tested-by: Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>Thanks, I forgot about this one, but still think it's a good idea. Applied it to for-next.
Agree, it's a nice addition.
Signature · 1 line
Jens Axboe
Sagi Grimberg
maintainerPATCH 1/1 nvme/pci: Poll CQ on timeout
Quoted 5 lines
Quoted 3 lines
Thanks, I forgot about this one, but still think it's a good idea. Applied it to for-next.
Agree, it's a nice addition.
I think its harmless to have. especially when we do log that it happened.
PATCH 1/1 nvme/pci: Poll CQ on timeout
Quoted 8 lines · Sagi Grimberg
Quoted 5 lines
Quoted 3 lines
Thanks, I forgot about this one, but still think it's a good idea. Applied it to for-next.
Agree, it's a nice addition.
I think its harmless to have. especially when we do log that it happened.
It's actually proved to be quite useful and maybe good for stable. Any objections to roll this into 4.11 instead 4.12?
PATCH 1/1 nvme/pci: Poll CQ on timeout
Quoted 5 lines · Keith Busch
Quoted 4 lines · Sagi Grimberg
Quoted 4 lines
Quoted 3 lines
Thanks, I forgot about this one, but still think it's a good idea. Applied it to for-next.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Agree, it's a nice addition.
I think its harmless to have. especially when we do log that it happened.
It's actually proved to be quite useful and maybe good for stable. Any objections to roll this into 4.11 instead 4.12?
I'll object to that, we should not queue up anything for 4.11 that isn't potentially fixing a major issue at this point.
Signature · 1 line
Jens Axboe
Christoph Hellwig
maintainerPATCH 1/1 nvme/pci: Poll CQ on timeout
Quoted 8 lines · Jens Axboe
Quoted 7 lines · Keith Busch
Quoted 6 lines · Sagi Grimberg
Quoted 5 lines
Quoted 3 lines
Thanks, I forgot about this one, but still think it's a good idea. Applied it to for-next.
Agree, it's a nice addition.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
I think its harmless to have. especially when we do log that it happened.
It's actually proved to be quite useful and maybe good for stable. Any objections to roll this into 4.11 instead 4.12?
I'll object to that, we should not queue up anything for 4.11 that isn't potentially fixing a major issue at this point.
Agreed. I'll send it to you for 4.12 with the nvme pull request later today.