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
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
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 <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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
>
> 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
On 09/21/2013 07:24 AM, KY Srinivasan wrote:
>
>
>> -----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
>>
>> 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 [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
> -----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
>
> On 09/21/2013 07:24 AM, KY Srinivasan wrote:
> >
> >
> >> -----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
> >>
> >> 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 [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
Thanks Jack. Mike, do you know what the concerns were as to why this
patch was not accepted?
Regards,
K. Y
On 09/24/2013 02:35 PM, KY Srinivasan wrote:
>
>
>> -----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
>>
>> On 09/21/2013 07:24 AM, KY Srinivasan wrote:
>>>
>>>
>>>> -----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
>>>>
>>>> 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 [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
>
> 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
On 09/24/2013 07:35 AM, KY Srinivasan wrote:
>
>
>> -----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
>>
>> On 09/21/2013 07:24 AM, KY Srinivasan wrote:
>>>
>>>
>>>> -----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
>>>>
>>>> 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 [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
>
> 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.
> -----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
>
> On 09/24/2013 07:35 AM, KY Srinivasan wrote:
> >
> >
> >> -----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
> >>
> >> On 09/21/2013 07:24 AM, KY Srinivasan wrote:
> >>>
> >>>
> >>>> -----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
> >>>>
> >>>> 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 [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
> >
> > 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.
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
On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <[email protected]> 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 -- [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
> -----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
>
> On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <[email protected]> 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 -- [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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, 2013-10-02 at 18:29 +0000, KY Srinivasan wrote:
>
> > -----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
> >
> > On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <[email protected]> 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 */
On Wed, 2013-10-02 at 18:29 +0000, KY Srinivasan wrote:
>
> > -----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
> >
> > On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <[email protected]> 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
> -----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
>
> On Wed, 2013-10-02 at 18:29 +0000, KY Srinivasan wrote:
> >
> > > -----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
> > >
> > > On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <[email protected]> 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 */
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger
<[email protected]> 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.
On Thu, 2013-10-03 at 13:48 -0700, Eric Seppanen wrote:
> On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger
> <[email protected]> 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
> -----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
>
> On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger
> <[email protected]> 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
On Fri, 2013-10-04 at 15:02 +0000, KY Srinivasan wrote:
>
> > -----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
> >
> > On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger
> > <[email protected]> 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
On Fri, Oct 4, 2013 at 5:18 AM, Ewan Milne <[email protected]> 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).