Drivers: scsi: FLUSH timeout

Drivers: scsi: FLUSH timeout

[K. Y. Srinivasan]

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

Re: Drivers: scsi: FLUSH timeout

[Greg KH]

On Fri, Sep 20, 2013 at 12:32:27PM -0700, K. Y. Srinivasan wrote:
> The SD_FLUSH_TIMEOUT value is currently hardcoded.

Hardcoded where?  Please, more context.

> 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

Re: Drivers: scsi: FLUSH timeout

[Laurence Oberman]

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

Laurence


On Fri, Sep 20, 2013 at 4:32 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Sep 20, 2013 at 12:32:27PM -0700, K. Y. Srinivasan wrote:
>> The SD_FLUSH_TIMEOUT value is currently hardcoded.
>
> Hardcoded where?  Please, more context.
>
>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Friday, September 20, 2013 1:32 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> ohering@suse.com; jbottomley@parallels.com; hch@infradead.org; linux-
> scsi@vger.kernel.org
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On Fri, Sep 20, 2013 at 12:32:27PM -0700, K. Y. Srinivasan wrote:
> > 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)
> 
> > 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
> 
> greg k-h

Re: Drivers: scsi: FLUSH timeout

[Jack Wang]

On 09/21/2013 07:24 AM, KY Srinivasan wrote:
> 
> 
>> -----Original Message-----
>> From: Greg KH [mailto:gregkh@linuxfoundation.org]
>> Sent: Friday, September 20, 2013 1:32 PM
>> To: KY Srinivasan
>> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
>> ohering@suse.com; jbottomley@parallels.com; hch@infradead.org; linux-
>> scsi@vger.kernel.org
>> Subject: Re: Drivers: scsi: FLUSH timeout
>>
>> On Fri, Sep 20, 2013 at 12:32:27PM -0700, K. Y. Srinivasan wrote:
>>> 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)
>>
>>> 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
>>
>> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> 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

RE: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -----Original Message-----
> From: Jack Wang [mailto:xjtuwjp@gmail.com]
> Sent: Tuesday, September 24, 2013 5:08 AM
> To: KY Srinivasan
> Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> ohering@suse.com; jbottomley@parallels.com; hch@infradead.org; linux-
> scsi@vger.kernel.org; Mike Christie
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On 09/21/2013 07:24 AM, KY Srinivasan wrote:
> >
> >
> >> -----Original Message-----
> >> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> >> Sent: Friday, September 20, 2013 1:32 PM
> >> To: KY Srinivasan
> >> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> >> ohering@suse.com; jbottomley@parallels.com; hch@infradead.org; linux-
> >> scsi@vger.kernel.org
> >> Subject: Re: Drivers: scsi: FLUSH timeout
> >>
> >> On Fri, Sep 20, 2013 at 12:32:27PM -0700, K. Y. Srinivasan wrote:
> >>> 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)
> >>
> >>> 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
> >>
> >> greg k-h
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > 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

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

Regards,

K. Y 

Re: Drivers: scsi: FLUSH timeout

[Bernd Schubert]

On 09/24/2013 02:35 PM, KY Srinivasan wrote:
>
>
>> -----Original Message-----
>> From: Jack Wang [mailto:xjtuwjp@gmail.com]
>> Sent: Tuesday, September 24, 2013 5:08 AM
>> To: KY Srinivasan
>> Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
>> ohering@suse.com; jbottomley@parallels.com; hch@infradead.org; linux-
>> scsi@vger.kernel.org; Mike Christie
>> Subject: Re: Drivers: scsi: FLUSH timeout
>>
>> On 09/21/2013 07:24 AM, KY Srinivasan wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Greg KH [mailto:gregkh@linuxfoundation.org]
>>>> Sent: Friday, September 20, 2013 1:32 PM
>>>> To: KY Srinivasan
>>>> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
>>>> ohering@suse.com; jbottomley@parallels.com; hch@infradead.org; linux-
>>>> scsi@vger.kernel.org
>>>> Subject: Re: Drivers: scsi: FLUSH timeout
>>>>
>>>> On Fri, Sep 20, 2013 at 12:32:27PM -0700, K. Y. Srinivasan wrote:
>>>>> 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)
>>>>
>>>>> 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
>>>>
>>>> greg k-h
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>>> the body of a message to majordomo@vger.kernel.org
>>> 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
>
> Thanks Jack. Mike, do you know what the concerns were as to why this
> patch was not accepted?
>

See also this discussion:

http://marc.info/?l=linux-scsi&m=127167679221319&w=2

And retries have been added by commit 
c213e1407be6b04b144794399a91472e0ef92aec


Cheers,
Bernd

Re: Drivers: scsi: FLUSH timeout

[Mike Christie]

On 09/24/2013 07:35 AM, KY Srinivasan wrote:
> 
> 
>> -----Original Message-----
>> From: Jack Wang [mailto:xjtuwjp@gmail.com]
>> Sent: Tuesday, September 24, 2013 5:08 AM
>> To: KY Srinivasan
>> Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
>> ohering@suse.com; jbottomley@parallels.com; hch@infradead.org; linux-
>> scsi@vger.kernel.org; Mike Christie
>> Subject: Re: Drivers: scsi: FLUSH timeout
>>
>> On 09/21/2013 07:24 AM, KY Srinivasan wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Greg KH [mailto:gregkh@linuxfoundation.org]
>>>> Sent: Friday, September 20, 2013 1:32 PM
>>>> To: KY Srinivasan
>>>> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
>>>> ohering@suse.com; jbottomley@parallels.com; hch@infradead.org; linux-
>>>> scsi@vger.kernel.org
>>>> Subject: Re: Drivers: scsi: FLUSH timeout
>>>>
>>>> On Fri, Sep 20, 2013 at 12:32:27PM -0700, K. Y. Srinivasan wrote:
>>>>> 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)
>>>>
>>>>> 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
>>>>
>>>> greg k-h
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>>> the body of a message to majordomo@vger.kernel.org
>>> 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
> 
> 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 <mchristi@redhat.com>
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.

RE: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -----Original Message-----
> From: Mike Christie [mailto:michaelc@cs.wisc.edu]
> Sent: Tuesday, September 24, 2013 10:21 AM
> To: KY Srinivasan
> Cc: Jack Wang; Greg KH; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; jbottomley@parallels.com;
> hch@infradead.org; linux-scsi@vger.kernel.org
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On 09/24/2013 07:35 AM, KY Srinivasan wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jack Wang [mailto:xjtuwjp@gmail.com]
> >> Sent: Tuesday, September 24, 2013 5:08 AM
> >> To: KY Srinivasan
> >> Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> >> ohering@suse.com; jbottomley@parallels.com; hch@infradead.org; linux-
> >> scsi@vger.kernel.org; Mike Christie
> >> Subject: Re: Drivers: scsi: FLUSH timeout
> >>
> >> On 09/21/2013 07:24 AM, KY Srinivasan wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> >>>> Sent: Friday, September 20, 2013 1:32 PM
> >>>> To: KY Srinivasan
> >>>> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> >>>> ohering@suse.com; jbottomley@parallels.com; hch@infradead.org; linux-
> >>>> scsi@vger.kernel.org
> >>>> Subject: Re: Drivers: scsi: FLUSH timeout
> >>>>
> >>>> On Fri, Sep 20, 2013 at 12:32:27PM -0700, K. Y. Srinivasan wrote:
> >>>>> 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)
> >>>>
> >>>>> 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
> >>>>
> >>>> greg k-h
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> 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
> >
> > 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 <mchristi@redhat.com>
> 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

Re: Drivers: scsi: FLUSH timeout

[Geert Uytterhoeven]

On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <kys@microsoft.com> wrote:
> 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".

> 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 -- geert@linux-m68k.org

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

RE: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -----Original Message-----
> From: geert.uytterhoeven@gmail.com [mailto:geert.uytterhoeven@gmail.com]
> On Behalf Of Geert Uytterhoeven
> Sent: Wednesday, September 25, 2013 1:40 AM
> To: KY Srinivasan
> Cc: Mike Christie; Jack Wang; Greg KH; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; jbottomley@parallels.com;
> hch@infradead.org; linux-scsi@vger.kernel.org
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <kys@microsoft.com> wrote:
> > 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".
> 
> > 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?

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
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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

Re: Drivers: scsi: FLUSH timeout

[Nicholas A. Bellinger]

On Wed, 2013-10-02 at 18:29 +0000, KY Srinivasan wrote:
> 
> > -----Original Message-----
> > From: geert.uytterhoeven@gmail.com [mailto:geert.uytterhoeven@gmail.com]
> > On Behalf Of Geert Uytterhoeven
> > Sent: Wednesday, September 25, 2013 1:40 AM
> > To: KY Srinivasan
> > Cc: Mike Christie; Jack Wang; Greg KH; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; ohering@suse.com; jbottomley@parallels.com;
> > hch@infradead.org; linux-scsi@vger.kernel.org
> > Subject: Re: Drivers: scsi: FLUSH timeout
> > 
> > On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <kys@microsoft.com> wrote:
> > > 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".
> > 
> > > 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?
> 
> 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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index e62d17d..f7a8c5b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -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);
 
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 7a049de..ac7cd0f 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -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 */

RE: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -----Original Message-----
> From: Nicholas A. Bellinger [mailto:nab@linux-iscsi.org]
> Sent: Thursday, October 03, 2013 5:09 AM
> To: KY Srinivasan
> Cc: Geert Uytterhoeven; Mike Christie; Jack Wang; Greg KH; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; ohering@suse.com;
> jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On Wed, 2013-10-02 at 18:29 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: geert.uytterhoeven@gmail.com
> [mailto:geert.uytterhoeven@gmail.com]
> > > On Behalf Of Geert Uytterhoeven
> > > Sent: Wednesday, September 25, 2013 1:40 AM
> > > To: KY Srinivasan
> > > Cc: Mike Christie; Jack Wang; Greg KH; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; ohering@suse.com;
> jbottomley@parallels.com;
> > > hch@infradead.org; linux-scsi@vger.kernel.org
> > > Subject: Re: Drivers: scsi: FLUSH timeout
> > >
> > > On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <kys@microsoft.com> wrote:
> > > > 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".
> > >
> > > > 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?
> >
> > 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.

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
> 
> --nab
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e62d17d..f7a8c5b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -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);
> 
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 7a049de..ac7cd0f 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -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 */

Re: Drivers: scsi: FLUSH timeout

[Eric Seppanen]

On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
>
> On Wed, 2013-10-02 at 18:29 +0000, KY Srinivasan wrote:
> > 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..?

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.

Re: Drivers: scsi: FLUSH timeout

[Ewan Milne]

On Thu, 2013-10-03 at 13:48 -0700, Eric Seppanen wrote:
> On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> >
> > On Wed, 2013-10-02 at 18:29 +0000, KY Srinivasan wrote:
> > > 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..?
> 
> 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

Re: Drivers: scsi: FLUSH timeout

[Eric Seppanen]

On Fri, Oct 4, 2013 at 5:18 AM, Ewan Milne <emilne@redhat.com> wrote:
> On Thu, 2013-10-03 at 13:48 -0700, Eric Seppanen wrote:
>> 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...

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).

RE: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -----Original Message-----
> From: Eric Seppanen [mailto:eric@purestorage.com]
> Sent: Thursday, October 03, 2013 1:49 PM
> To: Nicholas A. Bellinger
> Cc: KY Srinivasan; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> linux-scsi@vger.kernel.org
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> >
> > On Wed, 2013-10-02 at 18:29 +0000, KY Srinivasan wrote:
> > > 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..?
> 
> 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

Re: Drivers: scsi: FLUSH timeout

[James Bottomley]

On Fri, 2013-10-04 at 15:02 +0000, KY Srinivasan wrote:
> 
> > -----Original Message-----
> > From: Eric Seppanen [mailto:eric@purestorage.com]
> > Sent: Thursday, October 03, 2013 1:49 PM
> > To: Nicholas A. Bellinger
> > Cc: KY Srinivasan; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > linux-scsi@vger.kernel.org
> > Subject: Re: Drivers: scsi: FLUSH timeout
> > 
> > On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger
> > <nab@linux-iscsi.org> wrote:
> > >
> > > On Wed, 2013-10-02 at 18:29 +0000, KY Srinivasan wrote:
> > > > 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..?
> > 
> > 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.

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

Re: Drivers: scsi: FLUSH timeout

[James Bottomley]

On Wed, 2013-10-02 at 18:29 +0000, KY Srinivasan wrote:
> 
> > -----Original Message-----
> > From: geert.uytterhoeven@gmail.com [mailto:geert.uytterhoeven@gmail.com]
> > On Behalf Of Geert Uytterhoeven
> > Sent: Wednesday, September 25, 2013 1:40 AM
> > To: KY Srinivasan
> > Cc: Mike Christie; Jack Wang; Greg KH; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; ohering@suse.com; jbottomley@parallels.com;
> > hch@infradead.org; linux-scsi@vger.kernel.org
> > Subject: Re: Drivers: scsi: FLUSH timeout
> > 
> > On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <kys@microsoft.com> wrote:
> > > 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".
> > 
> > > 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?
> 
> 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