[RFC] Deferred disk spinup during system resume

[RFC] Deferred disk spinup during system resume

[maksim.rayskiy]

Sorry, put some irrelevant stuff in the previous patch

---
 drivers/ata/libata-core.c |    8 ++++++++
 drivers/ata/libata-eh.c   |   45 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-scsi.c |   24 +++++-------------------
 include/linux/libata.h    |    4 +++-
 4 files changed, 61 insertions(+), 20 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 7f77c67..0a9d400 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4978,6 +4978,14 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 	struct ata_link *link = qc->dev->link;
 	u8 prot = qc->tf.protocol;
 
+	if (unlikely(link->eh_info.dev_action[qc->dev->devno] &
+			ATA_EH_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..3823bc6 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3252,6 +3252,48 @@ 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_eh_about_to_do(link, dev, ATA_EH_VERIFY);
+
+	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;
+	}
+
+	/* don't retry VERIFY */
+	ata_eh_done(link, dev, ATA_EH_VERIFY);
+
+	return rc;
+}
+
 /**
  *	ata_eh_set_lpm - configure SATA interface power management
  *	@link: link to configure power management
@@ -3738,6 +3780,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..aadcd0d 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1276,6 +1276,7 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth,
  */
 static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 {
+	struct ata_device *dev = qc->dev;
 	struct scsi_cmnd *scmd = qc->scsicmd;
 	struct ata_taskfile *tf = &qc->tf;
 	const u8 *cdb = scmd->cmnd;
@@ -1293,25 +1294,10 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 	if (((cdb[4] >> 4) & 0xf) != 0)
 		goto invalid_fld;       /* power conditions not supported */
 
-	if (cdb[4] & 0x1) {
-		tf->nsect = 1;	/* 1 sector, lba=0 */
-
-		if (qc->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 */
-	} else {
+	if (cdb[4] & 0x1)
+		/* spinup is asynchronous */
+		dev->link->eh_info.dev_action[dev->devno] |= ATA_EH_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..5d91fd0 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -326,9 +326,11 @@ enum {
 	ATA_EH_HARDRESET	= (1 << 2), /* meaningful only in ->prereset */
 	ATA_EH_RESET		= ATA_EH_SOFTRESET | ATA_EH_HARDRESET,
 	ATA_EH_ENABLE_LINK	= (1 << 3),
+	ATA_EH_VERIFY		= (1 << 4), /* used to force spin up */
 	ATA_EH_PARK		= (1 << 5), /* unload heads and stop I/O */
 
-	ATA_EH_PERDEV_MASK	= ATA_EH_REVALIDATE | ATA_EH_PARK,
+	ATA_EH_PERDEV_MASK	= ATA_EH_REVALIDATE | ATA_EH_PARK |
+				  ATA_EH_VERIFY,
 	ATA_EH_ALL_ACTIONS	= ATA_EH_REVALIDATE | ATA_EH_RESET |
 				  ATA_EH_ENABLE_LINK,
 
-- 
1.7.0.4

Re: [RFC] Deferred disk spinup during system resume

[Tejun Heo]

Hello,

On Thu, Jan 06, 2011 at 05:27:06PM -0800, maksim.rayskiy@gmail.com wrote:
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 7f77c67..0a9d400 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4978,6 +4978,14 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>  	struct ata_link *link = qc->dev->link;
>  	u8 prot = qc->tf.protocol;
>  
> +	if (unlikely(link->eh_info.dev_action[qc->dev->devno] &
> +			ATA_EH_VERIFY)) {
> +		ata_port_schedule_eh(ap);
> +		qc->scsidone(qc->scsicmd);
> +		ata_qc_free(qc);
> +		return;

Hmm... this is weird.  You'll probably want to define a qc flag to
indicate that the command is for verify and then schedule EH from
ata_qc_issue().  That said, I'm not quite sure whether that's any
better than scheduling EH from libata-scsi.  It's not like the
boundary is clearly defined.  We already play with EH for other stuff
from libata-scsi.  Jeff, do you think going through QCFLAG is
important?

Thanks.

-- 
tejun

Re: [RFC] Deferred disk spinup during system resume

[Jeff Garzik]

On 01/07/2011 02:06 PM, Tejun Heo wrote:
> Hello,
>
> On Thu, Jan 06, 2011 at 05:27:06PM -0800, maksim.rayskiy@gmail.com wrote:
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 7f77c67..0a9d400 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -4978,6 +4978,14 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>>   	struct ata_link *link = qc->dev->link;
>>   	u8 prot = qc->tf.protocol;
>>
>> +	if (unlikely(link->eh_info.dev_action[qc->dev->devno]&
>> +			ATA_EH_VERIFY)) {
>> +		ata_port_schedule_eh(ap);
>> +		qc->scsidone(qc->scsicmd);
>> +		ata_qc_free(qc);
>> +		return;
>
> Hmm... this is weird.  You'll probably want to define a qc flag to
> indicate that the command is for verify and then schedule EH from
> ata_qc_issue().  That said, I'm not quite sure whether that's any
> better than scheduling EH from libata-scsi.  It's not like the
> boundary is clearly defined.  We already play with EH for other stuff
> from libata-scsi.  Jeff, do you think going through QCFLAG is
> important?

The boundary is clearly defined.  It's just not a file boundary :) 
libata-scsi contains EH callouts, but all of those are (a) sysfs 
attribute handling, (b) for callers located outside libata-scsi, or (c) 
the SCSI host scan.

The command translation and execution hot path is notably not in that list.

It is a clear laying violation to call out from the middle of a command 
translation, to directly execute some EH actions.  Remember, this is the 
stage at which we are QUEUEING commands.  There may be other commands in 
the queue, or currently executing.  If someone sends a READ VERIFY, we 
do not want to IMMEDIATELY schedule EH, we want to execute an EH action 
when the READ VERIFY command is ready to be executed.

It is quite incorrect to assume that READ VERIFY will only be sent to 
libata for execution during system resume.  The removal of READ VERIFY 
translation from ata_scsi_start_stop_xlat() is clearly wrong.

	Jeff

Re: [RFC] Deferred disk spinup during system resume

[Maksim Rayskiy]

> The taskfile you are to pass to the hardware is created in
> atapi_start_stop_xlat(), so you should not create a new taskfile in
> ata_eh_maybe_verify()

In order for resume to proceed without waiting for START to finish I
need to complete and release READ VERIFY qc immediately. I can
schedule EH action but it will be using a new qc and a new taskfile.

> It is a clear laying violation to call out from the middle of a command
> translation, to directly execute some EH actions.  Remember, this is the
> stage at which we are QUEUEING commands.  There may be other commands in the
> queue, or currently executing.  If someone sends a READ VERIFY, we do not
> want to IMMEDIATELY schedule EH, we want to execute an EH action when the
> READ VERIFY command is ready to be executed.
>
> It is quite incorrect to assume that READ VERIFY will only be sent to libata
> for execution during system resume.  The removal of READ VERIFY translation
> from ata_scsi_start_stop_xlat() is clearly wrong.
>

There is also ata_scsi_verify_xlat() that translates READ VERIFY.
How can I tell in ata_scsi_start_stop_xlat() if this particular
request is coming from system resume? Doesn't that mean I will have
defer all START requests? Or I need a separate command instead of
START_STOP to indicate that this request can be done asynchronously?

Thanks,
Maksim.

[RFC] Deferred disk spinup during system resume

[maksim.rayskiy]

Tejun,

Thanks and sorry for late response, I was testing the patch on several platforms, 
including the ones with sata-based rootfs. The patch seems to be working fine for me. 
I only had to include ATA_EH_VERIFY into ATA_EH_PERDEV_MASK mask to avoid warning 
from ata_eh_clear_action().

Are you going to submit the patch to the mainline?

Maksim.

---
 drivers/ata/libata-eh.c   |   45 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-scsi.c |   21 ++++-----------------
 include/linux/libata.h    |    4 +++-
 3 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 5e59050..3823bc6 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3252,6 +3252,48 @@ 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_eh_about_to_do(link, dev, ATA_EH_VERIFY);
+
+	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;
+	}
+
+	/* don't retry VERIFY */
+	ata_eh_done(link, dev, ATA_EH_VERIFY);
+
+	return rc;
+}
+
 /**
  *	ata_eh_set_lpm - configure SATA interface power management
  *	@link: link to configure power management
@@ -3738,6 +3780,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..ae8d9ca 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1276,6 +1276,7 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth,
  */
 static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 {
+	struct ata_device *dev = qc->dev;
 	struct scsi_cmnd *scmd = qc->scsicmd;
 	struct ata_taskfile *tf = &qc->tf;
 	const u8 *cdb = scmd->cmnd;
@@ -1294,23 +1295,9 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 		goto invalid_fld;       /* power conditions not supported */
 
 	if (cdb[4] & 0x1) {
-		tf->nsect = 1;	/* 1 sector, lba=0 */
-
-		if (qc->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 */
+		dev->link->eh_info.dev_action[dev->devno] |= ATA_EH_VERIFY;
+		ata_port_schedule_eh(dev->link->ap);
+		goto skip;
 	} 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..5d91fd0 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -326,9 +326,11 @@ enum {
 	ATA_EH_HARDRESET	= (1 << 2), /* meaningful only in ->prereset */
 	ATA_EH_RESET		= ATA_EH_SOFTRESET | ATA_EH_HARDRESET,
 	ATA_EH_ENABLE_LINK	= (1 << 3),
+	ATA_EH_VERIFY		= (1 << 4), /* used to force spin up */
 	ATA_EH_PARK		= (1 << 5), /* unload heads and stop I/O */
 
-	ATA_EH_PERDEV_MASK	= ATA_EH_REVALIDATE | ATA_EH_PARK,
+	ATA_EH_PERDEV_MASK	= ATA_EH_REVALIDATE | ATA_EH_PARK |
+				  ATA_EH_VERIFY,
 	ATA_EH_ALL_ACTIONS	= ATA_EH_REVALIDATE | ATA_EH_RESET |
 				  ATA_EH_ENABLE_LINK,
 
-- 
1.7.0.4

Re: [RFC] Deferred disk spinup during system resume

[Jeff Garzik]

(linux-ide CC added)

On Thu, Jan 6, 2011 at 3:20 PM,  <maksim.rayskiy@gmail.com> wrote:
> index 66aa4be..ae8d9ca 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1276,6 +1276,7 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth,
>  */
>  static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
>  {
> +       struct ata_device *dev = qc->dev;
>        struct scsi_cmnd *scmd = qc->scsicmd;
>        struct ata_taskfile *tf = &qc->tf;
>        const u8 *cdb = scmd->cmnd;
> @@ -1294,23 +1295,9 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
>                goto invalid_fld;       /* power conditions not supported */
>
>        if (cdb[4] & 0x1) {
> -               tf->nsect = 1;  /* 1 sector, lba=0 */
> -
> -               if (qc->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 */
> +               dev->link->eh_info.dev_action[dev->devno] |= ATA_EH_VERIFY;
> +               ata_port_schedule_eh(dev->link->ap);
> +               goto skip;
>        } else {
>                /* Some odd clown BIOSen issue spindown on power off (ACPI S4
>                 * or S5) causing some drives to spin up and down again.

This really breaks the model where we are simply performing a
translation -- where queueing and execution of the translated qc is
kept separate from the SCSI->ATA translation itself.

The above code should be left as-is, except perhaps it could set a
'verify' flag in the qc, which would then trigger the EH behavior you
seek.

     Jeff

[RFC] Deferred disk spinup during system resume

[Maksim Rayskiy]

Hello,

When resuming from system suspend, scsi disks are being spun up which
takes quite a lot of time (5+ seconds in my case). The spinup is done
synchronously, so this time adds up to overall system resume time.
Ours is an embedded platform and we are using flash-based rootfs, so
there is no immediate need in harddrive after resume. What is much
more important for us is to minimize time-to-full-power. To speed up
resume, we would like to have an option to defer the spinup or run it
in parallel with system resume. I could not find any existing
mechanism to do the trick, but I might have missed something.

Can anybody comment on this?

Thanks,
Maksim.