Hard disk S3 resume time optimization
PATCH 2/2 Hard disk S3 resume time optimization
Part 2 of the hard disk resume optimization patch, this one applies to the scsi subsystem.
drivers/scsi/sd.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-)
drivers/scsi/sd.c +81 -1
@@ -107,6 +107,7 @@ static int sd_remove(struct device *);static void sd_shutdown (struct device *);static int sd_suspend (struct device *);static int sd_resume (struct device *); +static int sd_resume_async (struct device *);static void sd_rescan (struct device *);static int sd_done (struct scsi_cmnd *);static int sd_eh_action (struct scsi_cmnd *,unsigned char *,int ,int );
@@ -484,7 +485,7 @@ static struct class sd_disk_class = {static const struct dev_pm_ops sd_pm_ops = { .suspend = sd_suspend, - .resume = sd_resume, + .resume = sd_resume_async, .poweroff = sd_suspend, .restore = sd_resume, .runtime_suspend = sd_suspend,
@@ -3137,6 +3138,85 @@ done:return ret; } +static void sd_resume_async_end (struct request *rq,int error) +{ +struct scsi_sense_hdr sshdr; +struct scsi_disk *sdkp = rq->end_io_data; +char *sense = rq->sense; + +if (error) { +sd_printk (KERN_WARNING , sdkp,"START FAILED\n" ); +sd_print_result (sdkp, error); +if (sense && (driver_byte (error) &DRIVER_SENSE )) { +scsi_normalize_sense (sense, +SCSI_SENSE_BUFFERSIZE , &sshdr); +sd_print_sense_hdr (sdkp, &sshdr); + } + }else +sd_printk (KERN_NOTICE , sdkp,"START SUCCESS\n" ); + +kfree (sense); + rq->sense =NULL ; + rq->end_io_data =NULL ; +__blk_put_request (rq->q, rq); +scsi_disk_put (sdkp); +} + +static int sd_resume_async (struct device *dev) +{ +unsigned char cmd[6 ] = {START_STOP }; +struct scsi_disk *sdkp =scsi_disk_get_from_dev (dev); +struct request *req; +char *sense =NULL ; +int ret =0 ; + +if (!sdkp->device->manage_start_stop) + goto error; + +sd_printk (KERN_NOTICE , sdkp,"Starting disk\n" ); + + cmd[4 ] |=1 ; + +if (sdkp->device->start_stop_pwr_cond) + cmd[4 ] |=1 <<4 ;/* Active or Standby */ + +if (!scsi_device_online (sdkp->device)) { + ret = -ENODEV ; + goto error; + } + + req =blk_get_request (sdkp->device->request_queue,0 , __GFP_WAIT); +if (!req) { + ret = -ENOMEM ; + goto error; + } + + sense =kzalloc (SCSI_SENSE_BUFFERSIZE ,GFP_NOIO ); +if (!sense) { + ret = -ENOMEM ; + goto error_sense; + } + + req->cmd_len =COMMAND_SIZE (cmd[0 ]); +memcpy (req->cmd, cmd, req->cmd_len); + req->sense = sense; + req->sense_len =0 ; + req->retries =SD_MAX_RETRIES ; + req->timeout =SD_TIMEOUT ; + req->cmd_type =REQ_TYPE_BLOCK_PC ; + req->cmd_flags |=REQ_PM |REQ_QUIET |REQ_PREEMPT ; + + req->end_io_data = sdkp; +blk_execute_rq_nowait (req->q,NULL , req,1 , sd_resume_async_end); +return 0 ; + + error_sense: +__blk_put_request (req->q, req); + error: +scsi_disk_put (sdkp); +return ret; +} +/** * init_sd - entry point for this driver (both when built in or when * a module).
Hello.
Quoted 2 lines · Todd E Brandt
Part 2 of the hard disk resume optimization patch, this one applies to the scsi subsystem.
Don't give the same name to both patches. In this case, you could prefix
the patch name with e.g. "sd: ".
Quoted 10 lines
Signed-off-by Todd Brandt <[email protected]>Signed-off-by Arjan van de Ven <[email protected]>drivers/scsi/sd.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-)drivers/scsi/sd.c +0
-0
[...]
Quoted 10 lines
drivers/scsi/sd.c +6
-0@@ -3137,6 +3138,85 @@ done:return ret; } +static void sd_resume_async_end (struct request *rq,int error) +{ +struct scsi_sense_hdr sshdr; +struct scsi_disk *sdkp = rq->end_io_data; +char *sense = rq->sense; +
drivers/scsi/sd.c done: · line 3156
Quoted 10 lines
drivers/scsi/sd.c +10
-0+if (error) { +sd_printk (KERN_WARNING , sdkp,"START FAILED\n" ); +sd_print_result (sdkp, error); +if (sense && (driver_byte (error) &DRIVER_SENSE )) { +scsi_normalize_sense (sense, +SCSI_SENSE_BUFFERSIZE , &sshdr); +sd_print_sense_hdr (sdkp, &sshdr); + } + }else +sd_printk (KERN_NOTICE , sdkp,"START SUCCESS\n" );
According to Documentation/CodingStyle, both arms of the *if* statement
should have {} when one arm has them.
WBR, Sergei
James Bottomley
maintainerQuoted 2 lines · Todd E Brandt
Part 2 of the hard disk resume optimization patch, this one applies to the scsi subsystem.
This is a horrible patch description, it won't tell the next reader anything about what is going on and why. Give the patches two separate summaries and refer back by summary explaining what this one does.
Also, having two code paths do the same thing is always a bad idea:
someone will update one and not the other. sd_resume() should be come sd_resume_async(); wait for async event
James
Hi,
Quoted 2 lines · Todd E Brandt
Part 2 of the hard disk resume optimization patch, this one applies to the scsi subsystem.
Please update the patch description to say what the patch does (and why).
Quoted 19 lines
Signed-off-by Todd Brandt <[email protected]>Signed-off-by Arjan van de Ven <[email protected]>drivers/scsi/sd.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-)drivers/scsi/sd.c +1
-0@@ -107,6 +107,7 @@ static int sd_remove(struct device *);static void sd_shutdown (struct device *);static int sd_suspend (struct device *);static int sd_resume (struct device *); +static int sd_resume_async (struct device *);static void sd_rescan (struct device *);static int sd_done (struct scsi_cmnd *);static int sd_eh_action (struct scsi_cmnd *,unsigned char *,int ,int );
drivers/scsi/sd.c static struct class sd_disk_class = { · line 488
Quoted 5 lines
drivers/scsi/sd.c +1
-1@@ -484,7 +485,7 @@ static struct class sd_disk_class = {static const struct dev_pm_ops sd_pm_ops = { .suspend = sd_suspend, - .resume = sd_resume, + .resume = sd_resume_async,
Same comment as with patch #1.
When the command execution later fails the error will be now lost (->resume returned 0 already and nothing can be done to fix it).
Sorry but this looks like a wrong approach and you should fix the PM layer to do async ->resume when possible instead.
Quoted 79 lines
drivers/scsi/sd.c +72
-0.poweroff = sd_suspend, .restore = sd_resume, .runtime_suspend = sd_suspend,@@ -3137,6 +3138,85 @@ done:return ret; } +static void sd_resume_async_end (struct request *rq,int error) +{ +struct scsi_sense_hdr sshdr; +struct scsi_disk *sdkp = rq->end_io_data; +char *sense = rq->sense; + +if (error) { +sd_printk (KERN_WARNING , sdkp,"START FAILED\n" ); +sd_print_result (sdkp, error); +if (sense && (driver_byte (error) &DRIVER_SENSE )) { +scsi_normalize_sense (sense, +SCSI_SENSE_BUFFERSIZE , &sshdr); +sd_print_sense_hdr (sdkp, &sshdr); + } + }else +sd_printk (KERN_NOTICE , sdkp,"START SUCCESS\n" ); + +kfree (sense); + rq->sense =NULL ; + rq->end_io_data =NULL ; +__blk_put_request (rq->q, rq); +scsi_disk_put (sdkp); +} + +static int sd_resume_async (struct device *dev) +{ +unsigned char cmd[6 ] = {START_STOP }; +struct scsi_disk *sdkp =scsi_disk_get_from_dev (dev); +struct request *req; +char *sense =NULL ; +int ret =0 ; + +if (!sdkp->device->manage_start_stop) + goto error; + +sd_printk (KERN_NOTICE , sdkp,"Starting disk\n" ); + + cmd[4 ] |=1 ; + +if (sdkp->device->start_stop_pwr_cond) + cmd[4 ] |=1 <<4 ;/* Active or Standby */ + +if (!scsi_device_online (sdkp->device)) { + ret = -ENODEV ; + goto error; + } + + req =blk_get_request (sdkp->device->request_queue,0 , __GFP_WAIT); +if (!req) { + ret = -ENOMEM ; + goto error; + } + + sense =kzalloc (SCSI_SENSE_BUFFERSIZE ,GFP_NOIO ); +if (!sense) { + ret = -ENOMEM ; + goto error_sense; + } + + req->cmd_len =COMMAND_SIZE (cmd[0 ]); +memcpy (req->cmd, cmd, req->cmd_len); + req->sense = sense; + req->sense_len =0 ; + req->retries =SD_MAX_RETRIES ; + req->timeout =SD_TIMEOUT ; + req->cmd_type =REQ_TYPE_BLOCK_PC ; + req->cmd_flags |=REQ_PM |REQ_QUIET |REQ_PREEMPT ; + + req->end_io_data = sdkp; +blk_execute_rq_nowait (req->q,NULL , req,1 , sd_resume_async_end); +return 0 ; +
drivers/scsi/sd.c done: · line 3222
Quoted 10 lines
drivers/scsi/sd.c +7
-0+ error_sense: +__blk_put_request (req->q, req); + error: +scsi_disk_put (sdkp); +return ret; +} +/** * init_sd - entry point for this driver (both when built in or when * a module).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Sorry for the missing description, I've submitted it several times so I just assumed that the previous entries in the thread were enough. I'll resubmit with a full description.
As for replacing sd_resume altogether, the reason I didn't is because there are three callbacks which use sd_resume. I've only tested this functionality for the system resume call, not restore or runtime_resume, and I didn't know if the other two made sense as asynchronous since it doesn't really buy anything in runtime_resume, but I understand your point that it adds unnecessary complexity. I'll fix it.
I'm sure there are no issues but I'll be sure and test all the new scenerios before I resubmit.
So, with fixes to those two issues, do you think this technique is something you'd accept? I've tried it two different ways, one where I changed the pm code to allow nonblocking resume callbacks, which just finalized system resume without waiting for the ata and scsi drivers to finish. But that didn't fly well with Rafael as it violates some implicit assumptions about how the callbacks function. I worked with Rafael to come up with a "skip_resume" version which just skipped the system resume of the disks, changed them to runtime suspended, and let them runtime resume whenever they were accessed, but it proved to be impractical in a real OS environment because the OS tends to access everything at once when it comes back online.
This approach is really the only remaining way (that I can think of) to remove the ata-port wakeup delay in software, so given the massive performance benefit I hope you'll consider it. Thanks for the reply!
Quoted 15 lines · James Bottomley
Quoted 2 lines · Todd E Brandt
Part 2 of the hard disk resume optimization patch, this one applies to the scsi subsystem.
This is a horrible patch description, it won't tell the next reader anything about what is going on and why. Give the patches two separate summaries and refer back by summary explaining what this one does.
Also, having two code paths do the same thing is always a bad idea:
someone will update one and not the other.sd_resume()should be comesd_resume_async(); wait for async eventJames
I'll fix those, thanks for the feedback.
Quoted 47 lines · Sergei Shtylyov
Hello.
Quoted 2 lines · Todd E Brandt
Part 2 of the hard disk resume optimization patch, this one applies to the scsi subsystem.
Don't give the same name to both patches. In this case, you could
prefix the patch name with e.g. "sd: ".Quoted 10 lines
Signed-off-by Todd Brandt <[email protected]>Signed-off-by Arjan van de Ven <[email protected]>drivers/scsi/sd.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-)drivers/scsi/sd.c +0
-0[...]
Quoted 10 lines
drivers/scsi/sd.c +6
-0@@ -3137,6 +3138,85 @@ done:return ret; } +static void sd_resume_async_end (struct request *rq,int error) +{ +struct scsi_sense_hdr sshdr; +struct scsi_disk *sdkp = rq->end_io_data; +char *sense = rq->sense; +drivers/scsi/sd.c done: · line 3156
Quoted 10 lines
drivers/scsi/sd.c +10
-0+if (error) { +sd_printk (KERN_WARNING , sdkp,"START FAILED\n" ); +sd_print_result (sdkp, error); +if (sense && (driver_byte (error) &DRIVER_SENSE )) { +scsi_normalize_sense (sense, +SCSI_SENSE_BUFFERSIZE , &sshdr); +sd_print_sense_hdr (sdkp, &sshdr); + } + }else +sd_printk (KERN_NOTICE , sdkp,"START SUCCESS\n" );According to
Documentation/CodingStyle, both arms of the *if*
statement should have {} when one arm has them.WBR, Sergei
I'll resubmit with a better description. As for the return value, if you look at the mailing list history you'll see that I already tried that approach (and was actually advised to do it this way):
http://marc.info/?l=linux-scsi&m=136881989601276&w=2
Quoted 138 lines · Bartlomiej Zolnierkiewicz
Hi,
Quoted 2 lines · Todd E Brandt
Part 2 of the hard disk resume optimization patch, this one applies to the scsi subsystem.
Please update the patch description to say what the patch does (and why).
Quoted 19 lines
Signed-off-by Todd Brandt <[email protected]>Signed-off-by Arjan van de Ven <[email protected]>drivers/scsi/sd.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-)drivers/scsi/sd.c +1
-0@@ -107,6 +107,7 @@ static int sd_remove(struct device *);static void sd_shutdown (struct device *);static int sd_suspend (struct device *);static int sd_resume (struct device *); +static int sd_resume_async (struct device *);static void sd_rescan (struct device *);static int sd_done (struct scsi_cmnd *);static int sd_eh_action (struct scsi_cmnd *,unsigned char *,int ,int );drivers/scsi/sd.c static struct class sd_disk_class = { · line 488
Quoted 5 lines
drivers/scsi/sd.c +1
-1@@ -484,7 +485,7 @@ static struct class sd_disk_class = {static const struct dev_pm_ops sd_pm_ops = { .suspend = sd_suspend, - .resume = sd_resume, + .resume = sd_resume_async,Same comment as with patch #1.
When the command execution later fails the error will be now lost (->resume returned 0 already and nothing can be done to fix it).
Sorry but this looks like a wrong approach and you should fix the PM layer to do async ->resume when possible instead.
Quoted 79 lines
drivers/scsi/sd.c +72
-0.poweroff = sd_suspend, .restore = sd_resume, .runtime_suspend = sd_suspend,@@ -3137,6 +3138,85 @@ done:return ret; } +static void sd_resume_async_end (struct request *rq,int error) +{ +struct scsi_sense_hdr sshdr; +struct scsi_disk *sdkp = rq->end_io_data; +char *sense = rq->sense; + +if (error) { +sd_printk (KERN_WARNING , sdkp,"START FAILED\n" ); +sd_print_result (sdkp, error); +if (sense && (driver_byte (error) &DRIVER_SENSE )) { +scsi_normalize_sense (sense, +SCSI_SENSE_BUFFERSIZE , &sshdr); +sd_print_sense_hdr (sdkp, &sshdr); + } + }else +sd_printk (KERN_NOTICE , sdkp,"START SUCCESS\n" ); + +kfree (sense); + rq->sense =NULL ; + rq->end_io_data =NULL ; +__blk_put_request (rq->q, rq); +scsi_disk_put (sdkp); +} + +static int sd_resume_async (struct device *dev) +{ +unsigned char cmd[6 ] = {START_STOP }; +struct scsi_disk *sdkp =scsi_disk_get_from_dev (dev); +struct request *req; +char *sense =NULL ; +int ret =0 ; + +if (!sdkp->device->manage_start_stop) + goto error; + +sd_printk (KERN_NOTICE , sdkp,"Starting disk\n" ); + + cmd[4 ] |=1 ; + +if (sdkp->device->start_stop_pwr_cond) + cmd[4 ] |=1 <<4 ;/* Active or Standby */ + +if (!scsi_device_online (sdkp->device)) { + ret = -ENODEV ; + goto error; + } + + req =blk_get_request (sdkp->device->request_queue,0 , __GFP_WAIT); +if (!req) { + ret = -ENOMEM ; + goto error; + } + + sense =kzalloc (SCSI_SENSE_BUFFERSIZE ,GFP_NOIO ); +if (!sense) { + ret = -ENOMEM ; + goto error_sense; + } + + req->cmd_len =COMMAND_SIZE (cmd[0 ]); +memcpy (req->cmd, cmd, req->cmd_len); + req->sense = sense; + req->sense_len =0 ; + req->retries =SD_MAX_RETRIES ; + req->timeout =SD_TIMEOUT ; + req->cmd_type =REQ_TYPE_BLOCK_PC ; + req->cmd_flags |=REQ_PM |REQ_QUIET |REQ_PREEMPT ; + + req->end_io_data = sdkp; +blk_execute_rq_nowait (req->q,NULL , req,1 , sd_resume_async_end); +return 0 ; +drivers/scsi/sd.c done: · line 3222
Quoted 10 lines
drivers/scsi/sd.c +7
-0+ error_sense: +__blk_put_request (req->q, req); + error: +scsi_disk_put (sdkp); +return ret; +} +/** * init_sd - entry point for this driver (both when built in or when * a module). Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics