[PATCH] Update WRITE_SAME timeout in sd_setup_discard_cmnd

[PATCH] Update WRITE_SAME timeout in sd_setup_discard_cmnd

[Shaun Tancheff]

In sd_setup_discard_cmnd() there are a some discard
methods that fall back to using WRITE_SAME. It
appears that those paths using WRITE_SAME should
also use the SD_WRITE_SAME_TIMEOUT instead of the
default SD_TIMEOUT.

Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com>
---
I don't have a use case that breaks the current code.
It just seems to me that setups for discard and
write same should be consistent.
---
 drivers/scsi/sd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d3e852a..3c15f3a 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -722,6 +722,8 @@ static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
 	if (!page)
 		return BLKPREP_DEFER;
 
+	rq->timeout = SD_TIMEOUT;
+
 	switch (sdkp->provisioning_mode) {
 	case SD_LBP_UNMAP:
 		buf = page_address(page);
@@ -746,6 +748,7 @@ static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
 		put_unaligned_be32(nr_sectors, &cmd->cmnd[10]);
 
 		len = sdkp->device->sector_size;
+		rq->timeout = SD_WRITE_SAME_TIMEOUT;
 		break;
 
 	case SD_LBP_WS10:
@@ -758,6 +761,7 @@ static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
 		put_unaligned_be16(nr_sectors, &cmd->cmnd[7]);
 
 		len = sdkp->device->sector_size;
+		rq->timeout = SD_WRITE_SAME_TIMEOUT;
 		break;
 
 	default:
@@ -766,8 +770,6 @@ static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
 	}
 
 	rq->completion_data = page;
-	rq->timeout = SD_TIMEOUT;
-
 	cmd->transfersize = len;
 	cmd->allowed = SD_MAX_RETRIES;
 
-- 
2.8.1

Re: [PATCH] Update WRITE_SAME timeout in sd_setup_discard_cmnd

[Martin K. Petersen]

>>>>> "Shaun" == Shaun Tancheff <shaun@tancheff.com> writes:

Shaun,

Shaun> In sd_setup_discard_cmnd() there are a some discard methods that
Shaun> fall back to using WRITE_SAME. It appears that those paths using
Shaun> WRITE_SAME should also use the SD_WRITE_SAME_TIMEOUT instead of
Shaun> the default SD_TIMEOUT.

The expectation is that the UNMAP variants update a translation table of
some sort in close to constant time. Potentially with some head and tail
zeroing on media. And should therefore easily fall within the 30s limit.

But you have a point wrt. SD_LBP_ZERO. It was used for one particular
type of array that did zero block detection (and thus didn't actually
write anything either). I don't think anybody is using this mode of
operation anymore since thin provisioning has been formalized in T10 for
quite a while. But I'd be OK with a patch that uses
SD_WRITE_SAME_TIMEOUT for that particular code path.

-- 
Martin K. Petersen	Oracle Linux Engineering