Re: [PATCH 2/3] sd: implement START/STOP management
Tejun Heo wrote: Hello, Douglas. Douglas Gilbert wrote: Tejun, I note at this point that the IMMED bit in the START STOP UNIT cdb is clear. [The code might note that as well.] All SCSI disks that I have seen, implement the IMMED bit and according to the SAT standard, so should SAT layers like the one in libata. With the IMMED bit clear: - on spin up, it will wait until disk is ready. Okay unless there are a lot of disks, in which case we could ask Matthew Wilcox for help - on spin down, will wait until media is stopped. That could be 20 seconds, and if there were multiple disks I guess the question is do we need to wait until a disk is spun down before dropping power to it and suspending. I think we do. As we're issuing SYNCHRONIZE CACHE prior to spinning down disks, it's probably okay to drop power early data-integrity-wise but still... We can definitely use IMMED=1 during resume (needs to be throttled somehow tho). This helps even when there is only one disk. We can let the disk spin up in the background and proceed with the rest of resuming process. Unfortunately, libata SAT layer doesn't do IMMED and even if it does (I've tried and have a patch available) it doesn't really work because during host resume each port enters EH and resets and revalidates each device. Many if not most ATA harddisks don't respond to reset or IDENTIFY till it's fully spun up meaning libata EH has to wait for all drives to spin up. libata EH runs inside SCSI EH thread meaning SCSI comman issue blocks till libata EH finishes resetting the port. So, IMMED or not, sd gotta wait for libata disks. If we want to do parallel spin down, PM core needs to be updated such that there are two events - issue and done - somewhat similar to what SCSI is doing to probe devices parallelly. If we're gonna do that, we maybe can apply the same mechanism to resume path so that we can do things parallelly IMMED or not. Seems, there is another way of doing a bank spin up / spin down: doing it in two passes. On the first pass START_STOP will be issued with IMMED=1 on all devices, then on the second pass START_STOP will be issued with IMMED=0. So the devices will spin up / spin down in the parallel, but synchronously, hence the needed result will be achieved with minimal code changes, although it will indeed need upper layer changes in struct device_driver's suspend(), resume(), etc. callers. Vlad - 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 2/3] sd: implement START/STOP management
Henrique de Moraes Holschuh wrote: On Thu, 22 Mar 2007, Vladislav Bolkhovitin wrote: Seems, there is another way of doing a bank spin up / spin down: doing it in two passes. On the first pass START_STOP will be issued with IMMED=1 on all devices, then on the second pass START_STOP will be issued with IMMED=0. So the devices will spin up / spin down in the parallel, but synchronously, hence the needed result will be achieved And maybe trip the PSU's overcurrent defenses? There is a reason to default to sequential spin-up for disks... But on spin down there is no such problem Of course, it can be user-selectable. But should it be the default? - 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
[PATCH 2/3] sd: implement START/STOP management
Implement SBC START/STOP management. sdev-mange_start_stop is added.
When it's set to one, sd STOPs the device on suspend and shutdown and
STARTs it on resume. sdev-manage_start_stop defaults is in sdev
instead of scsi_disk cdev to allow -slave_config() override the
default configuration but is exported under scsi_disk sysfs node as
sdev-allow_restart is.
When manage_start_stop is zero (the default value), this patch doesn't
introduce any behavior change.
Signed-off-by: Tejun Heo [EMAIL PROTECTED]
---
drivers/scsi/scsi_sysfs.c | 31 +++--
drivers/scsi/sd.c | 102 +
include/scsi/scsi_device.h |1
3 files changed, 130 insertions(+), 4 deletions(-)
Index: work/drivers/scsi/sd.c
===
--- work.orig/drivers/scsi/sd.c
+++ work/drivers/scsi/sd.c
@@ -142,6 +142,8 @@ static void sd_rw_intr(struct scsi_cmnd
static int sd_probe(struct device *);
static int sd_remove(struct device *);
static void sd_shutdown(struct device *dev);
+static int sd_suspend(struct device *dev, pm_message_t state);
+static int sd_resume(struct device *dev);
static void sd_rescan(struct device *);
static int sd_init_command(struct scsi_cmnd *);
static int sd_issue_flush(struct device *, sector_t *);
@@ -206,6 +208,20 @@ static ssize_t sd_store_cache_type(struc
return count;
}
+static ssize_t sd_store_manage_start_stop(struct class_device *cdev,
+ const char *buf, size_t count)
+{
+ struct scsi_disk *sdkp = to_scsi_disk(cdev);
+ struct scsi_device *sdp = sdkp-device;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ sdp-manage_start_stop = simple_strtoul(buf, NULL, 10);
+
+ return count;
+}
+
static ssize_t sd_store_allow_restart(struct class_device *cdev, const char
*buf,
size_t count)
{
@@ -238,6 +254,14 @@ static ssize_t sd_show_fua(struct class_
return snprintf(buf, 20, %u\n, sdkp-DPOFUA);
}
+static ssize_t sd_show_manage_start_stop(struct class_device *cdev, char *buf)
+{
+ struct scsi_disk *sdkp = to_scsi_disk(cdev);
+ struct scsi_device *sdp = sdkp-device;
+
+ return snprintf(buf, 20, %u\n, sdp-manage_start_stop);
+}
+
static ssize_t sd_show_allow_restart(struct class_device *cdev, char *buf)
{
struct scsi_disk *sdkp = to_scsi_disk(cdev);
@@ -251,6 +275,8 @@ static struct class_device_attribute sd_
__ATTR(FUA, S_IRUGO, sd_show_fua, NULL),
__ATTR(allow_restart, S_IRUGO|S_IWUSR, sd_show_allow_restart,
sd_store_allow_restart),
+ __ATTR(manage_start_stop, S_IRUGO|S_IWUSR, sd_show_manage_start_stop,
+ sd_store_manage_start_stop),
__ATTR_NULL,
};
@@ -267,6 +293,8 @@ static struct scsi_driver sd_template =
.name = sd,
.probe = sd_probe,
.remove = sd_remove,
+ .suspend= sd_suspend,
+ .resume = sd_resume,
.shutdown = sd_shutdown,
},
.rescan = sd_rescan,
@@ -1776,6 +1804,32 @@ static void scsi_disk_release(struct cla
kfree(sdkp);
}
+static int sd_start_stop_device(struct scsi_device *sdp, int start)
+{
+ unsigned char cmd[6] = { START_STOP }; /* START_VALID */
+ struct scsi_sense_hdr sshdr;
+ int res;
+
+ if (start)
+ cmd[4] |= 1;/* START */
+
+ if (!scsi_device_online(sdp))
+ return -ENODEV;
+
+ res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, sshdr,
+ SD_TIMEOUT, SD_MAX_RETRIES);
+ if (res) {
+ printk(KERN_WARNING FAILED\n status = %x, message = %02x,
+ host = %d, driver = %02x\n ,
+ status_byte(res), msg_byte(res),
+ host_byte(res), driver_byte(res));
+ if (driver_byte(res) DRIVER_SENSE)
+ scsi_print_sense_hdr(sd, sshdr);
+ }
+
+ return res;
+}
+
/*
* Send a SYNCHRONIZE CACHE instruction down to the device through
* the normal SCSI command structure. Wait for the command to
@@ -1794,9 +1848,57 @@ static void sd_shutdown(struct device *d
sdkp-disk-disk_name);
sd_sync_cache(sdp);
}
+
+ if (system_state != SYSTEM_RESTART sdp-manage_start_stop) {
+ printk(KERN_NOTICE Stopping disk %s: \n,
+ sdkp-disk-disk_name);
+ sd_start_stop_device(sdp, 0);
+ }
+
scsi_disk_put(sdkp);
}
+static int sd_suspend(struct device *dev, pm_message_t mesg)
+{
+ struct scsi_device *sdp = to_scsi_device(dev);
+ struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
+ int ret;
+
+ if (!sdkp)
+ return 0; /*
Re: [PATCH 2/3] sd: implement START/STOP management
On Wed, 2007-03-21 at 00:13 +0900, Tejun Heo wrote: Implement SBC START/STOP management. sdev-mange_start_stop is added. When it's set to one, sd STOPs the device on suspend and shutdown and STARTs it on resume. sdev-manage_start_stop defaults is in sdev instead of scsi_disk cdev to allow -slave_config() override the default configuration but is exported under scsi_disk sysfs node as sdev-allow_restart is. When manage_start_stop is zero (the default value), this patch doesn't introduce any behavior change. I have this in scsi-misc-2.6. There was a minor space before tab no-no which git complained about (and I fixed). Unfortunately, there's a pretty major merge conflict with prior sd.c patches in scsi-misc-2.6. I've merged and fixed this but it's going to create a bit of a difficulty for applying the libata pieces. James - 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 2/3] sd: implement START/STOP management
On Wed, 2007-03-21 at 02:08 +0900, Tejun Heo wrote:
I got too comfortable with libata-dev#upstream and forgot to verify
patches against scsi-misc-2.6. Sorry about that. If you don't have
objection against the content, I'll resubmit the SCSI part against
scsi-misc-2.6. Once it's in scsi-misc-2.6, we can pull it into
libata-dev#upstream and do the libata part there. As the SCSI change
doesn't break libata, they can be done separately.
I should already have it in scsi-misc-2.6 ... you could verify I got the
merge right ...
There'll be an additional piece to put the new sd_printk helpers into
the routines, which I attach below.
James
---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 49a94ae..b044dcf 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1733,10 +1733,11 @@ static void scsi_disk_release(struct class_device *cdev)
kfree(sdkp);
}
-static int sd_start_stop_device(struct scsi_device *sdp, int start)
+static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
{
unsigned char cmd[6] = { START_STOP }; /* START_VALID */
struct scsi_sense_hdr sshdr;
+ struct scsi_device *sdp = sdkp-device;
int res;
if (start)
@@ -1748,12 +1749,10 @@ static int sd_start_stop_device(struct scsi_device
*sdp, int start)
res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, sshdr,
SD_TIMEOUT, SD_MAX_RETRIES);
if (res) {
- printk(KERN_WARNING FAILED\n status = %x, message = %02x,
- host = %d, driver = %02x\n ,
- status_byte(res), msg_byte(res),
- host_byte(res), driver_byte(res));
+ sd_printk(KERN_WARNING, sdkp, START_STOP FAILED\n);
+ sd_print_result(sdkp, res);
if (driver_byte(res) DRIVER_SENSE)
- scsi_print_sense_hdr(sd, sshdr);
+ sd_print_sense_hdr(sdkp, sshdr);
}
return res;
@@ -1766,7 +1765,6 @@ static int sd_start_stop_device(struct scsi_device *sdp,
int start)
*/
static void sd_shutdown(struct device *dev)
{
- struct scsi_device *sdp = to_scsi_device(dev);
struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
if (!sdkp)
@@ -1777,10 +1775,9 @@ static void sd_shutdown(struct device *dev)
sd_sync_cache(sdkp);
}
- if (system_state != SYSTEM_RESTART sdp-manage_start_stop) {
- printk(KERN_NOTICE Stopping disk %s: \n,
- sdkp-disk-disk_name);
- sd_start_stop_device(sdp, 0);
+ if (system_state != SYSTEM_RESTART sdkp-device-manage_start_stop) {
+ sd_printk(KERN_NOTICE, sdkp, Stopping disk\n);
+ sd_start_stop_device(sdkp, 0);
}
scsi_disk_put(sdkp);
@@ -1788,7 +1785,6 @@ static void sd_shutdown(struct device *dev)
static int sd_suspend(struct device *dev, pm_message_t mesg)
{
- struct scsi_device *sdp = to_scsi_device(dev);
struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
int ret;
@@ -1796,17 +1792,16 @@ static int sd_suspend(struct device *dev, pm_message_t
mesg)
return 0; /* this can happen */
if (sdkp-WCE) {
- printk(KERN_NOTICE Synchronizing SCSI cache for disk %s: \n,
- sdkp-disk-disk_name);
+ sd_printk(KERN_NOTICE, sdkp, Synchronizing SCSI cache\n);
ret = sd_sync_cache(sdkp);
if (ret)
return ret;
}
- if (mesg.event == PM_EVENT_SUSPEND sdp-manage_start_stop) {
- printk(KERN_NOTICE Stopping disk %s: \n,
- sdkp-disk-disk_name);
- ret = sd_start_stop_device(sdp, 0);
+ if (mesg.event == PM_EVENT_SUSPEND
+ sdkp-device-manage_start_stop) {
+ sd_printk(KERN_NOTICE, sdkp, Stopping disk\n);
+ ret = sd_start_stop_device(sdkp, 0);
if (ret)
return ret;
}
@@ -1816,15 +1811,14 @@ static int sd_suspend(struct device *dev, pm_message_t
mesg)
static int sd_resume(struct device *dev)
{
- struct scsi_device *sdp = to_scsi_device(dev);
struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
- if (!sdp-manage_start_stop)
+ if (!sdkp-device-manage_start_stop)
return 0;
- printk(KERN_NOTICE Starting disk %s: \n, sdkp-disk-disk_name);
+ sd_printk(KERN_NOTICE, sdkp, Starting disk\n);
- return sd_start_stop_device(sdp, 1);
+ return sd_start_stop_device(sdkp, 1);
}
/**
-
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 2/3] sd: implement START/STOP management
Tejun Heo wrote:
Implement SBC START/STOP management. sdev-mange_start_stop is added.
When it's set to one, sd STOPs the device on suspend and shutdown and
STARTs it on resume. sdev-manage_start_stop defaults is in sdev
instead of scsi_disk cdev to allow -slave_config() override the
default configuration but is exported under scsi_disk sysfs node as
sdev-allow_restart is.
When manage_start_stop is zero (the default value), this patch doesn't
introduce any behavior change.
Signed-off-by: Tejun Heo [EMAIL PROTECTED]
---
drivers/scsi/scsi_sysfs.c | 31 +++--
drivers/scsi/sd.c | 102
+
include/scsi/scsi_device.h |1
3 files changed, 130 insertions(+), 4 deletions(-)
Index: work/drivers/scsi/sd.c
===
--- work.orig/drivers/scsi/sd.c
+++ work/drivers/scsi/sd.c
@@ -142,6 +142,8 @@ static void sd_rw_intr(struct scsi_cmnd
static int sd_probe(struct device *);
static int sd_remove(struct device *);
static void sd_shutdown(struct device *dev);
+static int sd_suspend(struct device *dev, pm_message_t state);
+static int sd_resume(struct device *dev);
static void sd_rescan(struct device *);
static int sd_init_command(struct scsi_cmnd *);
static int sd_issue_flush(struct device *, sector_t *);
@@ -206,6 +208,20 @@ static ssize_t sd_store_cache_type(struc
return count;
}
+static ssize_t sd_store_manage_start_stop(struct class_device *cdev,
+ const char *buf, size_t count)
+{
+ struct scsi_disk *sdkp = to_scsi_disk(cdev);
+ struct scsi_device *sdp = sdkp-device;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ sdp-manage_start_stop = simple_strtoul(buf, NULL, 10);
+
+ return count;
+}
+
static ssize_t sd_store_allow_restart(struct class_device *cdev, const char
*buf,
size_t count)
{
@@ -238,6 +254,14 @@ static ssize_t sd_show_fua(struct class_
return snprintf(buf, 20, %u\n, sdkp-DPOFUA);
}
+static ssize_t sd_show_manage_start_stop(struct class_device *cdev, char
*buf)
+{
+ struct scsi_disk *sdkp = to_scsi_disk(cdev);
+ struct scsi_device *sdp = sdkp-device;
+
+ return snprintf(buf, 20, %u\n, sdp-manage_start_stop);
+}
+
static ssize_t sd_show_allow_restart(struct class_device *cdev, char *buf)
{
struct scsi_disk *sdkp = to_scsi_disk(cdev);
@@ -251,6 +275,8 @@ static struct class_device_attribute sd_
__ATTR(FUA, S_IRUGO, sd_show_fua, NULL),
__ATTR(allow_restart, S_IRUGO|S_IWUSR, sd_show_allow_restart,
sd_store_allow_restart),
+ __ATTR(manage_start_stop, S_IRUGO|S_IWUSR, sd_show_manage_start_stop,
+sd_store_manage_start_stop),
__ATTR_NULL,
};
@@ -267,6 +293,8 @@ static struct scsi_driver sd_template =
.name = sd,
.probe = sd_probe,
.remove = sd_remove,
+ .suspend= sd_suspend,
+ .resume = sd_resume,
.shutdown = sd_shutdown,
},
.rescan = sd_rescan,
@@ -1776,6 +1804,32 @@ static void scsi_disk_release(struct cla
kfree(sdkp);
}
+static int sd_start_stop_device(struct scsi_device *sdp, int start)
+{
+ unsigned char cmd[6] = { START_STOP }; /* START_VALID */
+ struct scsi_sense_hdr sshdr;
+ int res;
+
+ if (start)
+ cmd[4] |= 1;/* START */
+
+ if (!scsi_device_online(sdp))
+ return -ENODEV;
+
+ res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, sshdr,
+SD_TIMEOUT, SD_MAX_RETRIES);
Tejun,
I note at this point that the IMMED bit in the
START STOP UNIT cdb is clear. [The code might
note that as well.] All SCSI disks that I have
seen, implement the IMMED bit and according to
the SAT standard, so should SAT layers like the
one in libata.
With the IMMED bit clear:
- on spin up, it will wait until disk is ready.
Okay unless there are a lot of disks, in
which case we could ask Matthew Wilcox for help
- on spin down, will wait until media is
stopped. That could be 20 seconds, and if there
were multiple disks
I guess the question is do we need to wait until a
disk is spun down before dropping power to it
and suspending.
Doug Gilbert
-
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 2/3] sd: implement START/STOP management
On Tue, 2007-03-20 at 13:58 -0400, Douglas Gilbert wrote: I note at this point that the IMMED bit in the START STOP UNIT cdb is clear. [The code might note that as well.] All SCSI disks that I have seen, implement the IMMED bit and according to the SAT standard, so should SAT layers like the one in libata. With the IMMED bit clear: - on spin up, it will wait until disk is ready. Okay unless there are a lot of disks, in which case we could ask Matthew Wilcox for help - on spin down, will wait until media is stopped. That could be 20 seconds, and if there were multiple disks I guess the question is do we need to wait until a disk is spun down before dropping power to it and suspending. Realistically, if you're thinking of doing a bank spin up / spin down with IMMED=1 I suspect we can wait and see if anyone has sufficient need for it to be implemented before trying. The majority of the suspend/resume cases are laptops with a single disc ... and there immed is irrelevant to them. I think we need assurance that the head is parked before full suspend, yes, so IMMED=0 seems the easiest way to do this. James - 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 2/3] sd: implement START/STOP management
Hello, Douglas. Douglas Gilbert wrote: Tejun, I note at this point that the IMMED bit in the START STOP UNIT cdb is clear. [The code might note that as well.] All SCSI disks that I have seen, implement the IMMED bit and according to the SAT standard, so should SAT layers like the one in libata. With the IMMED bit clear: - on spin up, it will wait until disk is ready. Okay unless there are a lot of disks, in which case we could ask Matthew Wilcox for help - on spin down, will wait until media is stopped. That could be 20 seconds, and if there were multiple disks I guess the question is do we need to wait until a disk is spun down before dropping power to it and suspending. I think we do. As we're issuing SYNCHRONIZE CACHE prior to spinning down disks, it's probably okay to drop power early data-integrity-wise but still... We can definitely use IMMED=1 during resume (needs to be throttled somehow tho). This helps even when there is only one disk. We can let the disk spin up in the background and proceed with the rest of resuming process. Unfortunately, libata SAT layer doesn't do IMMED and even if it does (I've tried and have a patch available) it doesn't really work because during host resume each port enters EH and resets and revalidates each device. Many if not most ATA harddisks don't respond to reset or IDENTIFY till it's fully spun up meaning libata EH has to wait for all drives to spin up. libata EH runs inside SCSI EH thread meaning SCSI comman issue blocks till libata EH finishes resetting the port. So, IMMED or not, sd gotta wait for libata disks. If we want to do parallel spin down, PM core needs to be updated such that there are two events - issue and done - somewhat similar to what SCSI is doing to probe devices parallelly. If we're gonna do that, we maybe can apply the same mechanism to resume path so that we can do things parallelly IMMED or not. 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 2/3] sd: implement START/STOP management
James Bottomley wrote: On Wed, 2007-03-21 at 02:08 +0900, Tejun Heo wrote: I got too comfortable with libata-dev#upstream and forgot to verify patches against scsi-misc-2.6. Sorry about that. If you don't have objection against the content, I'll resubmit the SCSI part against scsi-misc-2.6. Once it's in scsi-misc-2.6, we can pull it into libata-dev#upstream and do the libata part there. As the SCSI change doesn't break libata, they can be done separately. I should already have it in scsi-misc-2.6 ... you could verify I got the merge right ... There'll be an additional piece to put the new sd_printk helpers into the routines, which I attach below. Yeap, everything looks fine. 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
