RE: Drivers: scsi: FLUSH timeout

[KY Srinivasan]

> -----Original Message-----
> From: Nicholas A. Bellinger [mailto:nab@xxxxxxxxxxxxxxx]
> Sent: Thursday, October 03, 2013 5:09 AM
> To: KY Srinivasan
> Cc: Geert Uytterhoeven; Mike Christie; Jack Wang; Greg KH; linux-
> kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; ohering@xxxxxxxx;
> jbottomley@xxxxxxxxxxxxx; hch@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx
> Subject: Re: Drivers: scsi: FLUSH timeout
> 
> On Wed, 2013-10-02 at 18:29 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: geert.uytterhoeven@xxxxxxxxx
> [mailto:geert.uytterhoeven@xxxxxxxxx]
> > > 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@xxxxxxxxxxxxxxx;
> > > devel@xxxxxxxxxxxxxxxxxxxxxx; ohering@xxxxxxxx;
> jbottomley@xxxxxxxxxxxxx;
> > > hch@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx
> > > Subject: Re: Drivers: scsi: FLUSH timeout
> > >
> > > On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <kys@xxxxxxxxxxxxx> 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Ç+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—