Drivers: scsi: FLUSH timeout

DISCUSSION K. Y. Srinivasan 12 participants 19 messages Sep 20, 2013 – Oct 4, 2013 SCSI

Share & tools

K. Y. Srinivasan

author

2013-09-20 18:50 UTC #0 lore raw

The SD_FLUSH_TIMEOUT value is currently hardcoded. On our cloud, we sometimes hit this timeout. I was wondering if we could make this a module parameter. If this is acceptable, I can send you a patch for this.

Regards,

K. Y

Greg KH

2013-09-20 20:31 UTC #1 lore raw

Quoted 1 line · K. Y. Srinivasan

The SD_FLUSH_TIMEOUT value is currently hardcoded.

Hardcoded where? Please, more context.

Quoted 3 lines

On our cloud, we sometimes hit this timeout. I was wondering if we could make this a module parameter. If this is acceptable, I can send you a patch for this.

A module parameter don't make sense for a per-device value, does it?

greg k-h

Laurence Oberman

2013-09-20 21:00 UTC #2 lore raw

I am thinking Srini meant in the sd_mod driver module. #define SD_FLUSH_TIMEOUT (60 * HZ)

Laurence

Quoted 16 lines · Greg KH

Quoted 1 line · K. Y. Srinivasan
The SD_FLUSH_TIMEOUT value is currently hardcoded.
Hardcoded where? Please, more context.
Quoted 3 lines
On our cloud, we sometimes hit this timeout. I was wondering if we could make this a module parameter. If this is acceptable, I can send you a patch for this.
A module parameter don't make sense for a per-device value, does it?
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

KY Srinivasan

author

2013-09-21 05:24 UTC #3 lore raw

In reply to Greg KH

Quoted 11 lines

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, September 20, 2013 1:32 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout

Quoted 4 lines

Quoted 1 line · K. Y. Srinivasan
The SD_FLUSH_TIMEOUT value is currently hardcoded.
Hardcoded where? Please, more context.

This is defined in scsi/sd.h:

#define SD_FLUSH_TIMEOUT (60 * HZ)

Quoted 6 lines

Quoted 4 lines
On our cloud, we sometimes hit this timeout. I was wondering if we could make this a module parameter. If this is acceptable, I can send you a patch for this.
A module parameter don't make sense for a per-device value, does it?

Currently, the 60 second timeout is applied across devices. Ideally, I want to be able to control the FLUSH TIMEOUT as we currently do I/O timeout. If this is acceptable, I can work on a patch for that as well.

Regards,

K. Y

Quoted 2 lines

greg k-h

Jack Wang

2013-09-24 12:08 UTC #4 lore raw

Quoted 31 lines · KY Srinivasan

Quoted 11 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, September 20, 2013 1:32 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 4 lines
Quoted 1 line · K. Y. Srinivasan
The SD_FLUSH_TIMEOUT value is currently hardcoded.
Hardcoded where? Please, more context.
This is defined in scsi/sd.h:
#define SD_FLUSH_TIMEOUT (60 * HZ)
Quoted 6 lines
Quoted 4 lines
On our cloud, we sometimes hit this timeout. I was wondering if we could make this a module parameter. If this is acceptable, I can send you a patch for this.
A module parameter don't make sense for a per-device value, does it?
Currently, the 60 second timeout is applied across devices. Ideally, I want to be able to control the FLUSH TIMEOUT as we currently do I/O timeout. If this is acceptable, I can work on a patch for that as well.
Regards,

Quoted 8 lines

K. Y
Quoted 2 lines
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

Back to 2010, Mike(cc-ed) try to add a flush time out interface, similar to what you want here, no idea why it's just ignored?
http://www.spinics.net/lists/linux-scsi/msg45017.html

Jack

KY Srinivasan

author

2013-09-24 12:35 UTC #5 lore raw

Quoted 51 lines

-----Original Message-----
From: Jack Wang [mailto:[email protected]]
Sent: Tuesday, September 24, 2013 5:08 AM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; Mike Christie
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 31 lines · KY Srinivasan
Quoted 11 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, September 20, 2013 1:32 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 4 lines
Quoted 1 line · K. Y. Srinivasan
The SD_FLUSH_TIMEOUT value is currently hardcoded.
Hardcoded where? Please, more context.
This is defined in scsi/sd.h:
#define SD_FLUSH_TIMEOUT (60 * HZ)
Quoted 6 lines
Quoted 4 lines
On our cloud, we sometimes hit this timeout. I was wondering if we could make this a module parameter. If this is acceptable, I can send you a patch for this.
A module parameter don't make sense for a per-device value, does it?
Currently, the 60 second timeout is applied across devices. Ideally, I want to be able to control the FLUSH TIMEOUT as we currently do I/O timeout. If this is acceptable, I can work on a patch for that as well.
Regards,
Quoted 8 lines
K. Y
Quoted 2 lines
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines

Hi,
Back to 2010, Mike(cc-ed) try to add a flush time out interface, similar to what you want here, no idea why it's just ignored?
http://www.spinics.net/lists/linux-scsi/msg45017.html

Thanks Jack. Mike, do you know what the concerns were as to why this patch was not accepted?

Regards,

K. Y

Bernd Schubert

2013-09-24 13:11 UTC #6 lore raw

Quoted 51 lines · KY Srinivasan

Quoted 51 lines
-----Original Message-----
From: Jack Wang [mailto:[email protected]]
Sent: Tuesday, September 24, 2013 5:08 AM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; Mike Christie
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 39 lines · KY Srinivasan
Quoted 11 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, September 20, 2013 1:32 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 4 lines
Quoted 1 line · K. Y. Srinivasan
The SD_FLUSH_TIMEOUT value is currently hardcoded.
Hardcoded where? Please, more context.
This is defined in scsi/sd.h:
#define SD_FLUSH_TIMEOUT (60 * HZ)
Quoted 6 lines
Quoted 4 lines
On our cloud, we sometimes hit this timeout. I was wondering if we could make this a module parameter. If this is acceptable, I can send you a patch for this.
A module parameter don't make sense for a per-device value, does it?
Currently, the 60 second timeout is applied across devices. Ideally, I want to be able to control the FLUSH TIMEOUT as we currently do I/O timeout. If this is acceptable, I can work on a patch for that as well.
Regards,
K. Y
Quoted 2 lines
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines

Quoted 5 lines
Hi,
Back to 2010, Mike(cc-ed) try to add a flush time out interface, similar to what you want here, no idea why it's just ignored?
http://www.spinics.net/lists/linux-scsi/msg45017.html
Thanks Jack. Mike, do you know what the concerns were as to why this patch was not accepted?

Mike Christie

2013-09-24 17:33 UTC #7 lore raw

In reply to KY Srinivasan

Quoted 51 lines · KY Srinivasan

Quoted 51 lines
-----Original Message-----
From: Jack Wang [mailto:[email protected]]
Sent: Tuesday, September 24, 2013 5:08 AM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; Mike Christie
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 39 lines · KY Srinivasan
Quoted 11 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, September 20, 2013 1:32 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 4 lines
Quoted 1 line · K. Y. Srinivasan
The SD_FLUSH_TIMEOUT value is currently hardcoded.
Hardcoded where? Please, more context.
This is defined in scsi/sd.h:
#define SD_FLUSH_TIMEOUT (60 * HZ)
Quoted 6 lines
Quoted 4 lines
On our cloud, we sometimes hit this timeout. I was wondering if we could make this a module parameter. If this is acceptable, I can send you a patch for this.
A module parameter don't make sense for a per-device value, does it?
Currently, the 60 second timeout is applied across devices. Ideally, I want to be able to control the FLUSH TIMEOUT as we currently do I/O timeout. If this is acceptable, I can work on a patch for that as well.
Regards,
K. Y
Quoted 2 lines
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines

Quoted 5 lines
Hi,
Back to 2010, Mike(cc-ed) try to add a flush time out interface, similar to what you want here, no idea why it's just ignored?
http://www.spinics.net/lists/linux-scsi/msg45017.html
Thanks Jack. Mike, do you know what the concerns were as to why this patch was not accepted?

I do not remember the exact concerns. We ended up just increasing the hard coded value in:

commit e3b3e6246726cd05950677ed843010b8e8c5884c

Author: Mike Christie <[email protected]>
Date:   Wed Aug 11 11:06:25 2010 -0500

[SCSI] scsi/block: increase flush/sync timeout

In the git commit message there is a comment about people thinking making it configurable for users was troublesome.

KY Srinivasan

author

2013-09-24 21:53 UTC #8 lore raw

Quoted 78 lines

-----Original Message-----
From: Mike Christie [mailto:[email protected]]
Sent: Tuesday, September 24, 2013 10:21 AM
To: KY Srinivasan
Cc: Jack Wang; Greg KH; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 30 lines · KY Srinivasan
Quoted 30 lines
-----Original Message-----
From: Jack Wang [mailto:[email protected]]
Sent: Tuesday, September 24, 2013 5:08 AM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; Mike Christie
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 18 lines · KY Srinivasan
Quoted 11 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, September 20, 2013 1:32 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 4 lines
Quoted 1 line · K. Y. Srinivasan
The SD_FLUSH_TIMEOUT value is currently hardcoded.
Hardcoded where? Please, more context.
This is defined in scsi/sd.h:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
#define SD_FLUSH_TIMEOUT (60 * HZ)
Quoted 6 lines
Quoted 4 lines
On our cloud, we sometimes hit this timeout. I was wondering if we could make this a module parameter. If this is acceptable, I can send you a patch for this.
A module parameter don't make sense for a per-device value, does it?
Currently, the 60 second timeout is applied across devices. Ideally, I want to
be
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
able to control the FLUSH TIMEOUT as we currently do I/O timeout. If this is acceptable, I can work on a patch for that as well.
Regards,
K. Y
Quoted 2 lines
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 9 lines
Quoted 5 lines
Hi,
Back to 2010, Mike(cc-ed) try to add a flush time out interface, similar to what you want here, no idea why it's just ignored?
http://www.spinics.net/lists/linux-scsi/msg45017.html
Thanks Jack. Mike, do you know what the concerns were as to why this patch was not accepted?
I do not remember the exact concerns. We ended up just increasing the hard coded value in:
commit e3b3e6246726cd05950677ed843010b8e8c5884c

Quoted 8 lines

Author: Mike Christie <[email protected]>
Date:   Wed Aug 11 11:06:25 2010 -0500
[SCSI] scsi/block: increase flush/sync timeout
In the git commit message there is a comment about people thinking making it configurable for users was troublesome.

I am not sure how that magic number was arrived at (the 60HZ number). We want this to be little higher - would there be any issues raising this to say 180 seconds. This is the value we currently have for I/O timeout.

Regards,

K. Y

Geert Uytterhoeven

2013-09-25 08:40 UTC #9 lore raw

Quoted 1 line · KY Srinivasan

I am not sure how that magic number was arrived at (the 60HZ number). We want this to be little higher -

"60 * HZ" means "60 seconds".

Quoted 2 lines

would there be any issues raising this to say 180 seconds. This is the value we currently have for I/O timeout.

So you want to replace it by "180 * HZ", which is ... another magic number?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

KY Srinivasan

author

2013-10-02 18:29 UTC #10 lore raw

Quoted 16 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Geert Uytterhoeven
Sent: Wednesday, September 25, 2013 1:40 AM
To: KY Srinivasan
Cc: Mike Christie; Jack Wang; Greg KH; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 1 line · KY Srinivasan
I am not sure how that magic number was arrived at (the 60HZ number). We
want this to be little higher -

Quoted 7 lines

"60 * HZ" means "60 seconds".
Quoted 1 line
would there be any issues raising this to say 180 seconds. This is the value we
currently have for I/O
Quoted 1 line
timeout.
So you want to replace it by "180 * HZ", which is ... another magic number?

Ideally, I want this to be adjustable like the way we can change the I/O timeout. Since that has been attempted earlier and rejected (not clear what the reasons were), I was suggesting that we pick a larger number. James, let me know how I should proceed here.

Regards,

K. Y

Quoted 11 lines

Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Nicholas A. Bellinger

2013-10-03 12:00 UTC #11 lore raw

Quoted 18 lines · KY Srinivasan

Quoted 18 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Geert Uytterhoeven
Sent: Wednesday, September 25, 2013 1:40 AM
To: KY Srinivasan
Cc: Mike Christie; Jack Wang; Greg KH; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 1 line · KY Srinivasan
I am not sure how that magic number was arrived at (the 60HZ number). We
want this to be little higher -
"60 * HZ" means "60 seconds".
Quoted 1 line
would there be any issues raising this to say 180 seconds. This is the value we

Quoted 9 lines

Quoted 4 lines
currently have for I/O
Quoted 1 line
timeout.
So you want to replace it by "180 * HZ", which is ... another magic number?
Ideally, I want this to be adjustable like the way we can change the I/O timeout. Since that has been attempted earlier and rejected (not clear what the reasons were), I was suggesting that we pick a larger number. James, let me know how I should proceed here.

I think the objection was to making a module parameter for doing this globally for all struct scsi_disk, and not the idea of making it adjustable on an individual basis per-say..

What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..?

Here's a quick (untested) patch to that effect.

--nab

drivers/scsi/sd.c +37 -1

index e62d17d..f7a8c5b 100644
@@ -460,6 +460,38 @@ max_write_same_blocks_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(max_write_same_blocks);
 
+static ssize_t
+flush_timeout_show(struct device *dev,
+		   struct device_attribute *attr, char *buf)
+{
+	struct scsi_disk *sdkp = to_scsi_disk(dev);
+
+	return snprintf(buf, 20, "%u\n", sdkp->flush_timeout);
+}
+
+static ssize_t
+flush_timeout_store(struct device *dev,
+		    struct device_attribute *attr, const char *buf,
+		    size_t count)
+{
+	struct scsi_disk *sdkp = to_scsi_disk(dev);
+	unsigned int flush_timeout;
+	int err;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+
+	err = kstrtouint(buf, 10, &flush_timeout);
+
+	if (flush_timeout > SD_FLUSH_TIMEOUT_MAX)
+		return -EINVAL;
+
+	sdkp->flush_timeout = flush_timeout;
+
+	return err ? err : count;
+}
+static DEVICE_ATTR_RW(flush_timeout);
+
 static struct attribute *sd_disk_attrs[] = {
 	&dev_attr_cache_type.attr,
 	&dev_attr_FUA.attr,

@@ -472,6 +504,7 @@ static struct attribute *sd_disk_attrs[] = {
 	&dev_attr_provisioning_mode.attr,
 	&dev_attr_max_write_same_blocks.attr,
 	&dev_attr_max_medium_access_timeouts.attr,
+	&dev_attr_flush_timeout.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(sd_disk);

@@ -829,7 +862,9 @@ static int sd_setup_write_same_cmnd(struct scsi_device *sdp, struct request *rq)
 
 static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
 {
-	rq->timeout = SD_FLUSH_TIMEOUT;
+	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
+
+	rq->timeout = sdkp->flush_timeout;
 	rq->retries = SD_MAX_RETRIES;
 	rq->cmd[0] = SYNCHRONIZE_CACHE;
 	rq->cmd_len = 10;

@@ -2841,6 +2876,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 	sdkp->ATO = 0;
 	sdkp->first_scan = 1;
 	sdkp->max_medium_access_timeouts = SD_MAX_MEDIUM_TIMEOUTS;
+	sdkp->flush_timeout = SD_FLUSH_TIMEOUT;
 
 	sd_revalidate_disk(gd);

drivers/scsi/sd.h +2 -0

index 7a049de..ac7cd0f 100644
@@ -14,6 +14,7 @@
 #define SD_TIMEOUT		(30 * HZ)
 #define SD_MOD_TIMEOUT		(75 * HZ)
 #define SD_FLUSH_TIMEOUT	(60 * HZ)
+#define SD_FLUSH_TIMEOUT_MAX	(180 * HZ)
 #define SD_WRITE_SAME_TIMEOUT	(120 * HZ)
 
 /*

@@ -68,6 +69,7 @@ struct scsi_disk {
 	unsigned int	physical_block_size;
 	unsigned int	max_medium_access_timeouts;
 	unsigned int	medium_access_timed_out;
+	unsigned int	flush_timeout;
 	u8		media_present;
 	u8		write_prot;
 	u8		protection_type;/* Data Integrity Field */

KY Srinivasan

author

2013-10-03 14:13 UTC #12 lore raw

Quoted 45 lines

-----Original Message-----
From: Nicholas A. Bellinger [mailto:[email protected]]
Sent: Thursday, October 03, 2013 5:09 AM
To: KY Srinivasan
Cc: Geert Uytterhoeven; Mike Christie; Jack Wang; Greg KH; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 3 lines · KY Srinivasan
Quoted 3 lines
-----Original Message-----
From: [email protected]
[mailto:[email protected]]
Quoted 5 lines
Quoted 5 lines
On Behalf Of Geert Uytterhoeven
Sent: Wednesday, September 25, 2013 1:40 AM
To: KY Srinivasan
Cc: Mike Christie; Jack Wang; Greg KH; [email protected]; [email protected]; [email protected];
[email protected];
Quoted 10 lines
Quoted 10 lines
[email protected]; [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 1 line · KY Srinivasan
I am not sure how that magic number was arrived at (the 60HZ number). We
want this to be little higher -
"60 * HZ" means "60 seconds".
Quoted 1 line
would there be any issues raising this to say 180 seconds. This is the value
we
Quoted 7 lines
Quoted 4 lines
currently have for I/O
Quoted 1 line
timeout.
So you want to replace it by "180 * HZ", which is ... another magic number?
Ideally, I want this to be adjustable like the way we can change the I/O timeout. Since that has been attempted earlier and rejected (not clear what the reasons
were),
Quoted 1 line
I was suggesting that we pick a larger number. James, let me know how I should
proceed here.
Quoted 1 line

Quoted 7 lines

I think the objection was to making a module parameter for doing this globally for all struct scsi_disk, and not the idea of making it adjustable on an individual basis per-say..
What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..?
Here's a quick (untested) patch to that effect.

Thanks Nicholas. This is precisely what I was hoping we could agree on. James, if this is acceptable, we will go ahead and test this patch.

Regards,

K. Y

Quoted 96 lines

--nab

drivers/scsi/sd.c +32 -0

index e62d17d..f7a8c5b 100644
@@ -460,6 +460,38 @@ max_write_same_blocks_store(struct device *dev,
 struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(max_write_same_blocks);
 
+static ssize_t
+flush_timeout_show(struct device *dev,
+		   struct device_attribute *attr, char *buf)
+{
+	struct scsi_disk *sdkp = to_scsi_disk(dev);
+
+	return snprintf(buf, 20, "%u\n", sdkp->flush_timeout);
+}
+
+static ssize_t
+flush_timeout_store(struct device *dev,
+		    struct device_attribute *attr, const char *buf,
+		    size_t count)
+{
+	struct scsi_disk *sdkp = to_scsi_disk(dev);
+	unsigned int flush_timeout;
+	int err;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+
+	err = kstrtouint(buf, 10, &flush_timeout);
+
+	if (flush_timeout > SD_FLUSH_TIMEOUT_MAX)
+		return -EINVAL;
+
+	sdkp->flush_timeout = flush_timeout;
+
+	return err ? err : count;
+}
+static DEVICE_ATTR_RW(flush_timeout);
+
 static struct attribute *sd_disk_attrs[] = {
 	&dev_attr_cache_type.attr,

drivers/scsi/sd.c +4 -1

 	&dev_attr_FUA.attr,

@@ -472,6 +504,7 @@ static struct attribute *sd_disk_attrs[] = {
 	&dev_attr_provisioning_mode.attr,
 	&dev_attr_max_write_same_blocks.attr,
 	&dev_attr_max_medium_access_timeouts.attr,
+	&dev_attr_flush_timeout.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(sd_disk);

@@ -829,7 +862,9 @@ static int sd_setup_write_same_cmnd(struct scsi_device
 *sdp, struct request *rq)
 
 static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
 {
-	rq->timeout = SD_FLUSH_TIMEOUT;
+	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
+
+	rq->timeout = sdkp->flush_timeout;
 	rq->retries = SD_MAX_RETRIES;
 	rq->cmd[0] = SYNCHRONIZE_CACHE;

drivers/scsi/sd.c +1 -0

 	rq->cmd_len = 10;

@@ -2841,6 +2876,7 @@ static void sd_probe_async(void *data, async_cookie_t
 cookie)
 	sdkp->ATO = 0;
 	sdkp->first_scan = 1;
 	sdkp->max_medium_access_timeouts = SD_MAX_MEDIUM_TIMEOUTS;
+	sdkp->flush_timeout = SD_FLUSH_TIMEOUT;
 
 	sd_revalidate_disk(gd);

drivers/scsi/sd.h +2 -0

index 7a049de..ac7cd0f 100644
@@ -14,6 +14,7 @@
 #define SD_TIMEOUT		(30 * HZ)
 #define SD_MOD_TIMEOUT		(75 * HZ)
 #define SD_FLUSH_TIMEOUT	(60 * HZ)
+#define SD_FLUSH_TIMEOUT_MAX	(180 * HZ)
 #define SD_WRITE_SAME_TIMEOUT	(120 * HZ)
 
 /*

@@ -68,6 +69,7 @@ struct scsi_disk {
 	unsigned int	physical_block_size;
 	unsigned int	max_medium_access_timeouts;
 	unsigned int	medium_access_timed_out;
+	unsigned int	flush_timeout;
 	u8		media_present;
 	u8		write_prot;
 	u8		protection_type;/* Data Integrity Field */

Eric Seppanen

2013-10-03 20:48 UTC #13 lore raw

In reply to Nicholas A. Bellinger

Quoted 7 lines · Nicholas A. Bellinger

Quoted 4 lines · KY Srinivasan
Ideally, I want this to be adjustable like the way we can change the I/O timeout. Since that has been attempted earlier and rejected (not clear what the reasons were), I was suggesting that we pick a larger number. James, let me know how I should proceed here.

Quoted 5 lines

I think the objection was to making a module parameter for doing this globally for all struct scsi_disk, and not the idea of making it adjustable on an individual basis per-say..
What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..?

Do I/O timeouts and flush timeouts need to be independently adjusted? If you're having trouble with slow operations, it seems likely to be across the board.

Flush timeout could be defined as 2x the read/write timeout. Any other command-specific timeouts could be scaled the same way.

Ewan Milne

2013-10-04 12:18 UTC #14 lore raw

Quoted 13 lines · Eric Seppanen

Quoted 11 lines · Nicholas A. Bellinger
Quoted 4 lines · KY Srinivasan
Ideally, I want this to be adjustable like the way we can change the I/O timeout. Since that has been attempted earlier and rejected (not clear what the reasons were), I was suggesting that we pick a larger number. James, let me know how I should proceed here.
I think the objection was to making a module parameter for doing this globally for all struct scsi_disk, and not the idea of making it adjustable on an individual basis per-say..

Quoted 8 lines

Quoted 1 line
What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..?
Do I/O timeouts and flush timeouts need to be independently adjusted? If you're having trouble with slow operations, it seems likely to be across the board.
Flush timeout could be defined as 2x the read/write timeout. Any other command-specific timeouts could be scaled the same way.

It seems to me that there isn't any reason to expect that the maximum amount of time a device might take to perform various operations are related by any coefficient. And, an HBA (particularly iSCSI or FC) could very well have different device types connected at different target IDs. So I think the flush timeout should be adjustable on a per-device basis. It's probably related more to the cache size on the device than anything else...

Also note that there is a SD_WRITE_SAME_TIMEOUT value that is currently 4x the default SD_TIMEOUT value. That should probably be adjustable as well.

-Ewan

Eric Seppanen

2013-10-04 18:12 UTC #15 lore raw

Quoted 8 lines · Ewan Milne

Quoted 6 lines · Eric Seppanen
Do I/O timeouts and flush timeouts need to be independently adjusted? If you're having trouble with slow operations, it seems likely to be across the board.
Flush timeout could be defined as 2x the read/write timeout. Any other command-specific timeouts could be scaled the same way.

Quoted 7 lines

It seems to me that there isn't any reason to expect that the maximum amount of time a device might take to perform various operations are related by any coefficient. And, an HBA (particularly iSCSI or FC) could very well have different device types connected at different target IDs. So I think the flush timeout should be adjustable on a per-device basis. It's probably related more to the cache size on the device than anything else...

There are two possible delays: how long the device might possibly take, and how long the storage fabric might take.

On a local device, only the first matters. But there are environments where the second dominates (e.g. a virtual machine, where the hypervisor's storage uses multipath with a long failover delay).

If somebody wants to set flush timeouts > 60 seconds, I would like to know if they're trying to address a slow device or a slow fabric. If it's the fabric, then it's kind of silly to make them set three different timeouts to address the same problem.

An alternate way of handling long fabric delays would be to have a fabric_timeout that gets added to all the other timeouts... could be a scsi_host parameter but that's probably overengineering the problem.

There are already VM vendors that tell customers to adjust the current sysfs timeout, so the least amount of work would be to make all of the other timeouts track that one in some way (additive or multiplicative).

KY Srinivasan

author

2013-10-04 15:02 UTC #16 lore raw

In reply to Eric Seppanen

Quoted 26 lines

-----Original Message-----
From: Eric Seppanen [mailto:[email protected]]
Sent: Thursday, October 03, 2013 1:49 PM
To: Nicholas A. Bellinger
Cc: KY Srinivasan; [email protected]; [email protected]; [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 3 lines · Nicholas A. Bellinger
Quoted 1 line · KY Srinivasan
Ideally, I want this to be adjustable like the way we can change the I/O
timeout.
Quoted 1 line
Quoted 1 line
Since that has been attempted earlier and rejected (not clear what the
reasons were),
Quoted 1 line
Quoted 1 line
I was suggesting that we pick a larger number. James, let me know how I
should proceed here.
Quoted 6 lines
Quoted 1 line
I think the objection was to making a module parameter for doing this globally for all struct scsi_disk, and not the idea of making it adjustable on an individual basis per-say..

Quoted 8 lines

Quoted 1 line
What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..?
Do I/O timeouts and flush timeouts need to be independently adjusted? If you're having trouble with slow operations, it seems likely to be across the board.
Flush timeout could be defined as 2x the read/write timeout. Any other command-specific timeouts could be scaled the same way.

I like this idea and would result in minimal changes. James, if it ok with you, I could send you the patch.

Regards,

K. Y

James Bottomley

maintainer

2013-10-04 16:28 UTC #17 lore raw

Quoted 31 lines · KY Srinivasan

Quoted 31 lines
-----Original Message-----
From: Eric Seppanen [mailto:[email protected]]
Sent: Thursday, October 03, 2013 1:49 PM
To: Nicholas A. Bellinger
Cc: KY Srinivasan; [email protected]; [email protected]; [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 3 lines · Nicholas A. Bellinger
Quoted 1 line · KY Srinivasan
Ideally, I want this to be adjustable like the way we can change the I/O
timeout.
Quoted 1 line
Quoted 1 line
Since that has been attempted earlier and rejected (not clear what the
reasons were),
Quoted 1 line
Quoted 1 line
I was suggesting that we pick a larger number. James, let me know how I
should proceed here.
Quoted 7 lines
Quoted 1 line
I think the objection was to making a module parameter for doing this globally for all struct scsi_disk, and not the idea of making it adjustable on an individual basis per-say..
What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..?
Do I/O timeouts and flush timeouts need to be independently adjusted? If you're having trouble with slow operations, it seems likely to be across the board.

Quoted 5 lines

Quoted 2 lines
Flush timeout could be defined as 2x the read/write timeout. Any other command-specific timeouts could be scaled the same way.
I like this idea and would result in minimal changes. James, if it ok with you, I could send you the patch.

Depends: I still prefer the per-target override, but if the proposal is to take the existing variable timeout for the queue and have 2x that for the flush, so you plan to increase the per-device timeout with hyper-v to 90s via sysfs, then I'm OK with it.

James

James Bottomley

2013-10-03 14:01 UTC #18 lore raw

In reply to KY Srinivasan

Quoted 18 lines · KY Srinivasan

Quoted 18 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Geert Uytterhoeven
Sent: Wednesday, September 25, 2013 1:40 AM
To: KY Srinivasan
Cc: Mike Christie; Jack Wang; Greg KH; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: Drivers: scsi: FLUSH timeout
Quoted 1 line · KY Srinivasan
I am not sure how that magic number was arrived at (the 60HZ number). We
want this to be little higher -
"60 * HZ" means "60 seconds".
Quoted 1 line
would there be any issues raising this to say 180 seconds. This is the value we

Quoted 8 lines

Quoted 4 lines
currently have for I/O
Quoted 1 line
timeout.
So you want to replace it by "180 * HZ", which is ... another magic number?
Ideally, I want this to be adjustable like the way we can change the I/O timeout. Since that has been attempted earlier and rejected (not clear what the reasons were), I was suggesting that we pick a larger number. James, let me know how I should proceed here.

It's only one problem device with its own driver, right? how about a per target value you set in target_configure? That way there's no damage to the rest of the system even in a mixed device environment. I don't see a particular need to expose this via sysfs unless someone else has a use case.

James