nvme: Limit command retries
PATCH 1/1 nvme: Limit command retries
Many controller implementations will return errors to commands that will not succeed, but without the DNR bit set. The driver previously retried these commands an unlimited number of times until the command timeout has exceeded, which takes an unnecessarilly long period of time.
This patch limits the number of retries a command can have, defaulting to 5, but is user tunable at load or runtime.
The struct request's 'retries' field is used to track the number of retries attempted. This is in contrast with scsi's use of this field, which indicates how many retries are allowed.
--- drivers/nvme/host/core.c | 5 +++++ drivers/nvme/host/nvme.h | 5 ++++- drivers/nvme/host/pci.c | 6 ++++++ 3 files changed, 15 insertions(+), 1 deletion(-)
drivers/nvme/host/core.c +5 -0
@@ -47,6 +47,11 @@ unsigned char shutdown_timeout = 5;module_param (shutdown_timeout, byte,0644 );MODULE_PARM_DESC (shutdown_timeout,"timeout in seconds for controller shutdown" ); +unsigned int nvme_max_retries =5 ; +module_param_named (max_retries, nvme_max_retries, uint,0644 ); +MODULE_PARM_DESC (max_retries,"max number of retries a command may have" ); +EXPORT_SYMBOL_GPL (nvme_max_retries); +static int nvme_major;module_param (nvme_major, int,0 );
drivers/nvme/host/nvme.h +4 -1
@@ -38,6 +38,8 @@ extern unsigned char admin_timeout;extern unsigned char shutdown_timeout;#define SHUTDOWN_TIMEOUT (shutdown_timeout * HZ) +extern unsigned int nvme_max_retries; +enum {NVME_NS_LBA =0 ,NVME_NS_LIGHTNVM =1 ,
@@ -204,7 +206,8 @@ static inline int nvme_error_status(u16 status)static inline bool nvme_req_needs_retry (struct request *req,u16 status) {return !(status &NVME_SC_DNR ||blk_noretry_request (req)) && - (jiffies - req->start_time) < req->timeout; + (jiffies - req->start_time) < req->timeout && + req->retries < nvme_max_retries; }void nvme_cancel_request (struct request *req,void *data,bool reserved);
drivers/nvme/host/pci.c +6 -0
@@ -310,6 +310,11 @@ static int nvme_init_iod(struct request *rq, unsigned size, iod->npages =-1 ; iod->nents =0 ; iod->length = size; + +if (!(rq->cmd_flags &REQ_DONTPREP )) { + rq->retries =0 ; + rq->cmd_flags |=REQ_DONTPREP ; + }return 0 ; }
@@ -623,6 +628,7 @@ static void nvme_complete_rq(struct request *req)if (unlikely (req->errors)) {if (nvme_req_needs_retry (req, req->errors)) { + req->retries++;nvme_requeue_req (req);return ; }
Signature · 1 line
2.7.2
Sagi Grimberg
maintainerPATCH 1/1 nvme: Limit command retries
Quoted 6 lines
Many controller implementations will return errors to commands that will not succeed, but without the DNR bit set. The driver previously retried these commands an unlimited number of times until the command timeout has exceeded, which takes an unnecessarilly long period of time.
Quoted 6 lines
This patch limits the number of retries a command can have, defaulting to 5, but is user tunable at load or runtime.
The struct request's 'retries' field is used to track the number of retries attempted. This is in contrast with scsi's use of this field, which indicates how many retries are allowed.
Why not follow the same scheme?
Quoted 13 lines
Signed-off-by Keith Busch <keith.busch at intel.com>--- drivers/nvme/host/core.c | 5 +++++ drivers/nvme/host/nvme.h | 5 ++++- drivers/nvme/host/pci.c | 6 ++++++ 3 files changed, 15 insertions(+), 1 deletion(-)drivers/nvme/host/core.c +0
-0
drivers/nvme/host/core.c unsigned char shutdown_timeout = 5; · line 53
Quoted 7 lines
drivers/nvme/host/core.c +4
-0@@ -47,6 +47,11 @@ unsigned char shutdown_timeout = 5;module_param (shutdown_timeout, byte,0644 );MODULE_PARM_DESC (shutdown_timeout,"timeout in seconds for controller shutdown" ); +unsigned int nvme_max_retries =5 ; +module_param_named (max_retries, nvme_max_retries, uint,0644 ); +MODULE_PARM_DESC (max_retries,"max number of retries a command may have" ); +EXPORT_SYMBOL_GPL (nvme_max_retries);
This looks like its a per-controller attribute, it might be better to make nvme-cli pass this via the vendor extensions? This way we can have it on a per-controller basis.
PATCH 1/1 nvme: Limit command retries
Quoted 5 lines · Sagi Grimberg
Quoted 3 lines
The struct request's 'retries' field is used to track the number of retries attempted. This is in contrast with scsi's use of this field, which indicates how many retries are allowed.
Why not follow the same scheme?
It's not documented to mean what scsi thinks it means, and it saves some space from yet another field to track how many retries have occured. For the pci driver, that field could be added to struct nvme_iod, but that's not used for fabrics. Using req->retries can be consistent with all nvme transports.
Quoted 8 lines
drivers/nvme/host/core.c unsigned char shutdown_timeout = 5; · line 53
Quoted 4 lines
drivers/nvme/host/core.c +4
-0+unsigned int nvme_max_retries =5 ; +module_param_named (max_retries, nvme_max_retries, uint,0644 ); +MODULE_PARM_DESC (max_retries,"max number of retries a command may have" ); +EXPORT_SYMBOL_GPL (nvme_max_retries);This looks like its a per-controller attribute, it might be better to make nvme-cli pass this via the vendor extensions? This way we can have it on a per-controller basis.
If we need per-controller, I'd like a sysfs attribute. nvme-cli mostly does ioctl's, but it also does some sysfs stuff too.
PATCH 1/1 nvme: Limit command retries
In reply to Keith BuschI've got some work in progress that gets rid of the SCSI-specific fields in struct request and moves them into a scsi specific private data structure, and that also does the same for NVMe.
How urgent is this patch? If it fixes an actual major issue seen in the field we should probably but the ->retries use in as-is, otherwise I'd prefer to avoid introducing more users of it for now.
PATCH 1/1 nvme: Limit command retries
Quoted 3 lines · Christoph Hellwig
How urgent is this patch? If it fixes an actual major issue seen in the field we should probably but the ->retries use in as-is, otherwise I'd prefer to avoid introducing more users of it for now.
Cool, thanks. This is not an urgent issue for us, but was hoping to make the 4.8 window. If we can flush something better out, I'm happy to wait.
PATCH 1/1 nvme: Limit command retries
Quoted 7 lines · Keith Busch
Quoted 3 lines · Christoph Hellwig
How urgent is this patch? If it fixes an actual major issue seen in the field we should probably but the ->retries use in as-is, otherwise I'd prefer to avoid introducing more users of it for now.
Cool, thanks. This is not an urgent issue for us, but was hoping to make the 4.8 window. If we can flush something better out, I'm happy to wait.
Given that even the prep patches for that work aren't merged yet they aren't likely to make it for 4.8.
Based on that I think we should get this patch in for the merge window:
PATCH 1/1 nvme: Limit command retries
In reply to Keith BuschQuoted 11 lines · Keith Busch
Many controller implementations will return errors to commands that will not succeed, but without the DNR bit set. The driver previously retried these commands an unlimited number of times until the command timeout has exceeded, which takes an unnecessarilly long period of time.
This patch limits the number of retries a command can have, defaulting to 5, but is user tunable at load or runtime.
The struct request's 'retries' field is used to track the number of retries attempted. This is in contrast with scsi's use of this field, which indicates how many retries are allowed.
Added for 4.8, thanks.
Signature · 1 line
Jens Axboe