Re: [PATCH v3 2/2] Hard disk S3 resume time optimization

Re: [PATCH v3 2/2] Hard disk S3 resume time optimization

[Oliver Neukum]

On Fri, 2013-08-23 at 14:43 -0700, Brandt, Todd E wrote:
> This is v3 of the non-blocking S3 resume patch. It's been broken into
> two pieces, this part is for the scsi subsystem. I've addressed Alan 
> Stern's comments in particular by reformatting the call to conform
> to the proper style guidelines.

Hi,

you are changing the generic resume for all sd disks.
In particular, you are dropping the check of
sdkp->device->manage_start_stop
What happens if you use this patch on a real SCSI disk?

	Regards
		Oliver

RE: [PATCH v3 2/2] Hard disk S3 resume time optimization

[Brandt, Todd E]

All the same checks are preserved, including the manage_start_stop check:

+	if (!sdkp->device->manage_start_stop)
+		goto error;

That function is just a simplified version of sd_start_stop_device but with only the start functionality used.

-----Original Message-----
From: Oliver Neukum [mailto:oneukum@suse.de] 
Sent: Saturday, August 24, 2013 1:21 AM
To: Brandt, Todd E
Cc: linux-ide@vger.kernel.org; linux-scsi@vger.kernel.org; tj@kernel.org; gregkh@linuxfoundation.org; stern@rowland.harvard.edu
Subject: Re: [PATCH v3 2/2] Hard disk S3 resume time optimization

On Fri, 2013-08-23 at 14:43 -0700, Brandt, Todd E wrote:
> This is v3 of the non-blocking S3 resume patch. It's been broken into
> two pieces, this part is for the scsi subsystem. I've addressed Alan 
> Stern's comments in particular by reformatting the call to conform
> to the proper style guidelines.

Hi,

you are changing the generic resume for all sd disks.
In particular, you are dropping the check of
sdkp->device->manage_start_stop
What happens if you use this patch on a real SCSI disk?

	Regards
		Oliver

[PATCH v3 2/2] Hard disk S3 resume time optimization

[Todd E Brandt]

On resume, the SD driver currently waits until the block driver finishes
executing a disk start command with blk_execute_rq. This patch changes
the sd_resume callback to use blk_execute_rq_nowait instead, which allows
it to return immediately, thus allowing the next device in the pm queue
to resume. The return value of blk_execute_rq_nowait is handled in the
background by sd_resume_complete. Any commands issued to the scsi disk
during the startup will be queued up and executed once the disk is online.
Thus no information is lost.

This patch applies to all three resume callbacks: resume, restore, and
runtime-resume. There is only a performance benefit for resume, but for
simplicity both restore and runtime-resume use the same code path.

The execution flow has changed from a single, synchronous call, to two
calls: one called to start sd_resume asynchronously, the other called on
completion. Thus the dmesg log will now show two prints for each drive
resume, on resume start and complete. The scsi sense data is managed
the same, but is now analyzed and freed in sd_resume_complete.

This code copies a portion of sd_start_stop_device, scsi_execute_req_flags,
and scsi_execute directly into sd_resume: effectively circumventing
sd_start_stop_device to start disks. This is to enable only the START_STOP
command to use blk_execute_rq_nowait instead of blk_execute_rq. So
sd_start_stop_device is now only used to stop the device. The disk is
started from within the sd_resume call itself. Another approach might be to
create an async version of scsi_execute to better preserve the code path
but I'm reticent to allow any other scsi commands to execute asynchronously.

Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

 drivers/scsi/sd.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 67 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 69725f7..c780f75 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3172,18 +3172,82 @@ static int sd_suspend_runtime(struct device *dev)
 	return sd_suspend_common(dev, false);
 }
 
+static void sd_resume_complete(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(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 done;
+		goto error;
 
 	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
-	ret = sd_start_stop_device(sdkp, 1);
 
-done:
+	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_complete);
+	return 0;
+
+ error_sense:
+	__blk_put_request(req->q, req);
+ error:
 	scsi_disk_put(sdkp);
 	return ret;
 }

[PATCH v3 2/2] Hard disk S3 resume time optimization

[Brandt, Todd E]

This is v3 of the non-blocking S3 resume patch. It's been broken into
two pieces, this part is for the scsi subsystem. I've addressed Alan 
Stern's comments in particular by reformatting the call to conform
to the proper style guidelines.

Note - the two patches will function separately but both are required 
to achieve a performance improvement.


Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>

 drivers/scsi/sd.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 86fcf2c..d4bf784 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -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).