Skip to contentSkip to conversation

NVMe: Increase shutdown complete time

PATCH New Keith Busch 6 participants 12 messages Mar 31, 2014 – Apr 4, 2014

Subsystems (2)
Block layer NVMe
lore
Share & tools
lore

Keith Busch

author
#0 lore raw

PATCH 1/1 NVMe: Increase shutdown complete time

The spec doesn't have a recommendation for shutdown beyond "that the host wait a minimum of one second for the shutdown operations to complete", so we need to choose an arbitrarily value so we don't wait forever but high enough to prevent unsafe shutdowns. Some h/w vendors say the previous two seconds is not long enough at some capacities. Twenty seconds ought to be enough for anybody, right?

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/block/nvme-core.c +1 -1
index 625259d..103da93 100644
@@ -1352,7 +1352,7 @@ static int nvme_shutdown_ctrl(struct nvme_dev *dev)
 	cc = (readl(&dev->bar->cc) & ~NVME_CC_SHN_MASK) | NVME_CC_SHN_NORMAL;
 	writel(cc, &dev->bar->cc);
 
-	timeout = 2 * HZ + jiffies;
+	timeout = 20 * HZ + jiffies;
 	while ((readl(&dev->bar->csts) & NVME_CSTS_SHST_MASK) !=
 							NVME_CSTS_SHST_CMPLT) {
 		msleep(100);
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 1/1 NVMe: Increase shutdown complete time

Seems reasonable to me.

Quoted 34 lines · Keith Busch

The spec doesn't have a recommendation for shutdown beyond "that the host wait a minimum of one second for the shutdown operations to complete", so we need to choose an arbitrarily value so we don't wait forever but high enough to prevent unsafe shutdowns. Some h/w vendors say the previous two seconds is not long enough at some capacities. Twenty seconds ought to be enough for anybody, right?

Signed-off-by Keith Busch <keith.busch at intel.com>

---
drivers/block/nvme-core.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)
drivers/block/nvme-core.c +1 -1
index 625259d..103da93 100644
@@ -1352,7 +1352,7 @@ static int nvme_shutdown_ctrl(struct nvme_dev *dev)
 	cc = (readl(&dev->bar->cc) & ~NVME_CC_SHN_MASK) | NVME_CC_SHN_NORMAL;
 	writel(cc, &dev->bar->cc);
 
-	timeout = 2 * HZ + jiffies;
+	timeout = 20 * HZ + jiffies;
 	while ((readl(&dev->bar->csts) & NVME_CSTS_SHST_MASK) !=
 							NVME_CSTS_SHST_CMPLT) {
 		msleep(100);
Signature · 7 lines
1.7.10.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme
#2 lore raw

PATCH 1/1 NVMe: Increase shutdown complete time

You're probably fine waiting anywhere up to CAP.TO.

-----Original Message-----
From: Linux-nvme [mailto:[email protected]] On Behalf Of Dan McLeran
Sent: Monday, March 31, 2014 11:57 AM
To: Busch, Keith
Cc: linux-nvme at lists.infradead.org
Subject: Re: [PATCH] NVMe: Increase shutdown complete time

Seems reasonable to me.

Quoted 29 lines · Keith Busch

The spec doesn't have a recommendation for shutdown beyond "that the host wait a minimum of one second for the shutdown operations to complete", so we need to choose an arbitrarily value so we don't wait forever but high enough to prevent unsafe shutdowns. Some h/w vendors say the previous two seconds is not long enough at some capacities. Twenty seconds ought to be enough for anybody, right?

Signed-off-by Keith Busch <keith.busch at intel.com>

---
drivers/block/nvme-core.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)
drivers/block/nvme-core.c +1 -2
index 625259d..103da93 100644
@@ -1352,7 +1352,7 @@ static int nvme_shutdown_ctrl(struct nvme_dev *dev)
 	cc = (readl(&dev->bar->cc) & ~NVME_CC_SHN_MASK) | NVME_CC_SHN_NORMAL;
 	writel(cc, &dev->bar->cc);
 
-	timeout = 2 * HZ + jiffies;
+	timeout = 20 * HZ + jiffies;
 	while ((readl(&dev->bar->csts) & NVME_CSTS_SHST_MASK) !=
 							NVME_CSTS_SHST_CMPLT) {
 		msleep(100);
--

1.7.10.4

Quoted 5 lines

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme

#3 lore raw

PATCH 1/1 NVMe: Increase shutdown complete time

CAP.TO is defined as the worst case time the driver should wait for the controller to come ready. Not sure it can be used for shutdown as well. If so, then we should just read that value and use it as we do in nvme_wait_ready.

Quoted 53 lines · Raymond C

You're probably fine waiting anywhere up to CAP.TO.

-----Original Message-----
From: Linux-nvme [mailto:linux-nvme-bounces at lists.infradead.org] On Behalf Of Dan McLeran
Sent: Monday, March 31, 2014 11:57 AM
To: Busch, Keith
Cc: linux-nvme at lists.infradead.org
Subject: Re: [PATCH] NVMe: Increase shutdown complete time

Seems reasonable to me.

Quoted 29 lines · Keith Busch

The spec doesn't have a recommendation for shutdown beyond "that the host wait a minimum of one second for the shutdown operations to complete", so we need to choose an arbitrarily value so we don't wait forever but high enough to prevent unsafe shutdowns. Some h/w vendors say the previous two seconds is not long enough at some capacities. Twenty seconds ought to be enough for anybody, right?

Signed-off-by Keith Busch <keith.busch at intel.com>

---
drivers/block/nvme-core.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)
drivers/block/nvme-core.c +1 -2
index 625259d..103da93 100644
@@ -1352,7 +1352,7 @@ static int nvme_shutdown_ctrl(struct nvme_dev *dev)
 	cc = (readl(&dev->bar->cc) & ~NVME_CC_SHN_MASK) | NVME_CC_SHN_NORMAL;
 	writel(cc, &dev->bar->cc);
 
-	timeout = 2 * HZ + jiffies;
+	timeout = 20 * HZ + jiffies;
 	while ((readl(&dev->bar->csts) & NVME_CSTS_SHST_MASK) !=
 							NVME_CSTS_SHST_CMPLT) {
 		msleep(100);
--

1.7.10.4

Quoted 5 lines

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme

#4 lore raw

PATCH 1/1 NVMe: Increase shutdown complete time

Hi, all:

Another issue we encountered is I/O timeout. I believe it is set to 5 seconds in the current driver. When the system has a lot of queues, each queue has a lot of entries, and each entry is a large size request, it is very easy to exceed this number during benchmarking. Take an example, if an NVMe card supports 64 queues, 1K entries per queue and 1MB per request, there are about 64GB data ahead of a request when it is put to the queue. The timer starts to tick. If the card can process 4GB/s, it still takes 16 seconds to process earlier requests. As a result, the request will receive a timeout (EIO5) from the driver. I understand in real application this is unlikely to happen. But it is not uncommon during benchmarking.

Why do we choose 5 seconds? Are we able to make this parameter configurable? Thanks.

Yung-Chin Chen
Greenliant Systems

-----Original Message-----
From: Linux-nvme [mailto:[email protected]] On Behalf Of Dan McLeran
Sent: Monday, March 31, 2014 4:57 PM
To: Robles, Raymond C
Cc: Busch, Keith; linux-nvme at lists.infradead.org; Mcleran, Daniel
Subject: RE: [PATCH] NVMe: Increase shutdown complete time

CAP.TO is defined as the worst case time the driver should wait for the controller to come ready. Not sure it can be used for shutdown as well. If so, then we should just read that value and use it as we do in nvme_wait_ready.

Quoted 28 lines · Raymond C

You're probably fine waiting anywhere up to CAP.TO.

-----Original Message-----
From: Linux-nvme [mailto:linux-nvme-bounces at lists.infradead.org] On Behalf Of Dan McLeran
Sent: Monday, March 31, 2014 11:57 AM
To: Busch, Keith
Cc: linux-nvme at lists.infradead.org
Subject: Re: [PATCH] NVMe: Increase shutdown complete time

Seems reasonable to me.

Quoted 14 lines · Keith Busch

The spec doesn't have a recommendation for shutdown beyond "that the host wait a minimum of one second for the shutdown operations to complete", so we need to choose an arbitrarily value so we don't wait

forever but high enough to prevent unsafe shutdowns. Some h/w vendors

say the previous two seconds is not long enough at some capacities. Twenty seconds ought to be enough for anybody, right?

Signed-off-by Keith Busch <keith.busch at intel.com>

---
drivers/block/nvme-core.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 5 lines

drivers/block/nvme-core.c static int nvme_shutdown_ctrl(struct nvme_dev

Quoted 5 lines
drivers/block/nvme-core.c +0 -0
index 625259d..103da93 100644
@@ -1352,7 +1352,7 @@ static int nvme_shutdown_ctrl(struct nvme_dev

*dev)

Quoted 1 line

drivers/block/nvme-core.c static int nvme_shutdown_ctrl(struct nvme_dev · line 1352

Quoted 1 line
drivers/block/nvme-core.c +0 -0
 	cc = (readl(&dev->bar->cc) & ~NVME_CC_SHN_MASK) |

NVME_CC_SHN_NORMAL;

Quoted 6 lines

drivers/block/nvme-core.c static int nvme_shutdown_ctrl(struct nvme_dev *dev)

Quoted 6 lines
drivers/block/nvme-core.c +1 -1
 	writel(cc, &dev->bar->cc);
 
-	timeout = 2 * HZ + jiffies;
+	timeout = 20 * HZ + jiffies;
 	while ((readl(&dev->bar->csts) & NVME_CSTS_SHST_MASK) !=

NVME_CSTS_SHST_CMPLT) {

Quoted 11 lines
Quoted 10 lines
drivers/block/nvme-core.c +0 -1
 		msleep(100);
--

1.7.10.4

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme

Quoted 5 lines

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme

author
#5 lore raw

PATCH 1/1 NVMe: Increase shutdown complete time

Quoted 2 lines · Yung-Chin Chen

Why do we choose 5 seconds? Are we able to make this parameter configurable? Thanks.

This is the second time I've been asked this in two days. :)

Any thoughts on something like the following?

drivers/block/nvme-core.c +4 -0
index 59e2adcc..6ade8de 100644
@@ -51,6 +51,10 @@
  #define ADMIN_TIMEOUT	(60 * HZ)
 
+int nvme_io_timeout = 5;
+module_param(nvme_io_timeout, int, 0);
+MODULE_PARM_DESC(nvme_io_timeout, "timeout in seconds for io submitted to queue");
+
  static int nvme_major;
  module_param(nvme_major, int, 0);
include/linux/nvme.h +2 -1
index 5993455..490488e 100644
@@ -66,7 +66,8 @@ enum {
 
  #define NVME_VS(major, minor)	(major << 16 | minor)
 
-#define NVME_IO_TIMEOUT	(5 * HZ)
+extern int nvme_io_timeout;
+#define NVME_IO_TIMEOUT	(nvme_io_timeout * HZ)
 
  /*
   * Represents an NVM Express device.  Each nvme_dev is a PCI function.
#6 lore raw

PATCH 1/1 NVMe: Increase shutdown complete time

Thanks, Keith, for your prompt response. I do not know Linux driver that well. If you use MODULE_PARM_DESC(), how do I specify the parameter for bootable NVMe device?

For example, RedHat 7.0 has built-in NVMe driver and supports bootable from an NVMe device. The driver is buil-in to the kernel, and is not a loadable module. Where can I specify the parameters for built-in driver? Thanks.

Yung-Chin Chen
Greenliant Systems

-----Original Message-----
From: Keith Busch [mailto:[email protected]]
Sent: Thursday, April 03, 2014 4:13 PM
To: Yung-Chin Chen
Cc: Dan McLeran; Robles, Raymond C; Busch, Keith; linux-nvme at lists.infradead.org
Subject: RE: [PATCH] NVMe: Increase shutdown complete time

Quoted 2 lines · Yung-Chin Chen

Why do we choose 5 seconds? Are we able to make this parameter configurable? Thanks.

This is the second time I've been asked this in two days. :)

Any thoughts on something like the following?

drivers/block/nvme-core.c index renamed from drivers/block/nvme-core.c +0 -0

59e2adcc..6ade8de 100644

drivers/block/nvme-core.c +5 -0
@@ -51,6 +51,10 @@
  #define ADMIN_TIMEOUT	(60 * HZ)
 
+int nvme_io_timeout = 5;
+module_param(nvme_io_timeout, int, 0);
+MODULE_PARM_DESC(nvme_io_timeout, "timeout in seconds for io submitted 
+to queue");
+
  static int nvme_major;
  module_param(nvme_major, int, 0);
include/linux/nvme.h index renamed from include/linux/nvme.h +0 -0

5993455..490488e 100644

include/linux/nvme.h +2 -1
@@ -66,7 +66,8 @@ enum {
 
  #define NVME_VS(major, minor)	(major << 16 | minor)
 
-#define NVME_IO_TIMEOUT	(5 * HZ)
+extern int nvme_io_timeout;
+#define NVME_IO_TIMEOUT	(nvme_io_timeout * HZ)
 
  /*
   * Represents an NVM Express device.  Each nvme_dev is a PCI function.
author
#7 lore raw

PATCH 1/1 NVMe: Increase shutdown complete time

Quoted 7 lines · Yung-Chin Chen

Thanks, Keith, for your prompt response. I do not know Linux driver that well. If you use MODULE_PARM_DESC(), how do I specify the parameter for bootable NVMe device?

For example, RedHat 7.0 has built-in NVMe driver and supports bootable from an NVMe device. The driver is buil-in to the kernel, and is not a loadable module. Where can I specify the parameters for built-in driver?

Okay, I'm not overly familiar with RHEL7, but I believe the following is the way. If the module is built in, we can specify module parameters at the vmlinuz line from the boot loader. To make it permanent, you can append:

<module>.<module_param>=<value>

to the GRUB_CMDLINE_LINUX line in /etc/default/grub. The proposed new parameter would look like 'nvme.nvme_io_timeout=20' if you want a 20 second timeout. After you set it, save the file, then run:

   grub2-mkconfig --output=/boot/efi/EFI/redhat/grub.cfg

I assume you're efi booting since you mention booting from NVMe. If not efi, the output would be in /boot/grub2/grub.cfg. The driver should then use the new timeout value by default on each reboot after that.

#8 lore raw

PATCH 1/1 NVMe: Increase shutdown complete time

In reply to Keith Busch

Probably want to use ulong if we're going to make this a module param.

Quoted 38 lines · Keith Busch
Quoted 2 lines · Yung-Chin Chen

Why do we choose 5 seconds? Are we able to make this parameter configurable? Thanks.

This is the second time I've been asked this in two days. :)

Any thoughts on something like the following?

drivers/block/nvme-core.c +4 -0
index 59e2adcc..6ade8de 100644
@@ -51,6 +51,10 @@
 #define ADMIN_TIMEOUT	(60 * HZ)
 
+int nvme_io_timeout = 5;
+module_param(nvme_io_timeout, int, 0);
+MODULE_PARM_DESC(nvme_io_timeout, "timeout in seconds for io submitted to 
 queue");
+
 static int nvme_major;
 module_param(nvme_major, int, 0);
include/linux/nvme.h +2 -1
index 5993455..490488e 100644
@@ -66,7 +66,8 @@ enum {
 
 #define NVME_VS(major, minor)	(major << 16 | minor)
 
-#define NVME_IO_TIMEOUT	(5 * HZ)
+extern int nvme_io_timeout;
+#define NVME_IO_TIMEOUT	(nvme_io_timeout * HZ)
 
 /*
 * Represents an NVM Express device.  Each nvme_dev is a PCI function.
#9 lore raw

PATCH 1/1 NVMe: Increase shutdown complete time

In reply to Keith Busch

drivers/block/nvme-core.c line 55

Quoted 3 lines · Keith Busch
drivers/block/nvme-core.c +3 -0
+int nvme_io_timeout = 5;
+module_param(nvme_io_timeout, int, 0);
+MODULE_PARM_DESC(nvme_io_timeout, "timeout in seconds for io submitted to queue");

Any reason not to make this module_param 0644? That would allow it to be written by root and read by anybody.

#10 lore raw

PATCH 1/1 NVMe: Increase shutdown complete time

In reply to Keith Busch

How do I have someone in Samsung added to this email distribution list?

Thanks,

Laura

-----Original Message-----
From: Linux-nvme [mailto:[email protected]] On Behalf Of Keith Busch
Sent: Monday, March 31, 2014 9:24 AM
To: linux-nvme at lists.infradead.org
Cc: Keith Busch
Subject: [PATCH] NVMe: Increase shutdown complete time

The spec doesn't have a recommendation for shutdown beyond "that the host wait a minimum of one second for the shutdown operations to complete", so we need to choose an arbitrarily value so we don't wait forever but high enough to prevent unsafe shutdowns. Some h/w vendors say the previous two seconds is not long enough at some capacities. Twenty seconds ought to be enough for anybody, right?

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/block/nvme-core.c +1 -1
@@ -1352,7 +1352,7 @@ static int nvme_shutdown_ctrl(struct nvme_dev *dev)
 	cc = (readl(&dev->bar->cc) & ~NVME_CC_SHN_MASK) | NVME_CC_SHN_NORMAL;
 	writel(cc, &dev->bar->cc);
 
-	timeout = 2 * HZ + jiffies;
+	timeout = 20 * HZ + jiffies;
 	while ((readl(&dev->bar->csts) & NVME_CSTS_SHST_MASK) !=
 							NVME_CSTS_SHST_CMPLT) {
 		msleep(100);

--
1.7.10.4

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme