linux-kernel - Re: Drivers: scsi: FLUSH timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1380802143.19256.95.camel@haakon3.risingtidesystems.com>
Date:	Thu, 03 Oct 2013 05:09:03 -0700
From:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:	KY Srinivasan <kys@...rosoft.com>
Cc:	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Mike Christie <michaelc@...wisc.edu>,
	Jack Wang <xjtuwjp@...il.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"ohering@...e.com" <ohering@...e.com>,
	"jbottomley@...allels.com" <jbottomley@...allels.com>,
	"hch@...radead.org" <hch@...radead.org>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.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@...il.com [mailto:geert.uytterhoeven@...il.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@...r.kernel.org;
> > devel@...uxdriverproject.org; ohering@...e.com; jbottomley@...allels.com;
> > hch@...radead.org; linux-scsi@...r.kernel.org
> > Subject: Re: Drivers: scsi: FLUSH timeout
> > 
> > On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan <kys@...rosoft.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 */

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/