[RFC/PATCH] Deferred disk spinup during system resume

[RFC/PATCH] Deferred disk spinup during system resume

[maksim.rayskiy]

To speed up time-to-full-power transition do not execute spinup request synchronously.
Schedule EH work and complete the request immediately.

Signed-off-by: Maksim Rayskiy <maksim.rayskiy@gmail.com>
---
 drivers/ata/libata-core.c |    7 +++++++
 drivers/ata/libata-eh.c   |   40 ++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-scsi.c |    1 +
 include/linux/libata.h    |    1 +
 4 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 7f77c67..6ed42d0 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4978,6 +4978,13 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 	struct ata_link *link = qc->dev->link;
 	u8 prot = qc->tf.protocol;
 
+	if (unlikely(qc->flags & ATA_QCFLAG_VERIFY)) {
+		ata_port_schedule_eh(ap);
+		qc->scsidone(qc->scsicmd);
+		ata_qc_free(qc);
+		return;
+	}
+
 	/* Make sure only one non-NCQ command is outstanding.  The
 	 * check is skipped for old EH because it reuses active qc to
 	 * request ATAPI sense.
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 5e59050..0e6f6b1 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3252,6 +3252,43 @@ static int ata_eh_maybe_retry_flush(struct ata_device *dev)
 	return rc;
 }
 
+static int ata_eh_maybe_verify(struct ata_device *dev)
+{
+	struct ata_link *link = dev->link;
+	struct ata_taskfile tf;
+	unsigned int err_mask;
+	int rc = 0;
+
+	ata_tf_init(dev, &tf);
+
+	if (dev->flags & ATA_DFLAG_LBA) {
+		tf.flags |= ATA_TFLAG_LBA;
+
+		tf.lbah = 0x0;
+		tf.lbam = 0x0;
+		tf.lbal = 0x0;
+		tf.device |= ATA_LBA;
+	} else {
+		/* CHS */
+		tf.lbal = 0x1; /* sect */
+		tf.lbam = 0x0; /* cyl low */
+		tf.lbah = 0x0; /* cyl high */
+	}
+
+	tf.command = ATA_CMD_VERIFY;	/* READ VERIFY */
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_LBA48 | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
+	if (err_mask) {
+		ata_dev_printk(dev, KERN_WARNING,
+			       "READ_VERIFY failed Emask 0x%x\n", err_mask);
+		rc = -EIO;
+	}
+
+	return rc;
+}
+
 /**
  *	ata_eh_set_lpm - configure SATA interface power management
  *	@link: link to configure power management
@@ -3738,6 +3775,9 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 			rc = ata_eh_maybe_retry_flush(dev);
 			if (rc)
 				goto rest_fail;
+			rc = ata_eh_maybe_verify(dev);
+			if (rc)
+				goto rest_fail;
 		}
 
 	config_lpm:
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 66aa4be..fb7d5b8 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1311,6 +1311,7 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 		}
 
 		tf->command = ATA_CMD_VERIFY;	/* READ VERIFY */
+		qc->flags |= ATA_QCFLAG_VERIFY;
 	} else {
 		/* Some odd clown BIOSen issue spindown on power off (ACPI S4
 		 * or S5) causing some drives to spin up and down again.
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d947b12..1bb2d55 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -234,6 +234,7 @@ enum {
 	ATA_QCFLAG_CLEAR_EXCL	= (1 << 5), /* clear excl_link on completion */
 	ATA_QCFLAG_QUIET	= (1 << 6), /* don't report device error */
 	ATA_QCFLAG_RETRY	= (1 << 7), /* retry after failure */
+	ATA_QCFLAG_VERIFY	= (1 << 8), /* used to force spin up */
 
 	ATA_QCFLAG_FAILED	= (1 << 16), /* cmd failed and is owned by EH */
 	ATA_QCFLAG_SENSE_VALID	= (1 << 17), /* sense data valid */
-- 
1.7.0.4

Re: [RFC/PATCH] Deferred disk spinup during system resume

[Tejun Heo]

Hello, Maksim, Jeff.

On Tue, Jan 11, 2011 at 05:24:17PM -0800, maksim.rayskiy@gmail.com wrote:
> @@ -4978,6 +4978,13 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>  	struct ata_link *link = qc->dev->link;
>  	u8 prot = qc->tf.protocol;
>  
> +	if (unlikely(qc->flags & ATA_QCFLAG_VERIFY)) {
> +		ata_port_schedule_eh(ap);
> +		qc->scsidone(qc->scsicmd);
> +		ata_qc_free(qc);
> +		return;

I still prefer the original patch where EH is scheduled from the
translation layer.  This seems unnecessarily intrusive to me.  As I
wrote before, it's not like we have a clean translation anyway and I
think this better fits as impedance matching code in the translation
layer anyway.  But, that said, this is a rather ugly piece of code
which is necessary just to work around the fact that we live under
scsi, so as long as it works, one way or the other probably doesn't
matter all that much (the reason why I prefer the previous one, as it
doens't try to be pretty and just gets it done), so it's Jeff's call.

Thanks.

-- 
tejun

Re: [RFC/PATCH] Deferred disk spinup during system resume

[Jeff Garzik]

On 01/12/2011 06:21 AM, Tejun Heo wrote:
> Hello, Maksim, Jeff.
>
> On Tue, Jan 11, 2011 at 05:24:17PM -0800, maksim.rayskiy@gmail.com wrote:
>> @@ -4978,6 +4978,13 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>>   	struct ata_link *link = qc->dev->link;
>>   	u8 prot = qc->tf.protocol;
>>
>> +	if (unlikely(qc->flags&  ATA_QCFLAG_VERIFY)) {
>> +		ata_port_schedule_eh(ap);
>> +		qc->scsidone(qc->scsicmd);
>> +		ata_qc_free(qc);
>> +		return;
>
> I still prefer the original patch where EH is scheduled from the
> translation layer.  This seems unnecessarily intrusive to me.  As I
> wrote before, it's not like we have a clean translation anyway and I
> think this better fits as impedance matching code in the translation
> layer anyway.  But, that said, this is a rather ugly piece of code
> which is necessary just to work around the fact that we live under
> scsi, so as long as it works, one way or the other probably doesn't
> matter all that much (the reason why I prefer the previous one, as it
> doens't try to be pretty and just gets it done), so it's Jeff's call.

The previous patch breaks READ VERIFY translation for all cases -except- 
this one.

The bottom line is that this patch simply wants to trigger an ATA 
command, and return immediately, discarding the command results.  I'm 
not even sure a "run this command in background, and discard results" 
facility requires the EH.

	Jeff

Re: [RFC/PATCH] Deferred disk spinup during system resume

[Maksim Rayskiy]

>
> The bottom line is that this patch simply wants to trigger an ATA command,
> and return immediately, discarding the command results.  I'm not even sure a
> "run this command in background, and discard results" facility requires the
> EH.
>

This is why I was asking if using a workqueue instead of EH might be a
better idea.
EH looks like an overkill here.

Maksim.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] Deferred disk spinup during system resume

[Tejun Heo]

Hey, Jeff.

On Wed, Jan 12, 2011 at 01:35:07PM -0500, Jeff Garzik wrote:
> The previous patch breaks READ VERIFY translation for all cases
> -except- this one.

Ooh, right; then, can we just fix that?

> The bottom line is that this patch simply wants to trigger an ATA
> command, and return immediately, discarding the command results.
> I'm not even sure a "run this command in background, and discard
> results" facility requires the EH.

It doens't necessarily require EH but the dependency is rather
delicate because we still ride on SCSI EH.  For example, we don't have
any provision for running qc's without the associated scmd.  If the
command fails, libata EH rides on SCSI EH and the interaction relies
on the association between qc's and scmd's.  We may be able to add
privison for this case but we already have working mechanism in EH for
internal commands so it's just easiser that way.

Well, actually, the _really_ right(eous) thing to do here would be
just not doing it as it's an ugly hack no matter which way we spin it,
but this has a quite significant impact on resume from STR latency
so...

Thanks.

-- 
tejun

Re: [RFC/PATCH] Deferred disk spinup during system resume

[Tejun Heo]

Hello,

On Wed, Jan 12, 2011 at 12:01:56PM -0800, Maksim Rayskiy wrote:
> This is why I was asking if using a workqueue instead of EH might be
> a better idea.  EH looks like an overkill here.

We're not in need of execution context here.  libata machinary doesn't
need context to run commands.  The tricky part is the association
between scsi commands and libata qc's.  The reason why it's easy to do
from EH is because it already has access to internal command
execution.  It has nothing to do with the execution context and wq
won't change anything.

Thanks.

-- 
tejun

Re: [RFC/PATCH] Deferred disk spinup during system resume

[Jeff Garzik]

On 01/13/2011 10:37 AM, Tejun Heo wrote:
> Hey, Jeff.
>
> On Wed, Jan 12, 2011 at 01:35:07PM -0500, Jeff Garzik wrote:
>> The previous patch breaks READ VERIFY translation for all cases
>> -except- this one.
>
> Ooh, right; then, can we just fix that?
>
>> The bottom line is that this patch simply wants to trigger an ATA
>> command, and return immediately, discarding the command results.
>> I'm not even sure a "run this command in background, and discard
>> results" facility requires the EH.
>
> It doens't necessarily require EH but the dependency is rather
> delicate because we still ride on SCSI EH.  For example, we don't have
> any provision for running qc's without the associated scmd.  If the
> command fails, libata EH rides on SCSI EH and the interaction relies
> on the association between qc's and scmd's.  We may be able to add
> privison for this case but we already have working mechanism in EH for
> internal commands so it's just easiser that way.

In this case we -do- have an associated scmd, otherwise he would not be 
able to trigger the READ VERIFY translation code.

So it seems a question of (a) who generates READ VERIFY, and (b) can we 
have them issue the command without waiting for the results?

Which is something I would have thought SG_IO/bsg already provided...

It remains unclear to me why EH must be updated, simply to execute a 
non-data ATA command without waiting.

	Jeff

Re: [RFC/PATCH] Deferred disk spinup during system resume

[Tejun Heo]

On Thu, Jan 13, 2011 at 12:20:00PM -0500, Jeff Garzik wrote:
> In this case we -do- have an associated scmd, otherwise he would not
> be able to trigger the READ VERIFY translation code.

Yeah, we have on issue but we don't if we decide to execute it
asynchronously.  It would be best if SCSI can handle the
asynchronousity but I can't see how at this point.

> So it seems a question of (a) who generates READ VERIFY, and (b) can
> we have them issue the command without waiting for the results?
> 
> Which is something I would have thought SG_IO/bsg already provided...
> 
> It remains unclear to me why EH must be updated, simply to execute a
> non-data ATA command without waiting.

It's just there to use the internal qc.  Nothing else.  If
asynchronousity can be handled, it would be the best.

Thanks.

-- 
tejun

Re: [RFC/PATCH] Deferred disk spinup during system resume

[Jeff Garzik]

On 01/12/2011 03:01 PM, Maksim Rayskiy wrote:
>>
>> The bottom line is that this patch simply wants to trigger an ATA command,
>> and return immediately, discarding the command results.  I'm not even sure a
>> "run this command in background, and discard results" facility requires the
>> EH.
>>
>
> This is why I was asking if using a workqueue instead of EH might be a
> better idea.
> EH looks like an overkill here.

A kernel modification might not even be needed.

Have you tried simply issuing READ VERIFY via bsg, and not caring if it 
completes?  bsg should be able to handle an app submitting a command, 
but never checking the 'done' list, right?  A simple shell app could execute

	write(bsg_fd, ... SCSI READ VERIFY command ...)
	exit(0)

to avoid waiting for READ VERIFY command completion, I would hope.

	Jeff

Re: [RFC/PATCH] Deferred disk spinup during system resume

[Maksim Rayskiy]

> A kernel modification might not even be needed.
>
> Have you tried simply issuing READ VERIFY via bsg, and not caring if it
> completes?  bsg should be able to handle an app submitting a command, but
> never checking the 'done' list, right?  A simple shell app could execute
>
>        write(bsg_fd, ... SCSI READ VERIFY command ...)
>        exit(0)
>
> to avoid waiting for READ VERIFY command completion, I would hope.
>
>        Jeff
>

My question was how to speed up system resume when he the spinup
request is coming from sd_resume(). For shell method to work I would
have to ignore this request entirely and do READ VERIFY when system is
fully restored.
Can it be done without kernel modification? Tejun mentioned using
manage_start_stop flag but had some reservations against it.

Maksim.

Re: [RFC/PATCH] Deferred disk spinup during system resume

[Jeff Garzik]

On 01/19/2011 03:29 PM, Maksim Rayskiy wrote:
>> A kernel modification might not even be needed.
>>
>> Have you tried simply issuing READ VERIFY via bsg, and not caring if it
>> completes?  bsg should be able to handle an app submitting a command, but
>> never checking the 'done' list, right?  A simple shell app could execute
>>
>>         write(bsg_fd, ... SCSI READ VERIFY command ...)
>>         exit(0)
>>
>> to avoid waiting for READ VERIFY command completion, I would hope.
>>
>>         Jeff
>>
>
> My question was how to speed up system resume when he the spinup
> request is coming from sd_resume(). For shell method to work I would
> have to ignore this request entirely and do READ VERIFY when system is
> fully restored.
> Can it be done without kernel modification? Tejun mentioned using
> manage_start_stop flag but had some reservations against it.

Oh, if we're talking about sd_resume(), SCSI definitely has an internal 
mechanism to fire off a request, without blocking and waiting for a 
response.  It does sound like a kernel modification, but it should be 
straightforward via existing APIs.

	Jeff