| 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
| ||
|
Date: Fri, 4 Oct 2013 22:01:12 +0000
From: KY Srinivasan <kys@...rosoft.com>
To: James Bottomley <jbottomley@...allels.com>
CC: "gregkh@...uxfoundation.org" <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>,
"hch@...radead.org" <hch@...radead.org>,
"eric@...estorage.com" <eric@...estorage.com>,
"nab@...ux-iscsi.org" <nab@...ux-iscsi.org>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>
Subject: RE: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the
basic I/O timeout
> -----Original Message-----
> From: James Bottomley [mailto:jbottomley@...allels.com]
> Sent: Friday, October 04, 2013 2:42 PM
> To: KY Srinivasan
> Cc: gregkh@...uxfoundation.org; linux-kernel@...r.kernel.org;
> devel@...uxdriverproject.org; ohering@...e.com; hch@...radead.org;
> eric@...estorage.com; nab@...ux-iscsi.org; linux-scsi@...r.kernel.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the basic
> I/O timeout
>
> On Fri, 2013-10-04 at 12:38 -0700, K. Y. Srinivasan wrote:
> > Rather than having a separate constant for specifying the timeout on FLUSH
> > operations, use the basic I/O timeout value that is already configurable
> > on a per target basis to derive the FLUSH timeout. Looking at the current
> > definitions of these timeout values, the FLUSH operation is supposed to have
> > a value that is twice the normal timeout value. This patch preserves this
> > relationship while leveraging the flexibility of specifying the I/O timeout.
> >
> > I would like to thank Eric Seppanen <eric@...estorage.com> and
> > Nicholas A. Bellinger <nab@...ux-iscsi.org> for their help in resolving
> > this issue.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@...rosoft.com>
> > ---
> > drivers/scsi/sd.c | 5 +++--
> > 1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index e62d17d..8aff306 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -829,7 +829,7 @@ 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;
> > + rq->timeout *= 2;
> > rq->retries = SD_MAX_RETRIES;
> > rq->cmd[0] = SYNCHRONIZE_CACHE;
> > rq->cmd_len = 10;
> > @@ -1433,6 +1433,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > {
> > int retries, res;
> > struct scsi_device *sdp = sdkp->device;
> > + unsigned int timeout = sdp->request_queue->rq_timeout;
>
> The timeout is signed in the function prototype
>
> > struct scsi_sense_hdr sshdr;
> >
> > if (!scsi_device_online(sdp))
> > @@ -1448,7 +1449,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > * flush everything.
> > */
> > res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> > - &sshdr, SD_FLUSH_TIMEOUT,
> > + &sshdr, timeout * 2,
> > SD_MAX_RETRIES, NULL, REQ_PM);
> > if (res == 0)
> > break;
>
> Not like this, please: you now leave us with a dangling #define whose
> name makes you think it should be related to flushing and a couple of
> curious magic constants. It's almost hand crafted to confuse people
> reading the code.
>
> Please do it like this instead: with a comment explaining what we're
> doing above the #define and a name that clearly relates to the actions.
>
> James
>
> ---
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e62d17d..5c9496d 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -829,7 +829,7 @@ 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;
> + rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
> rq->retries = SD_MAX_RETRIES;
> rq->cmd[0] = SYNCHRONIZE_CACHE;
> rq->cmd_len = 10;
> @@ -1433,6 +1433,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> {
> int retries, res;
> struct scsi_device *sdp = sdkp->device;
> + const int timeout = sdp->request_queue->rq_timeout
> + * SD_FLUSH_TIMEOUT_MULTIPLIER;
> struct scsi_sense_hdr sshdr;
>
> if (!scsi_device_online(sdp))
> @@ -1448,8 +1450,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> * flush everything.
> */
> res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> - &sshdr, SD_FLUSH_TIMEOUT,
> - SD_MAX_RETRIES, NULL, REQ_PM);
> + &sshdr, timeout, SD_MAX_RETRIES,
> + NULL, REQ_PM);
> if (res == 0)
> break;
> }
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 7a049de..7f7999c 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -13,7 +13,11 @@
> */
> #define SD_TIMEOUT (30 * HZ)
> #define SD_MOD_TIMEOUT (75 * HZ)
> -#define SD_FLUSH_TIMEOUT (60 * HZ)
> +/*
> + * Flush timeout is a multiplier over the standard device timeout which is
> + * user modifiable via sysfs but initially set to SD_TIMEOUT
> + */
> +#define SD_FLUSH_TIMEOUT_MULTIPLIER 2
> #define SD_WRITE_SAME_TIMEOUT (120 * HZ)
>
> /*
Thanks James. Given that you have already made the changes you want done (in the
Patch above), do you still want me to send you a fixed up patch?
Regards,
K. Y
--
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/
Powered by blists - more mailing lists