Patchwork [v2,1/4] scsi: introduce sync_before_stop flag

login
register
mail settings
Submitter Aaron Lu
Date Sept. 18, 2012, 7 a.m.
Message ID <1347951631-1592-2-git-send-email-aaron.lu@intel.com>
Download mbox | patch
Permalink /patch/184629/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Aaron Lu - Sept. 18, 2012, 7 a.m.
When scsi device received stop command, it will take care of its
internal cache before enters stopped power condition. This command is
translated to standby immediate in libata-scsi, but standby doesn't
imply flush cache for ATA device, so to issue stop command to ATA
device, an additional flush cache has to be issued.

Introduce this flag so that when we are to stop the ATA disk in scsi
disk driver, also flush its internal cache.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 include/scsi/scsi_device.h | 1 +
 1 file changed, 1 insertion(+)
Oliver Neukum - Sept. 18, 2012, 7:30 a.m.
On Tuesday 18 September 2012 15:00:28 Aaron Lu wrote:
> When scsi device received stop command, it will take care of its
> internal cache before enters stopped power condition. This command is
> translated to standby immediate in libata-scsi, but standby doesn't
> imply flush cache for ATA device, so to issue stop command to ATA
> device, an additional flush cache has to be issued.

Why not just set WCE?

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu - Sept. 18, 2012, 7:47 a.m.
On Tue, Sep 18, 2012 at 09:30:11AM +0200, Oliver Neukum wrote:
> On Tuesday 18 September 2012 15:00:28 Aaron Lu wrote:
> > When scsi device received stop command, it will take care of its
> > internal cache before enters stopped power condition. This command is
> > translated to standby immediate in libata-scsi, but standby doesn't
> > imply flush cache for ATA device, so to issue stop command to ATA
> > device, an additional flush cache has to be issued.
> 
> Why not just set WCE?

This flag is used for devices whose WCE bit is set.

This flag means, when we are to issue a scsi stop command, we need to
issue an additional sync cache command first.

For scsi device, per the spec, there is no such need as the device will
take care of its internal cache when going to stop power condition.

But for ata device, the stop command is translated to standby immediate,
and we have to flush the internal cache before enter standby.

-Aaron
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley - Sept. 18, 2012, 7:56 a.m.
On Tue, 2012-09-18 at 15:00 +0800, Aaron Lu wrote:
> When scsi device received stop command, it will take care of its
> internal cache before enters stopped power condition. This command is
> translated to standby immediate in libata-scsi, but standby doesn't
> imply flush cache for ATA device, so to issue stop command to ATA
> device, an additional flush cache has to be issued.
> 
> Introduce this flag so that when we are to stop the ATA disk in scsi
> disk driver, also flush its internal cache.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  include/scsi/scsi_device.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 4712aa1..26c3621 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -160,6 +160,7 @@ struct scsi_device {
>  	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
>  	unsigned powered_off:1;	/* Device is powered off */
>  	unsigned may_power_off:1; /* Power off is allowed by user */
> +	unsigned sync_before_stop:1; /* Sync cache before stop */

Why do you need this?

Surely it's all conditioned on the WCE flag.  If WCE isn't set, the
cache is write through (or uncached) and there's no need for a sync
before power down.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu - Sept. 18, 2012, 8:09 a.m.
On Tue, Sep 18, 2012 at 08:56:55AM +0100, James Bottomley wrote:
> On Tue, 2012-09-18 at 15:00 +0800, Aaron Lu wrote:
> > When scsi device received stop command, it will take care of its
> > internal cache before enters stopped power condition. This command is
> > translated to standby immediate in libata-scsi, but standby doesn't
> > imply flush cache for ATA device, so to issue stop command to ATA
> > device, an additional flush cache has to be issued.
> > 
> > Introduce this flag so that when we are to stop the ATA disk in scsi
> > disk driver, also flush its internal cache.
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > ---
> >  include/scsi/scsi_device.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > index 4712aa1..26c3621 100644
> > --- a/include/scsi/scsi_device.h
> > +++ b/include/scsi/scsi_device.h
> > @@ -160,6 +160,7 @@ struct scsi_device {
> >  	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
> >  	unsigned powered_off:1;	/* Device is powered off */
> >  	unsigned may_power_off:1; /* Power off is allowed by user */
> > +	unsigned sync_before_stop:1; /* Sync cache before stop */
> 
> Why do you need this?
> 
> Surely it's all conditioned on the WCE flag.  If WCE isn't set, the
> cache is write through (or uncached) and there's no need for a sync
> before power down.

The set of this flag doesn't mean we will sync cache for sure.

It's only meaningful when WCE is set, and in that case, it means when we
are to send a stop command to the device, do we need to send an
additional flush cache command first?

In sd_suspend, the cache will be synchronized when:
1 For devices do not support start_stop, always;
2 For devices support start_stop, if it is standard scsi device, never;
  and if it is an ata device(reflected by this newly introduced flag),
  always.

The reason for this is, the scsi's stop command = flush_cache + standby
for ata, so I have to introduce this flag to explicitely do a cache
flush.

Makes sense?

-Aaron
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley - Sept. 18, 2012, 8:20 a.m.
On Tue, 2012-09-18 at 16:09 +0800, Aaron Lu wrote:
> On Tue, Sep 18, 2012 at 08:56:55AM +0100, James Bottomley wrote:
> > On Tue, 2012-09-18 at 15:00 +0800, Aaron Lu wrote:
> > > When scsi device received stop command, it will take care of its
> > > internal cache before enters stopped power condition. This command is
> > > translated to standby immediate in libata-scsi, but standby doesn't
> > > imply flush cache for ATA device, so to issue stop command to ATA
> > > device, an additional flush cache has to be issued.
> > > 
> > > Introduce this flag so that when we are to stop the ATA disk in scsi
> > > disk driver, also flush its internal cache.
> > > 
> > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > > ---
> > >  include/scsi/scsi_device.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > > index 4712aa1..26c3621 100644
> > > --- a/include/scsi/scsi_device.h
> > > +++ b/include/scsi/scsi_device.h
> > > @@ -160,6 +160,7 @@ struct scsi_device {
> > >  	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
> > >  	unsigned powered_off:1;	/* Device is powered off */
> > >  	unsigned may_power_off:1; /* Power off is allowed by user */
> > > +	unsigned sync_before_stop:1; /* Sync cache before stop */
> > 
> > Why do you need this?
> > 
> > Surely it's all conditioned on the WCE flag.  If WCE isn't set, the
> > cache is write through (or uncached) and there's no need for a sync
> > before power down.
> 
> The set of this flag doesn't mean we will sync cache for sure.
> 
> It's only meaningful when WCE is set, and in that case, it means when we
> are to send a stop command to the device, do we need to send an
> additional flush cache command first?

Then surely it indicates support for ACPI power down and it's wrongly
named?

> In sd_suspend, the cache will be synchronized when:
> 1 For devices do not support start_stop, always;
> 2 For devices support start_stop, if it is standard scsi device, never;
>   and if it is an ata device(reflected by this newly introduced flag),
>   always.

This doesn't look right to me.  I think it's probably just a layering
violation.  For sd, we need to use the standard SCSI commands.  That
means we treat power states as the SCSI standard says.  The fact that
ATA devices may be required to translate START_STOP_UNIT with STANDBY as
a flush followed by one of the ATA standby commands.  This is very
important: if we construct a libata SATL that doesn't conform to the
standards, things will eventually explode when we try to interact with
devices with their own internal SATL (like the LSI card, or various USB
devices) because eventually we'll make one unexpected interaction too
many.

> The reason for this is, the scsi's stop command = flush_cache +
> standby
> for ata, so I have to introduce this flag to explicitely do a cache
> flush.
> 
> Makes sense?

Really, no: your flag equates to "this is an ATA device, treat it
differently" that's knowledge that's not supposed to be in sd unless
absolutely necessary.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu - Sept. 18, 2012, 8:31 a.m.
On Tue, Sep 18, 2012 at 09:20:20AM +0100, James Bottomley wrote:
> On Tue, 2012-09-18 at 16:09 +0800, Aaron Lu wrote:
> > On Tue, Sep 18, 2012 at 08:56:55AM +0100, James Bottomley wrote:
> > > On Tue, 2012-09-18 at 15:00 +0800, Aaron Lu wrote:
> > > > When scsi device received stop command, it will take care of its
> > > > internal cache before enters stopped power condition. This command is
> > > > translated to standby immediate in libata-scsi, but standby doesn't
> > > > imply flush cache for ATA device, so to issue stop command to ATA
> > > > device, an additional flush cache has to be issued.
> > > > 
> > > > Introduce this flag so that when we are to stop the ATA disk in scsi
> > > > disk driver, also flush its internal cache.
> > > > 
> > > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > > > ---
> > > >  include/scsi/scsi_device.h | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > > > index 4712aa1..26c3621 100644
> > > > --- a/include/scsi/scsi_device.h
> > > > +++ b/include/scsi/scsi_device.h
> > > > @@ -160,6 +160,7 @@ struct scsi_device {
> > > >  	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
> > > >  	unsigned powered_off:1;	/* Device is powered off */
> > > >  	unsigned may_power_off:1; /* Power off is allowed by user */
> > > > +	unsigned sync_before_stop:1; /* Sync cache before stop */
> > > 
> > > Why do you need this?
> > > 
> > > Surely it's all conditioned on the WCE flag.  If WCE isn't set, the
> > > cache is write through (or uncached) and there's no need for a sync
> > > before power down.
> > 
> > The set of this flag doesn't mean we will sync cache for sure.
> > 
> > It's only meaningful when WCE is set, and in that case, it means when we
> > are to send a stop command to the device, do we need to send an
> > additional flush cache command first?
> 
> Then surely it indicates support for ACPI power down and it's wrongly
> named?

It's generic ATA requirement that before standby, cache has to be
flushed and unlike scsi, standby does not imply cache flush.

> 
> > In sd_suspend, the cache will be synchronized when:
> > 1 For devices do not support start_stop, always;
> > 2 For devices support start_stop, if it is standard scsi device, never;
> >   and if it is an ata device(reflected by this newly introduced flag),
> >   always.
> 
> This doesn't look right to me.  I think it's probably just a layering
> violation.  For sd, we need to use the standard SCSI commands.  That
> means we treat power states as the SCSI standard says.  The fact that
> ATA devices may be required to translate START_STOP_UNIT with STANDBY as
> a flush followed by one of the ATA standby commands.  This is very
> important: if we construct a libata SATL that doesn't conform to the
> standards, things will eventually explode when we try to interact with
> devices with their own internal SATL (like the LSI card, or various USB
> devices) because eventually we'll make one unexpected interaction too
> many.

I agree that it is better handled in libata's SALT, I tried to do this
but didn't find a good way so I introduced this flag. The SALT is 1-1
mapping, I'm not sure how to handle this 1-2 mapping.

I'll check this again to see how to do it there.

Thanks for your comments.

-Aaron
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - Sept. 24, 2012, 10:13 p.m.
On 09/18/2012 04:31 AM, Aaron Lu wrote:
> I agree that it is better handled in libata's SALT, I tried to do this
> but didn't find a good way so I introduced this flag. The SALT is 1-1
> mapping, I'm not sure how to handle this 1-2 mapping.

That is the crux of the matter:  it is rather difficult within the 
current code, if there is not a 1:1 correspondence between SCSI command 
and ATA command.

The solution that works within libata-scsi.c involves creating two 
ata_queued_cmd's, and managing the issuance and completion yourself.

But really -- the most practical route seems to be simply to set WCE, to 
trigger the desired behavior.

	Jeff



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

Patch

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 4712aa1..26c3621 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -160,6 +160,7 @@  struct scsi_device {
 	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
 	unsigned powered_off:1;	/* Device is powered off */
 	unsigned may_power_off:1; /* Power off is allowed by user */
+	unsigned sync_before_stop:1; /* Sync cache before stop */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */