[PATCH RESEND 1/2] SCSI: use scsi_device-timeout consistently
Each high level driver uses scsi_device-timeout diffrently.
* sd sets sdev-timeout to SD_TIMEOUT if it's zero on attach.
sdev-timeout is used for commands received from block queue but
internal commands use SD_TIMEOUT directly.
* sr uses constant SR_TIMEOUT for all non-ioctl commands and
IOCTL_TIMEOUT for sr ioctls.
* st sets sdev-timeout to ST_TIMEOUT unconditionally. st uses
either sdev-timeout or scsi_tape-long_timeout depending on command
type.
* osst uses osst_tape-timeout and osst_tape-long_timeout.
This patch unifies differing behaviors as follows.
* All highlevel drivers initialize sdev-timeout to its default value
iff it's zero; otherwise, the specified value is used.
* All commands w/o specific timeout requirement use sdev-timeout.
Commands which require special timeout uses HL-specific value such
as IOCTL_TIMEOUT and scsi/osst_tape-long_timeout.
This patch also adds sdev local variable for shorter notation where
multiple references to sdev are added.
Signed-off-by: Tejun Heo [EMAIL PROTECTED]
---
Sorry, the original post had [EMAIL PROTECTED] instead of
[EMAIL PROTECTED] Reposting.
drivers/scsi/osst.c | 78 ++--
drivers/scsi/osst.h |1
drivers/scsi/sd.c | 31 +++-
drivers/scsi/sr.c | 49 ++--
drivers/scsi/st.c |3 +-
5 files changed, 87 insertions(+), 75 deletions(-)
Index: scsi-misc-2.6/drivers/scsi/sd.c
===
--- scsi-misc-2.6.orig/drivers/scsi/sd.c
+++ scsi-misc-2.6/drivers/scsi/sd.c
@@ -186,7 +186,7 @@ static ssize_t sd_store_cache_type(struc
return -EINVAL;
rcd = ct 0x01 ? 1 : 0;
wce = ct 0x02 ? 1 : 0;
- if (scsi_mode_sense(sdp, 0x08, 8, buffer, sizeof(buffer), SD_TIMEOUT,
+ if (scsi_mode_sense(sdp, 0x08, 8, buffer, sizeof(buffer), sdp-timeout,
SD_MAX_RETRIES, data, NULL))
return -EINVAL;
len = min_t(size_t, sizeof(buffer), data.length - data.header_length -
@@ -197,7 +197,7 @@ static ssize_t sd_store_cache_type(struc
buffer_data[2] |= wce 2 | rcd;
sp = buffer_data[0] 0x80 ? 1 : 0;
- if (scsi_mode_select(sdp, 1, sp, 8, buffer_data, len, SD_TIMEOUT,
+ if (scsi_mode_select(sdp, 1, sp, 8, buffer_data, len, sdp-timeout,
SD_MAX_RETRIES, data, sshdr)) {
if (scsi_sense_valid(sshdr))
scsi_print_sense_hdr(sdkp-disk-disk_name, sshdr);
@@ -370,7 +370,6 @@ static int sd_init_command(struct scsi_c
struct gendisk *disk = rq-rq_disk;
sector_t block = rq-sector;
unsigned int this_count = SCpnt-request_bufflen 9;
- unsigned int timeout = sdp-timeout;
SCSI_LOG_HLQUEUE(1, printk(sd_init_command: disk=%s, block=%llu,
count=%d\n, disk-disk_name,
@@ -511,7 +510,7 @@ static int sd_init_command(struct scsi_c
SCpnt-transfersize = sdp-sector_size;
SCpnt-underflow = this_count 9;
SCpnt-allowed = SD_MAX_RETRIES;
- SCpnt-timeout_per_command = timeout;
+ SCpnt-timeout_per_command = sdp-timeout;
/*
* This is the completion routine we use. This is matched in terms
@@ -759,7 +758,7 @@ static int sd_media_changed(struct gendi
*/
retval = -ENODEV;
if (scsi_block_when_processing_errors(sdp))
- retval = scsi_test_unit_ready(sdp, SD_TIMEOUT, SD_MAX_RETRIES);
+ retval = scsi_test_unit_ready(sdp, sdp-timeout,
SD_MAX_RETRIES);
/*
* Unable to test, unit probably not ready. This usually
@@ -805,7 +804,7 @@ static int sd_sync_cache(struct scsi_dev
* flush everything.
*/
res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, sshdr,
- SD_TIMEOUT, SD_MAX_RETRIES);
+ sdp-timeout, SD_MAX_RETRIES);
if (res == 0)
break;
}
@@ -838,9 +837,12 @@ static int sd_issue_flush(struct device
static void sd_prepare_flush(request_queue_t *q, struct request *rq)
{
+ struct device *dev = rq-rq_disk-driverfs_dev;
+ struct scsi_device *sdp = to_scsi_device(dev);
+
memset(rq-cmd, 0, sizeof(rq-cmd));
rq-cmd_type = REQ_TYPE_BLOCK_PC;
- rq-timeout = SD_TIMEOUT;
+ rq-timeout = sdp-timeout;
rq-cmd[0] = SYNCHRONIZE_CACHE;
rq-cmd_len = 10;
}
@@ -1028,6 +1030,7 @@ static int media_not_present(struct scsi
static void
sd_spinup_disk(struct scsi_disk *sdkp, char *diskname)
{
+ struct scsi_device *sdp = sdkp-device;
unsigned char cmd[10];
unsigned long spintime_expire = 0;
int retries, spintime;
@@ -1046,9 +1049,9 @@ sd_spinup_disk(struct scsi_disk *sdkp, c
cmd[0] = TEST_UNIT_READY;
Re: [PATCH RESEND 2/2] SCSI: add scsi_device-retries
On Sun, 19 Nov 2006, Tejun Heo wrote: Add scsi_device-retries to provide generic control over command retries, which is very similar to sdev-timeout. The initial value is -1 and high level driver is free to override on attach if negative. Note that -1 has the same effect as 0 (no retry) and signifies that it's the default value. As with sdev-timeout, sdev-retries is exported under the device sysfs node. All high level drivers are converted to use it for retry value. However, there are exceptions. * sd_sync_cache() and sr::get_sectorsize() loop three more times around normal command execution on failure. * st uses three retry limits - MAX_RETRIES, MAX_WRITE_RETRIES and MAX_READY_RETRIES, which are all zero. This patch only converts MAX_RETRIES to sdev-retries. Defining WRITE and READY retries in terms of sdev-retries would make more sense. I am neither acking nor naking this now. The patch does not change st behavior but moves part of the retry strategy out of the driver. (Which is also good because it makes one of the retry limits user changeable.) Combining all retry counts is something that may not be a good thing. Below is justification why st has three different retry limits. For some devices one number of retries is not perfect for all functions. The firmware of tape devices usually retries quite thoroughly before returning error. This is why the default zero applies to most cases. MAX_WRITE_RETRIES is separate from MAX_RETRIES because a plausible strategy might be (maybe not any more) to allow no retries for write but allow retries for reading and repositioning. The writes should fail directly so that the minimum number of marginal errors will be written. The reads can be retried so that even marginal data can be recovered. (Note that this may lead to tape positioning errors and may not be a good strategy in all cases.) MAX_READY_RETRIES is used for commands that don't move the tape. This is a situation where retrying is probably harmless. -- Kai - 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
Re: [PATCH RESEND 1/2] SCSI: use scsi_device-timeout consistently
Kai Makisara wrote: On Sun, 19 Nov 2006, Tejun Heo wrote: Each high level driver uses scsi_device-timeout diffrently. ... Index: scsi-misc-2.6/drivers/scsi/st.c === --- scsi-misc-2.6.orig/drivers/scsi/st.c +++ scsi-misc-2.6/drivers/scsi/st.c @@ -3986,7 +3986,8 @@ static int st_probe(struct device *dev) tpnt-partition = 0; tpnt-new_partition = 0; tpnt-nbr_partitions = 0; - tpnt-device-timeout = ST_TIMEOUT; + if (!SDp-timeout) + SDp-timeout = ST_TIMEOUT; I don't think this should be done. It probably makes the default timeout way too short. The error recovery with tapes takes a long time. The user can change the timeout if the default (900 seconds) seems too long (or short). I don't think there is anything in the midlevel or low-level code to set the timeout based on the device characteristics. This is left to the ULD. Low level driver should configure timeout or retries to a known value iff it knows what it's doing, when it knows both transport and device-type specific characteristic. AFAICS, the only driver which modifies sdev-timeout is ipr and it does so only when it knows the device is of certain type. So, I don't think it will cause any trouble, and using different initialization in different ULDs is too subtle. Thanks. -- tejun - 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
Re: [PATCH RESEND 2/2] SCSI: add scsi_device-retries
Hello, Kai Makisara wrote: * st uses three retry limits - MAX_RETRIES, MAX_WRITE_RETRIES and MAX_READY_RETRIES, which are all zero. This patch only converts MAX_RETRIES to sdev-retries. Defining WRITE and READY retries in terms of sdev-retries would make more sense. I am neither acking nor naking this now. The patch does not change st behavior but moves part of the retry strategy out of the driver. (Which is also good because it makes one of the retry limits user changeable.) Combining all retry counts is something that may not be a good thing. Below is justification why st has three different retry limits. For some devices one number of retries is not perfect for all functions. The firmware of tape devices usually retries quite thoroughly before returning error. This is why the default zero applies to most cases. MAX_WRITE_RETRIES is separate from MAX_RETRIES because a plausible strategy might be (maybe not any more) to allow no retries for write but allow retries for reading and repositioning. The writes should fail directly so that the minimum number of marginal errors will be written. The reads can be retried so that even marginal data can be recovered. (Note that this may lead to tape positioning errors and may not be a good strategy in all cases.) MAX_READY_RETRIES is used for commands that don't move the tape. This is a situation where retrying is probably harmless. I see. Then, how about adding and initializing STp-write_retries and STp-ready_retries according to SDp-retries (some reasonable default value, say write_retries is always zero while ready_retries is round up of retries * 1.1) and export both through sysfs? That will give lower level primitive control as other ULDs do and also allow users to configure each timeout separately if necessary. Thanks. -- tejun - 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
