Re: libata: set queue SSD flag for SSD devices

Re: libata: set queue SSD flag for SSD devices

[Alan Cox]

> +	ATA_ID_ROT_SPEED	= 217,
>  	ATA_ID_PIO4		= (1 << 1),
>  
>  	ATA_ID_SERNO_LEN	= 20,
> @@ -691,6 +692,11 @@ static inline int ata_id_is_cfa(const u16 *id)
>  	return 0;
>  }
>  
> +static inline int ata_id_is_ssd(const u16 *id)
> +{
> +	return id[ATA_ID_ROT_SPEED] == 0x01;
> +}

This patch is buggy.

Word 217 only has a defined meaning in late revision ATA standards so a
version check is needed. Please copy ATA patch copies to linux-ide.

Alan

Re: libata: set queue SSD flag for SSD devices

[Jens Axboe]

On Fri, Oct 10 2008, Alan Cox wrote:
> > +	ATA_ID_ROT_SPEED	= 217,
> >  	ATA_ID_PIO4		= (1 << 1),
> >  
> >  	ATA_ID_SERNO_LEN	= 20,
> > @@ -691,6 +692,11 @@ static inline int ata_id_is_cfa(const u16 *id)
> >  	return 0;
> >  }
> >  
> > +static inline int ata_id_is_ssd(const u16 *id)
> > +{
> > +	return id[ATA_ID_ROT_SPEED] == 0x01;
> > +}
> 
> This patch is buggy.
> 
> Word 217 only has a defined meaning in late revision ATA standards so a
> version check is needed. Please copy ATA patch copies to linux-ide.

Good point, I'll be sure to submit an updated patch. I did mean to copy
linux-ide, the intention was actually to pass this libata enable patch
through Jeff after testing, but it slipped through.

In this specific patch, it'll do no harm at least since I very much
doubt we'll see a false positive. And even if, the consequences wont be
dire. But it does want the version check, of course.

-- 
Jens Axboe

Re: libata: set queue SSD flag for SSD devices

[Arjan van de Ven]

On Fri, 10 Oct 2008 22:05:28 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> In this specific patch, it'll do no harm at least since I very much
> doubt we'll see a false positive. And even if, the consequences wont
> be dire. But it does want the version check, of course.
> 

also, is "is an ssd" the right question or is "doesn't have seek
latency" the right one?
(difference is.. well EMC boxes with lots of ram etc)


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: libata: set queue SSD flag for SSD devices

[Jens Axboe]

On Fri, Oct 10 2008, Arjan van de Ven wrote:
> On Fri, 10 Oct 2008 22:05:28 +0200
> Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> > In this specific patch, it'll do no harm at least since I very much
> > doubt we'll see a false positive. And even if, the consequences wont
> > be dire. But it does want the version check, of course.
> > 
> 
> also, is "is an ssd" the right question or is "doesn't have seek
> latency" the right one?
> (difference is.. well EMC boxes with lots of ram etc)

The block layer uses the 'non rotational' nomenclature for this, but
that's not perfect either. And neither is 'no seek', a single
"parameter" is not enough to describe the device. But it's good enough I
think, it'll be expanded later with a (bit) fuller profile.

-- 
Jens Axboe

Re: libata: set queue SSD flag for SSD devices

[James Bottomley]

On Fri, 2008-10-10 at 17:55 -0700, Arjan van de Ven wrote:
> On Fri, 10 Oct 2008 22:05:28 +0200
> Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> > In this specific patch, it'll do no harm at least since I very much
> > doubt we'll see a false positive. And even if, the consequences wont
> > be dire. But it does want the version check, of course.
> > 
> 
> also, is "is an ssd" the right question or is "doesn't have seek
> latency" the right one?
> (difference is.. well EMC boxes with lots of ram etc)

For libata, the is ssd is the right test.

For things like the symmetrix we probably need to look at the caching
mode page and treat it like ssd if the cache is say > 1GB or whatever
heuristic is needed.

James

Re: libata: set queue SSD flag for SSD devices

[Jens Axboe]

On Sat, Oct 11 2008, James Bottomley wrote:
> On Fri, 2008-10-10 at 17:55 -0700, Arjan van de Ven wrote:
> > On Fri, 10 Oct 2008 22:05:28 +0200
> > Jens Axboe <jens.axboe@oracle.com> wrote:
> > > 
> > > In this specific patch, it'll do no harm at least since I very much
> > > doubt we'll see a false positive. And even if, the consequences wont
> > > be dire. But it does want the version check, of course.
> > > 
> > 
> > also, is "is an ssd" the right question or is "doesn't have seek
> > latency" the right one?
> > (difference is.. well EMC boxes with lots of ram etc)
> 
> For libata, the is ssd is the right test.
> 
> For things like the symmetrix we probably need to look at the caching
> mode page and treat it like ssd if the cache is say > 1GB or whatever
> heuristic is needed.

Still wont quite work, you're still going to have occasional seek
latencies no matter how large the cache is (unless it's == device size
:-)

So we need something a bit more involved, but not too complex. A fine
line...

-- 
Jens Axboe

Re: libata: set queue SSD flag for SSD devices

[James Bottomley]

On Sat, 2008-10-11 at 16:06 +0200, Jens Axboe wrote:
> On Sat, Oct 11 2008, James Bottomley wrote:
> > On Fri, 2008-10-10 at 17:55 -0700, Arjan van de Ven wrote:
> > > On Fri, 10 Oct 2008 22:05:28 +0200
> > > Jens Axboe <jens.axboe@oracle.com> wrote:
> > > > 
> > > > In this specific patch, it'll do no harm at least since I very much
> > > > doubt we'll see a false positive. And even if, the consequences wont
> > > > be dire. But it does want the version check, of course.
> > > > 
> > > 
> > > also, is "is an ssd" the right question or is "doesn't have seek
> > > latency" the right one?
> > > (difference is.. well EMC boxes with lots of ram etc)
> > 
> > For libata, the is ssd is the right test.
> > 
> > For things like the symmetrix we probably need to look at the caching
> > mode page and treat it like ssd if the cache is say > 1GB or whatever
> > heuristic is needed.
> 
> Still wont quite work, you're still going to have occasional seek
> latencies no matter how large the cache is (unless it's == device size
> :-)

True (just not very often) ... but statistically, the big symmetrixes
get better performance with the noop scheduler (which is treating them
like a SSD).  The occasional time they hiccough and seek tends to be
statistically in the noise.

As long as we do this stuff in userspace anyway, nothing needs to change
in the SCSI subsystem.

> So we need something a bit more involved, but not too complex. A fine
> line...

It's a policy ... just let userspace do it so the user can tune it.
That's what EMC does now (except I think they key of inquiry strings
rather than cache size).

James

Re: libata: set queue SSD flag for SSD devices

[Arjan van de Ven]

On Sat, 11 Oct 2008 17:44:13 +0200
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> 
> > So we need something a bit more involved, but not too complex. A
> > fine line...
> 
> It's a policy ... just let userspace do it so the user can tune it.
> That's what EMC does now (except I think they key of inquiry strings
> rather than cache size).


while the chosen elevator obviously is policy, the kernel really should
pick a sensible default based on what it knows.
Lets put it this way: if userland needs to do a tuning to the kernel
based on data only provided by the kernel, and will always do it the
same way, we should have made that choice the default policy in the
kernel in the first place.

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: libata: set queue SSD flag for SSD devices

[James Bottomley]

On Sat, 2008-10-11 at 09:04 -0700, Arjan van de Ven wrote:
> On Sat, 11 Oct 2008 17:44:13 +0200
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > 
> > > So we need something a bit more involved, but not too complex. A
> > > fine line...
> > 
> > It's a policy ... just let userspace do it so the user can tune it.
> > That's what EMC does now (except I think they key of inquiry strings
> > rather than cache size).
> 
> 
> while the chosen elevator obviously is policy, the kernel really should
> pick a sensible default based on what it knows.
> Lets put it this way: if userland needs to do a tuning to the kernel
> based on data only provided by the kernel, and will always do it the
> same way, we should have made that choice the default policy in the
> kernel in the first place.

Well, this is a bit of a nasty layering problem.  We certainly don't
want the Block layer to know how to poke at SATA, SCSI and other
esoteric media to see what elevator should be the default, so we'd have
to craft a new block API that the lower subsystems would implement for
this.  I'm really not sure it's worth the trouble when the boot system
can do it simply from userspace, but I'll defer to Jens.

James

Re: libata: set queue SSD flag for SSD devices

[Arjan van de Ven]

On Sat, 11 Oct 2008 18:38:44 +0200
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Sat, 2008-10-11 at 09:04 -0700, Arjan van de Ven wrote:
> > On Sat, 11 Oct 2008 17:44:13 +0200
> > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > 
> > > 
> > > > So we need something a bit more involved, but not too complex. A
> > > > fine line...
> > > 
> > > It's a policy ... just let userspace do it so the user can tune
> > > it. That's what EMC does now (except I think they key of inquiry
> > > strings rather than cache size).
> > 
> > 
> > while the chosen elevator obviously is policy, the kernel really
> > should pick a sensible default based on what it knows.
> > Lets put it this way: if userland needs to do a tuning to the kernel
> > based on data only provided by the kernel, and will always do it the
> > same way, we should have made that choice the default policy in the
> > kernel in the first place.
> 
> Well, this is a bit of a nasty layering problem.  We certainly don't
> want the Block layer to know how to poke at SATA, SCSI and other
> esoteric media to see what elevator should be the default, so we'd
> have to craft a new block API that the lower subsystems would
> implement for this.  I'm really not sure it's worth the trouble when
> the boot system can do it simply from userspace, but I'll defer to
> Jens.
> 

these devices already give the elevator layer information about the
device, like optimal/max io size etc.
having the elevator take a "don't bother optimizing for seeks" flag
is very much along the same lines, it's a device property that the
elevator needs to learn about in order to send the right kinds of IO
down.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: libata: set queue SSD flag for SSD devices

[Jens Axboe]

On Sat, Oct 11 2008, Arjan van de Ven wrote:
> On Sat, 11 Oct 2008 18:38:44 +0200
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > On Sat, 2008-10-11 at 09:04 -0700, Arjan van de Ven wrote:
> > > On Sat, 11 Oct 2008 17:44:13 +0200
> > > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > > 
> > > > 
> > > > > So we need something a bit more involved, but not too complex. A
> > > > > fine line...
> > > > 
> > > > It's a policy ... just let userspace do it so the user can tune
> > > > it. That's what EMC does now (except I think they key of inquiry
> > > > strings rather than cache size).
> > > 
> > > 
> > > while the chosen elevator obviously is policy, the kernel really
> > > should pick a sensible default based on what it knows.
> > > Lets put it this way: if userland needs to do a tuning to the kernel
> > > based on data only provided by the kernel, and will always do it the
> > > same way, we should have made that choice the default policy in the
> > > kernel in the first place.
> > 
> > Well, this is a bit of a nasty layering problem.  We certainly don't
> > want the Block layer to know how to poke at SATA, SCSI and other
> > esoteric media to see what elevator should be the default, so we'd
> > have to craft a new block API that the lower subsystems would
> > implement for this.  I'm really not sure it's worth the trouble when
> > the boot system can do it simply from userspace, but I'll defer to
> > Jens.
> > 
> 
> these devices already give the elevator layer information about the
> device, like optimal/max io size etc.
> having the elevator take a "don't bother optimizing for seeks" flag
> is very much along the same lines, it's a device property that the
> elevator needs to learn about in order to send the right kinds of IO
> down.

Completely agree. And this is very different from the 'choose elevator
based on device properties', a way of thinking that I don't agree with.
This 'non rotational' flag does just that, passes down more information
to the IO scheduler so it can make more informed choices.

-- 
Jens Axboe