sd take advantage of rotation speed
Subsystems (2)
Use the noop elevator by default for drives that do not spin
[Not for applying]
SSDs do not benefit from the elevator. It just wastes precious CPU cycles. By selecting the noop elevator by default, we can shave a few microseconds off each IO.
I've brazenly stolen sd_vpd_inquiry from mkp's patch here:
http://marc.info/?l=linux-scsi&m=121264354724277&w=2
No need to have two copies of that ... but this will conflict with his code.
On to the self-criticism:
I don't intend the final version of this patch to include a printk for the RPM or even a printk to say we switched IO elevator. I think we're too verbose in SCSI as it is.
I think there's an opportunity to improve sd_vpd_inquiry() to remove some of the duplicate code between sd_set_elevator() and sd_block_limits, but it's not terribly important.
The switching of the elevators isn't particularly nice. I assume that elevator_init("noop") cannot fail, which isn't true. It would be nice to use the #if 0 block instead, but that causes a null ptr dereference inside sysfs -- I suspect something isn't set up correctly.
drivers/scsi/sd.c +74 -0
@@ -1534,6 +1534,79 @@ defaults: sdkp->DPOFUA =0 ; } +static int sd_vpd_inquiry (struct scsi_disk *sdkp,unsigned char *buffer,u8 page,u8 len) +{ +int result; +unsigned char cmd[16 ]; +struct scsi_sense_hdr sshdr; + +memset (cmd,0 ,16 ); + cmd[0 ] =INQUIRY ; + cmd[1 ] =1 ;/* EVPD */ + cmd[2 ] = page;/* VPD page */ + cmd[3 ] = len; + + result =scsi_execute_req (sdkp->device, cmd,DMA_FROM_DEVICE , buffer, + len, &sshdr,SD_TIMEOUT ,SD_MAX_RETRIES ); + +if (media_not_present (sdkp, &sshdr)) +return -EIO ; + +if (result) { +sd_printk (KERN_ERR , sdkp,"EVPD Inquiry failed\n" ); +return -EIO ; + } + +if (buffer[1 ] != page) { +sd_printk (KERN_ERR , sdkp,"Page code not %2x (%2x)\n" , page, + buffer[1 ]); +return -EIO ; + } + +return buffer[3 ]; +} + +static void sd_set_elevator (struct scsi_disk *sdkp,unsigned char *buffer) +{ +struct scsi_device *sdp = sdkp->device; +int res, i, rotation; + + res =sd_vpd_inquiry (sdkp, buffer,0 ,255 ); +if (res <0 ) +return ; + +for (i =0 ; i < buffer[3 ]; i++) +if (buffer[i +4 ] ==0xb1 ) + goto found; +return ; + + found: + res =sd_vpd_inquiry (sdkp, buffer,0xb1 ,64 ); +if (res <0 ) +return ; + + rotation = (buffer[4 ] <<8 ) | buffer[5 ]; + +if (rotation ==0 ) +return ; + +if (rotation ==0x0001 ) { +#if 0 + res =elv_iosched_store (sdp->request_queue,"noop" ,5 ); +if (res >=0 ) +sd_printk (KERN_INFO , sdkp, +"Switched to noop elevator\n" ); +#else +elevator_exit (sdp->request_queue->elevator); + sdp->request_queue->elevator =NULL ; +elevator_init (sdp->request_queue,"noop" ); +sd_printk (KERN_INFO , sdkp,"Switched to noop elevator\n" ); +#endif + }else if (rotation >0x400 ) { +sd_printk (KERN_INFO , sdkp,"%u RPM drive" , rotation); + } +} +/** * sd_revalidate_disk - called the first time a new disk is seen, * performs disk spin up, read_capacity, etc.
@@ -1581,6 +1654,7 @@ static int sd_revalidate_disk(struct gendisk *disk)sd_read_capacity (sdkp, buffer);sd_read_write_protect_flag (sdkp, buffer);sd_read_cache_type (sdkp, buffer); +sd_set_elevator (sdkp, buffer); }/*
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 8 lines · Matthew Wilcox
Use the noop elevator by default for drives that do not spin
[Not for applying]
SSDs do not benefit from the elevator. It just wastes precious CPU cycles. By selecting the noop elevator by default, we can shave a few microseconds off each IO.
Quoted 6 lines
I've brazenly stolen
sd_vpd_inquiryfrom mkp's patch here:http://marc.info/?l=linux-scsi&m=121264354724277&w=2
No need to have two copies of that ... but this will conflict with his code.
Why would we want to do this in the kernel instead of a udev rule similar to how we set timeouts. Since there already exists a sysfs interface to change the IO scheduler it would seem this would be more flexible in case someone did not like the kernel policy selected.
You would need some tool to obtain the vpd data from user space that would always be installed.
There was some past discussion on replacing scsi_id with a better tool to support more options. url provided below.
http://thread.gmane.org/gmane.linux.kernel.device-mapper.devel/5289/focus=5301
Quoted 118 lines
On to the self-criticism:
I don't intend the final version of this patch to include a printk for the RPM or even a printk to say we switched IO elevator. I think we're too verbose in SCSI as it is.
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.The switching of the elevators isn't particularly nice. I assume that
elevator_init("noop")cannot fail, which isn't true. It would be nice to use the #if 0 block instead, but that causes a null ptr dereference inside sysfs -- I suspect something isn't set up correctly.Not-signed-off-by Matthew Wilcox <[email protected]>drivers/scsi/sd.c +74
-0@@ -1534,6 +1534,79 @@ defaults: sdkp->DPOFUA =0 ; } +static int sd_vpd_inquiry (struct scsi_disk *sdkp,unsigned char *buffer,u8 page,u8 len) +{ +int result; +unsigned char cmd[16 ]; +struct scsi_sense_hdr sshdr; + +memset (cmd,0 ,16 ); + cmd[0 ] =INQUIRY ; + cmd[1 ] =1 ;/* EVPD */ + cmd[2 ] = page;/* VPD page */ + cmd[3 ] = len; + + result =scsi_execute_req (sdkp->device, cmd,DMA_FROM_DEVICE , buffer, + len, &sshdr,SD_TIMEOUT ,SD_MAX_RETRIES ); + +if (media_not_present (sdkp, &sshdr)) +return -EIO ; + +if (result) { +sd_printk (KERN_ERR , sdkp,"EVPD Inquiry failed\n" ); +return -EIO ; + } + +if (buffer[1 ] != page) { +sd_printk (KERN_ERR , sdkp,"Page code not %2x (%2x)\n" , page, + buffer[1 ]); +return -EIO ; + } + +return buffer[3 ]; +} + +static void sd_set_elevator (struct scsi_disk *sdkp,unsigned char *buffer) +{ +struct scsi_device *sdp = sdkp->device; +int res, i, rotation; + + res =sd_vpd_inquiry (sdkp, buffer,0 ,255 ); +if (res <0 ) +return ; + +for (i =0 ; i < buffer[3 ]; i++) +if (buffer[i +4 ] ==0xb1 ) + goto found; +return ; + + found: + res =sd_vpd_inquiry (sdkp, buffer,0xb1 ,64 ); +if (res <0 ) +return ; + + rotation = (buffer[4 ] <<8 ) | buffer[5 ]; + +if (rotation ==0 ) +return ; + +if (rotation ==0x0001 ) { +#if 0 + res =elv_iosched_store (sdp->request_queue,"noop" ,5 ); +if (res >=0 ) +sd_printk (KERN_INFO , sdkp, +"Switched to noop elevator\n" ); +#else +elevator_exit (sdp->request_queue->elevator); + sdp->request_queue->elevator =NULL ; +elevator_init (sdp->request_queue,"noop" ); +sd_printk (KERN_INFO , sdkp,"Switched to noop elevator\n" ); +#endif + }else if (rotation >0x400 ) { +sd_printk (KERN_INFO , sdkp,"%u RPM drive" , rotation); + } +} +/** * sd_revalidate_disk - called the first time a new disk is seen, * performs disk spin up, read_capacity, etc. @@ -1581,6 +1654,7 @@ static int sd_revalidate_disk(struct gendisk *disk)sd_read_capacity (sdkp, buffer);sd_read_write_protect_flag (sdkp, buffer);sd_read_cache_type (sdkp, buffer); +sd_set_elevator (sdkp, buffer); }/* Signature · 8 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
-andmike
--
Michael Anderson
[email protected]
Quoted 4 lines · Mike Anderson
Why would we want to do this in the kernel instead of a udev rule similar to how we set timeouts. Since there already exists a sysfs interface to change the IO scheduler it would seem this would be more flexible in case someone did not like the kernel policy selected.
You don't lose any flexibility either way; you can always change the elevator default. I just thought this would be a nice little hack to give people with SSDs a better default. I'm not against doing it with udev, but it's not a lot of extra code either way.
Quoted 2 lines
You would need some tool to obtain the vpd data from user space that would always be installed.
Yes. I've been using sg_inq from sg3_utils to play with it, but one could write a specialised tool very quickly.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 2 lines · Matthew Wilcox
Use the noop elevator by default for drives that do not spin
<snip>
Quoted 6 lines
I've brazenly stolen
sd_vpd_inquiryfrom mkp's patch here:http://marc.info/?l=linux-scsi&m=121264354724277&w=2
No need to have two copies of that ... but this will conflict with his code.
<snip>
Quoted 4 lines
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.
<snip>
Quoted 31 lines
Not-signed-off-by Matthew Wilcox <[email protected]>drivers/scsi/sd.c +21
-0@@ -1534,6 +1534,79 @@ defaults: sdkp->DPOFUA =0 ; } +static int sd_vpd_inquiry (struct scsi_disk *sdkp,unsigned char *buffer,u8 page,u8 len) +{ +int result; +unsigned char cmd[16 ]; +struct scsi_sense_hdr sshdr; + +memset (cmd,0 ,16 ); + cmd[0 ] =INQUIRY ; + cmd[1 ] =1 ;/* EVPD */ + cmd[2 ] = page;/* VPD page */ + cmd[3 ] = len; + + result =scsi_execute_req (sdkp->device, cmd,DMA_FROM_DEVICE , buffer, + len, &sshdr,SD_TIMEOUT ,SD_MAX_RETRIES ); + +if (media_not_present (sdkp, &sshdr)) +return -EIO ; + +if (result) { +sd_printk (KERN_ERR , sdkp,"EVPD Inquiry failed\n" ); +return -EIO ;
drivers/scsi/sd.c defaults: · line 1567
Quoted 10 lines
drivers/scsi/sd.c +10
-0+ } + +if (buffer[1 ] != page) { +sd_printk (KERN_ERR , sdkp,"Page code not %2x (%2x)\n" , page, + buffer[1 ]); +return -EIO ; + } + +return buffer[3 ]; +}
I was just copy/pasting the same code into my ULD. Perhaps the low level of above could be made into a scsi_vpd_inquiry(...). From a quick search it looks like there are 3 more places that issue _vpd_inquiry in the tree today.
<snip>
Some of the more interesting device information are in vpd's. we should have a generic user-mode way to inquire them. sysfs structure, ioctl, I'm not sure, something good for udev. There are tones of such little knobs that could be tuned according to vpd information.
Thanks for doing this, I will investigate if current iscsi-targets will immediately benefit from this too. From my testing I found that it is usually better to let the target side elevator do the work, and set the Initiator elevator to noop. Do you know what the standard say about the default value should be, in the case the page is not present. I would assume it means "No spinning media" too, right?
Boaz
Quoted 42 lines · Boaz Harrosh
Quoted 2 lines · Matthew Wilcox
Use the noop elevator by default for drives that do not spin
<snip>
Quoted 6 lines
I've brazenly stolen
sd_vpd_inquiryfrom mkp's patch here:http://marc.info/?l=linux-scsi&m=121264354724277&w=2
No need to have two copies of that ... but this will conflict with his code.
<snip>
Quoted 31 lines
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.drivers/scsi/sd.c +27
-0+static int sd_vpd_inquiry (struct scsi_disk *sdkp,unsigned char *buffer,u8 page,u8 len) +{ +int result; +unsigned char cmd[16 ]; +struct scsi_sense_hdr sshdr; + +memset (cmd,0 ,16 ); + cmd[0 ] =INQUIRY ; + cmd[1 ] =1 ;/* EVPD */ + cmd[2 ] = page;/* VPD page */ + cmd[3 ] = len; + + result =scsi_execute_req (sdkp->device, cmd,DMA_FROM_DEVICE , buffer, + len, &sshdr,SD_TIMEOUT ,SD_MAX_RETRIES ); + +if (media_not_present (sdkp, &sshdr)) +return -EIO ; + +if (result) { +sd_printk (KERN_ERR , sdkp,"EVPD Inquiry failed\n" ); +return -EIO ; + } + +if (buffer[1 ] != page) { +sd_printk (KERN_ERR , sdkp,"Page code not %2x (%2x)\n" , page, + buffer[1 ]); +return -EIO ;
Quoted 8 lines
drivers/scsi/sd.c defaults: · line 1567
Quoted 4 lines
drivers/scsi/sd.c +4
-0+ } + +return buffer[3 ]; +}I was just copy/pasting the same code into my ULD. Perhaps the low level of above could be made into a
scsi_vpd_inquiry(...). From a quick search it looks like there are 3 more places that issue_vpd_inquiryin the tree today.
Yes, there's nothing sd-specific about it. Let me just make that minor improvement and I'll send out a proposal for scsi_vpd_inquiry().
Quoted 4 lines
Some of the more interesting device information are in vpd's. we should have a generic user-mode way to inquire them. sysfs structure, ioctl, I'm not sure, something good for udev. There are tones of such little knobs that could be tuned according to vpd information.
We have sg_inq and sg_vpd from Doug's sg3-utils.
Quoted 5 lines
Thanks for doing this, I will investigate if current iscsi-targets will immediately benefit from this too. From my testing I found that it is usually better to let the target side elevator do the work, and set the Initiator elevator to noop. Do you know what the standard say about the default value should be, in the case the page is not present. I would assume it means "No spinning media" too, right?
It means "Not reported". Which I translate from standard-ese as "Could be anything".
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 61 lines · Matthew Wilcox
Quoted 41 lines · Boaz Harrosh
Quoted 2 lines · Matthew Wilcox
Use the noop elevator by default for drives that do not spin
<snip>
Quoted 6 lines
I've brazenly stolen
sd_vpd_inquiryfrom mkp's patch here:http://marc.info/?l=linux-scsi&m=121264354724277&w=2
No need to have two copies of that ... but this will conflict with his code.
<snip>
Quoted 30 lines
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.drivers/scsi/sd.c +26
-0+static int sd_vpd_inquiry (struct scsi_disk *sdkp,unsigned char *buffer,u8 page,u8 len) +{ +int result; +unsigned char cmd[16 ]; +struct scsi_sense_hdr sshdr; + +memset (cmd,0 ,16 ); + cmd[0 ] =INQUIRY ; + cmd[1 ] =1 ;/* EVPD */ + cmd[2 ] = page;/* VPD page */ + cmd[3 ] = len; + + result =scsi_execute_req (sdkp->device, cmd,DMA_FROM_DEVICE , buffer, + len, &sshdr,SD_TIMEOUT ,SD_MAX_RETRIES ); + +if (media_not_present (sdkp, &sshdr)) +return -EIO ; + +if (result) { +sd_printk (KERN_ERR , sdkp,"EVPD Inquiry failed\n" ); +return -EIO ; + } + +if (buffer[1 ] != page) { +sd_printk (KERN_ERR , sdkp,"Page code not %2x (%2x)\n" , page, + buffer[1 ]);Quoted 8 lines
drivers/scsi/sd.c defaults: · line 1567
Quoted 5 lines
drivers/scsi/sd.c +5
-0+return -EIO ; + } + +return buffer[3 ]; +}I was just copy/pasting the same code into my ULD. Perhaps the low level of above could be made into a
scsi_vpd_inquiry(...). From a quick search it looks like there are 3 more places that issue_vpd_inquiryin the tree today.Yes, there's nothing sd-specific about it. Let me just make that minor improvement and I'll send out a proposal for
scsi_vpd_inquiry().Quoted 4 lines
Some of the more interesting device information are in vpd's. we should have a generic user-mode way to inquire them. sysfs structure, ioctl, I'm not sure, something good for udev. There are tones of such little knobs that could be tuned according to vpd information.
We have
sg_inqandsg_vpdfrom Doug's sg3-utils.
Quoted 9 lines
Quoted 5 lines
Thanks for doing this, I will investigate if current iscsi-targets will immediately benefit from this too. From my testing I found that it is usually better to let the target side elevator do the work, and set the Initiator elevator to noop. Do you know what the standard say about the default value should be, in the case the page is not present. I would assume it means "No spinning media" too, right?
It means "Not reported". Which I translate from standard-ese as "Could be anything".
OK Thanks. I'll make sure all iscsi targets I care about return this page. Am anticipating this patchset to be submitted, thanks again.
Boaz
James Bottomley
maintainerQuoted 16 lines · Matthew Wilcox
Use the noop elevator by default for drives that do not spin
[Not for applying]
SSDs do not benefit from the elevator. It just wastes precious CPU cycles. By selecting the noop elevator by default, we can shave a few microseconds off each IO.
I've brazenly stolen
sd_vpd_inquiryfrom mkp's patch here:http://marc.info/?l=linux-scsi&m=121264354724277&w=2
No need to have two copies of that ... but this will conflict with his code.
On to the self-criticism:
Quoted 7 lines
I don't intend the final version of this patch to include a printk for the RPM or even a printk to say we switched IO elevator. I think we're too verbose in SCSI as it is.
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.
and actually ses.c as well.
Quoted 4 lines
The switching of the elevators isn't particularly nice. I assume that
elevator_init("noop")cannot fail, which isn't true. It would be nice to use the #if 0 block instead, but that causes a null ptr dereference inside sysfs -- I suspect something isn't set up correctly.
I'm really not very keen on this patch, since it's implementing elevator policy from sd which is a bit of a layering violation (and a policy should be in userspace one).
What's wrong with doing this entirely from udev (it already issues the vpd's today, that's how it gets the id from page 0x83 ... it could easily look at 0xb1 and set the elevator
using /sys/block/<>/queue/scheduler)?.
James
Quoted 5 lines · James Bottomley
Quoted 3 lines
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.and actually
ses.cas well.
Ah yes. I'll rip it out of ses.c. Thanks.
A question of policy ... should we interrogate page 0 to find out if page 0x83 exists? You don't currently, but ses.c is limited to devices with an enclosure ... which is presumably only newer devices. Do we have any idea if devices blow up on being asked for random VPD that they might not have?
Quoted 8 lines
Quoted 4 lines
The switching of the elevators isn't particularly nice. I assume that
elevator_init("noop")cannot fail, which isn't true. It would be nice to use the #if 0 block instead, but that causes a null ptr dereference inside sysfs -- I suspect something isn't set up correctly.I'm really not very keen on this patch, since it's implementing elevator policy from sd which is a bit of a layering violation (and a policy should be in userspace one).
It's only choosing a default, not implementing policy.
Quoted 4 lines
What's wrong with doing this entirely from udev (it already issues the vpd's today, that's how it gets the id from page
0x83... it could easily look at0xb1and set the elevator
using /sys/block/<>/queue/scheduler)?.
I'll take a look at udev on Monday. It certainly seems less controversial than this patch.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Matthew" == Matthew Wilcox <[email protected]> writes:
Matthew> A question of policy ... should we interrogate page 0 to find Matthew> out if page 0x83 exists? You don't currently, but ses.c is Matthew> limited to devices with an enclosure ... which is presumably Matthew> only newer devices. Do we have any idea if devices blow up Matthew> on being asked for random VPD that they might not have?
We should check.
I actually changed my VPD code a bit on Friday. There's a function to do the heavy lifting and sanity checking, and a helper that can be called without worrying about the details. I moved the page list checking from the caller to the helper function.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
drivers/scsi/sd.c +105 -0
@@ -1534,6 +1538,109 @@ defaults: sdkp->DPOFUA =0 ; } +static int sd_vpd_inquiry (struct scsi_disk *sdkp,unsigned char *buffer,u8 page,u8 len) +{ +int result; +unsigned char cmd[16 ]; +struct scsi_sense_hdr sshdr; + +memset (cmd,0 ,16 ); + cmd[0 ] =INQUIRY ; + cmd[1 ] =1 ;/* EVPD */ + cmd[2 ] = page;/* VPD page */ + cmd[3 ] = len; + + result =scsi_execute_req (sdkp->device, cmd,DMA_FROM_DEVICE , buffer, + len, &sshdr,SD_TIMEOUT ,SD_MAX_RETRIES ); + +if (media_not_present (sdkp, &sshdr)) +return -EIO ; + +if (result) { +sd_printk (KERN_ERR , sdkp,"EVPD Inquiry failed\n" ); +return -EIO ; + } + +if (buffer[1 ] != page) { +sd_printk (KERN_ERR , sdkp,"Page code not %2x (%2x)\n" , page, + buffer[1 ]); +return -EIO ; + } + +return buffer[3 ]; +} + +static int sd_get_vpd_page (struct scsi_disk *sdkp,unsigned char *buffer,u8 page,u8 len) +{ +int i, result; + +/* Get Supported Pages list */ +if (sd_vpd_inquiry (sdkp, buffer,0x0 ,255 ) <0 ) +return -1 ; + +for (i =0 ; i < buffer[3 ] ; i++) +if (buffer[4 +i] == page) + goto found; + +return -1 ; + +found: + result =sd_vpd_inquiry (sdkp, buffer, page, len); + +if (result <0 ) +return -1 ; + +if (buffer[3 ] != len) +sd_printk (KERN_ERR , sdkp,"VPD pg %x, req. %u bytes, got %u\n" , + page, len, buffer[3 ]); + +return result; +} + +/** +* sd_block_limits - Query disk device for preferred I/O sizes. +* @disk: disk to query +* @buffer: temporary buffer to store inquiry response in +*/ +static void sd_block_limits (struct scsi_disk *sdkp,unsigned char *buffer) +{ +struct io_topology *iot = sdkp->disk->topology; + +/* Block Limits VPD */ +if (sd_get_vpd_page (sdkp, buffer,0xb0 ,16 ) <0 ) +return ; + + iot->phys_off =0 ; + iot->opt_block =be16_to_cpup ((__be16 *)&buffer[6 ]) + * sdkp->device->sector_size; + iot->max_length =be32_to_cpup ((__be32 *)&buffer[8 ]) + * sdkp->device->sector_size; + iot->opt_length =be32_to_cpup ((__be32 *)&buffer[12 ]) + * sdkp->device->sector_size; +} + +/** +* sd_block_characteristics - Query block dev. characteristics +* @disk: disk to query +* @buffer: temporary buffer to store inquiry response in +*/ +static void sd_block_characteristics (struct scsi_disk *sdkp,unsigned char *buffer) +{ +struct io_topology *iot = sdkp->disk->topology; +unsigned int rotation; + +/* Block Device Characteristics VPD */ +if (sd_get_vpd_page (sdkp, buffer,0xb1 ,64 ) <0 ) +return ; + + rotation =be16_to_cpup ((__be16 *)&buffer[4 ]); + +if (rotation ==1 ) + iot->dev_type =IO_TYPE_SSD ; +else if (rotation >0x400 ) + iot->dev_type =IO_TYPE_DISK ; +} +/** * sd_revalidate_disk - called the first time a new disk is seen, * performs disk spin up, read_capacity, etc.
@@ -1579,6 +1686,8 @@ static int sd_revalidate_disk(struct gen*/ if (sdkp->media_present) {sd_read_capacity (sdkp, buffer); +sd_block_limits (sdkp, buffer); +sd_block_characteristics (sdkp, buffer);sd_read_write_protect_flag (sdkp, buffer);sd_read_cache_type (sdkp, buffer); }
drivers/scsi/sd.c defaults: · line 1551
Quoted 5 lines · Martin K. Petersen
drivers/scsi/sd.c +5
-0+memset (cmd,0 ,16 ); + cmd[0 ] =INQUIRY ; + cmd[1 ] =1 ;/* EVPD */ + cmd[2 ] = page;/* VPD page */ + cmd[3 ] = len;
This looks like a bug to me.
The MSB of the length is supposed to be in byte 3 and the LSB in byte 4. So you're asking for much more data than could fit in the buffer -- fortunately the device won't be able to return as much as you ask for, so the buffer won't overflow.
Quoted 8 lines
drivers/scsi/sd.c +8
-0+ result =scsi_execute_req (sdkp->device, cmd,DMA_FROM_DEVICE , buffer, + len, &sshdr,SD_TIMEOUT ,SD_MAX_RETRIES ); + +if (media_not_present (sdkp, &sshdr)) +return -EIO ; + +if (result) { +sd_printk (KERN_ERR , sdkp,"EVPD Inquiry failed\n" );
drivers/scsi/sd.c defaults: · line 1570
Quoted 10 lines
drivers/scsi/sd.c +10
-0+return -EIO ; + } + +if (buffer[1 ] != page) { +sd_printk (KERN_ERR , sdkp,"Page code not %2x (%2x)\n" , page, + buffer[1 ]); +return -EIO ; + } + +return buffer[3 ];
This isn't true for all pages. For example, 0x83 and 0x85 have a length in buffer[2] and [3]. Now, all other pages I've looked at reserve byte 2, and devices should be setting that to 0 ... but I wouldn't feel right depending on it. For scsi_request_vpd(), I think we should return 0 for success and -Error. The caller knows which page it requested and can find out the length for itself.
Quoted 16 lines
drivers/scsi/sd.c +16
-0+static int sd_get_vpd_page (struct scsi_disk *sdkp,unsigned char *buffer,u8 page,u8 len) +{ +int i, result; + +/* Get Supported Pages list */ +if (sd_vpd_inquiry (sdkp, buffer,0x0 ,255 ) <0 ) +return -1 ; + +for (i =0 ; i < buffer[3 ] ; i++) +if (buffer[4 +i] == page) + goto found; + +return -1 ; + +found: + result =sd_vpd_inquiry (sdkp, buffer, page, len);
drivers/scsi/sd.c defaults: · line 1598
Quoted 10 lines
drivers/scsi/sd.c +10
-0+ +if (result <0 ) +return -1 ; + +if (buffer[3 ] != len) +sd_printk (KERN_ERR , sdkp,"VPD pg %x, req. %u bytes, got %u\n" , + page, len, buffer[3 ]); + +return result; +}
Yes, this refactoring is exactly what I had in mind.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Matthew" == Matthew Wilcox <[email protected]> writes:
Quoted 2 lines · Martin K. Petersen
Quoted 2 lines
+
memset(cmd, 0, 16); + cmd[0] = INQUIRY; + cmd[1] = 1; /* EVPD */
+ cmd[2] = page; /* VPD page */ + cmd[3] = len;
Matthew> This looks like a bug to me.
Yeah, what I meant was cmd[4] = len;
Matthew> This isn't true for all pages. For example, 0x83 and 0x85 Matthew> have a length in buffer[2] and [3].
Fair enough. I only looked at b0 and b1.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
James Bottomley
maintainerQuoted 7 lines · Matthew Wilcox
Quoted 5 lines · James Bottomley
Quoted 3 lines
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.and actually
ses.cas well.
Quoted 7 lines
Ah yes. I'll rip it out of
ses.c. Thanks.A question of policy ... should we interrogate page 0 to find out if page
0x83exists? You don't currently, butses.cis limited to devices with an enclosure ... which is presumably only newer devices. Do we have any idea if devices blow up on being asked for random VPD that they might not have?
It's wise to. For true SCSI devices (going all the way back to my 8" HVD FAST-10 ones of ca 1989) the answer is that they respond correctly (as in ILLEGAL REQUEST). For the ATA devices through SAT, the answer's also that. Unfortunately, for devices which have their own eccentric SCSI interpreters (glares at USB) the answer is that they can do horrible things: we had a bug filed only this year where a VPD inquiry was crashing a USB device. This is another reason for punting this to userspace. The heuristics of what works are already embedded into udev.
I think the solution is eventually just to push all VPD stuff to userspace (including ses), but that's long term.
Quoted 11 lines
Quoted 8 lines
Quoted 4 lines
The switching of the elevators isn't particularly nice. I assume that
elevator_init("noop")cannot fail, which isn't true. It would be nice to use the #if 0 block instead, but that causes a null ptr dereference inside sysfs -- I suspect something isn't set up correctly.I'm really not very keen on this patch, since it's implementing elevator policy from sd which is a bit of a layering violation (and a policy should be in userspace one).
It's only choosing a default, not implementing policy.
Quoted 7 lines
Quoted 4 lines
What's wrong with doing this entirely from udev (it already issues the vpd's today, that's how it gets the id from page
0x83... it could easily look at0xb1and set the elevator
using /sys/block/<>/queue/scheduler)?.I'll take a look at udev on Monday. It certainly seems less controversial than this patch.
Thanks!
James
Quoted 20 lines · Matthew Wilcox
Use the noop elevator by default for drives that do not spin
[Not for applying]
SSDs do not benefit from the elevator. It just wastes precious CPU cycles. By selecting the noop elevator by default, we can shave a few microseconds off each IO.
I've brazenly stolen
sd_vpd_inquiryfrom mkp's patch here:http://marc.info/?l=linux-scsi&m=121264354724277&w=2
No need to have two copies of that ... but this will conflict with his code.
On to the self-criticism:
I don't intend the final version of this patch to include a printk for the RPM or even a printk to say we switched IO elevator. I think we're too verbose in SCSI as it is.
Quoted 8 lines
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.The switching of the elevators isn't particularly nice. I assume that
elevator_init("noop")cannot fail, which isn't true. It would be nice to use the #if 0 block instead, but that causes a null ptr dereference inside sysfs -- I suspect something isn't set up correctly.
I disagree with this approach. For now, lets just add a queue flag that says the device doesn't have a seek penalty and let the io schedulers do what they need to avoid that (it'd be a one-liner change to cfq and as). There's more to io scheduling than just seek reduction, so this is the wrong direction to take imo.
Signature · 1 line
Jens Axboe
Quoted 30 lines · Jens Axboe
Quoted 20 lines · Matthew Wilcox
Use the noop elevator by default for drives that do not spin
[Not for applying]
SSDs do not benefit from the elevator. It just wastes precious CPU cycles. By selecting the noop elevator by default, we can shave a few microseconds off each IO.
I've brazenly stolen
sd_vpd_inquiryfrom mkp's patch here:http://marc.info/?l=linux-scsi&m=121264354724277&w=2
No need to have two copies of that ... but this will conflict with his code.
On to the self-criticism:
I don't intend the final version of this patch to include a printk for the RPM or even a printk to say we switched IO elevator. I think we're too verbose in SCSI as it is.
Quoted 8 lines
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.The switching of the elevators isn't particularly nice. I assume that
elevator_init("noop")cannot fail, which isn't true. It would be nice to use the #if 0 block instead, but that causes a null ptr dereference inside sysfs -- I suspect something isn't set up correctly.
Quoted 5 lines
I disagree with this approach. For now, lets just add a queue flag that says the device doesn't have a seek penalty and let the io schedulers do what they need to avoid that (it'd be a one-liner change to cfq and as). There's more to io scheduling than just seek reduction, so this is the wrong direction to take imo.
OK, a two liner for CFQ. Something like this would be much better I think - maintain the check in SCSI but flag the queue as seek-free if SSD is detected, and let the lower layer worry about the details. Much cleaner, doesn't allow dirty SCSI fingers where they don't belong.
block/blk-settings.c +12 -0
@@ -410,6 +410,18 @@ void blk_queue_update_dma_alignment(struct request_queue *q, int mask) }EXPORT_SYMBOL (blk_queue_update_dma_alignment); +void blk_queue_seek_free (struct request_queue *q,int set) +{ +/* +* initialization stuff, don't worry about concurrency +*/ +if (set) +queue_flag_set_unlocked (QUEUE_FLAG_SEEKFREE , q); +else +queue_flag_clear_unlocked (QUEUE_FLAG_SEEKFREE , q); +} +EXPORT_SYMBOL (blk_queue_seek_free); +static int __init blk_settings_init (void ) { blk_max_low_pfn = max_low_pfn -1 ;
block/cfq-iosched.c +4 -2
@@ -1423,7 +1423,8 @@ retry:cfq_init_prio_data (cfqq, ioc);if (is_sync) { -if (!cfq_class_idle (cfqq)) +if (!cfq_class_idle (cfqq) && + !test_bit (QUEUE_FLAG_SEEKFREE , &cfqd->queue->queue_flags))cfq_mark_cfqq_idle_window (cfqq);cfq_mark_cfqq_sync (cfqq); }
@@ -1680,7 +1681,8 @@ cfq_update_idle_window(struct cfq_data *cfqd, struct cfq_queue *cfqq,/* * Don't idle for async or idle io prio class */ -if (!cfq_cfqq_sync (cfqq) ||cfq_class_idle (cfqq)) +if (!cfq_cfqq_sync (cfqq) ||cfq_class_idle (cfqq) || +test_bit (QUEUE_FLAG_SEEKFREE , &cfqd->queue->queue_flags))return ; enable_idle =cfq_cfqq_idle_window (cfqq);
include/linux/blkdev.h +2 -0
@@ -409,6 +409,7 @@ struct request_queue#define QUEUE_FLAG_ELVSWITCH 8/* don't use elevator, just do FIFO */ #define QUEUE_FLAG_BIDI 9/* queue supports bidi requests */ #define QUEUE_FLAG_NOMERGES 10/* disable merge attempts */ +#define QUEUE_FLAG_SEEKFREE 11/* seeks are free */ static inline int queue_is_locked (struct request_queue *q) {
@@ -757,6 +758,7 @@ extern void blk_queue_prep_rq(struct request_queue *, prep_rq_fn *pfn);extern void blk_queue_merge_bvec (struct request_queue *,merge_bvec_fn *);extern void blk_queue_dma_alignment (struct request_queue *,int );extern void blk_queue_update_dma_alignment (struct request_queue *,int ); +extern void blk_queue_seek_free (struct request_queue *,int );extern void blk_queue_softirq_done (struct request_queue *,softirq_done_fn *);extern struct backing_dev_info *blk_get_backing_dev_info (struct block_device *bdev);extern int blk_queue_ordered (struct request_queue *,unsigned ,prepare_flush_fn *);
Signature · 1 line
Jens Axboe
Quoted 32 lines · Jens Axboe
Quoted 20 lines · Matthew Wilcox
Use the noop elevator by default for drives that do not spin
[Not for applying]
SSDs do not benefit from the elevator. It just wastes precious CPU cycles. By selecting the noop elevator by default, we can shave a few microseconds off each IO.
I've brazenly stolen
sd_vpd_inquiryfrom mkp's patch here:http://marc.info/?l=linux-scsi&m=121264354724277&w=2
No need to have two copies of that ... but this will conflict with his code.
On to the self-criticism:
I don't intend the final version of this patch to include a printk for the RPM or even a printk to say we switched IO elevator. I think we're too verbose in SCSI as it is.
Quoted 9 lines
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.The switching of the elevators isn't particularly nice. I assume that
elevator_init("noop")cannot fail, which isn't true. It would be nice to use the #if 0 block instead, but that causes a null ptr dereference inside sysfs -- I suspect something isn't set up correctly.
Quoted 7 lines
I disagree with this approach. For now, lets just add a queue flag that says the device doesn't have a seek penalty and let the io schedulers do what they need to avoid that (it'd be a one-liner change to cfq and as). There's more to io scheduling than just seek reduction, so this is the wrong direction to take imo.
Very true - you still will get a significant win by coalescing IO's (say for example, to do larger, aligned writes to flash devices).
ric
Quoted 39 lines · Ric Wheeler
Quoted 31 lines · Jens Axboe
Quoted 20 lines · Matthew Wilcox
Use the noop elevator by default for drives that do not spin
[Not for applying]
SSDs do not benefit from the elevator. It just wastes precious CPU cycles. By selecting the noop elevator by default, we can shave a few microseconds off each IO.
I've brazenly stolen
sd_vpd_inquiryfrom mkp's patch here:http://marc.info/?l=linux-scsi&m=121264354724277&w=2
No need to have two copies of that ... but this will conflict with his code.
On to the self-criticism:
I don't intend the final version of this patch to include a printk for the RPM or even a printk to say we switched IO elevator. I think we're too verbose in SCSI as it is.
Quoted 9 lines
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.The switching of the elevators isn't particularly nice. I assume that
elevator_init("noop")cannot fail, which isn't true. It would be nice to use the #if 0 block instead, but that causes a null ptr dereference inside sysfs -- I suspect something isn't set up correctly.Quoted 7 lines
I disagree with this approach. For now, lets just add a queue flag that says the device doesn't have a seek penalty and let the io schedulers do what they need to avoid that (it'd be a one-liner change to cfq and as). There's more to io scheduling than just seek reduction, so this is the wrong direction to take imo.
Quoted 5 lines
Very true - you still will get a significant win by coalescing IO's (say for example, to do larger, aligned writes to flash devices).
ric
And to not let HUGE writers hug the machine. A scheduler ...
Quoted 40 lines · Boaz Harrosh
Quoted 39 lines · Ric Wheeler
Quoted 38 lines · Jens Axboe
Quoted 20 lines · Matthew Wilcox
Use the noop elevator by default for drives that do not spin
[Not for applying]
SSDs do not benefit from the elevator. It just wastes precious CPU cycles. By selecting the noop elevator by default, we can shave a few microseconds off each IO.
I've brazenly stolen
sd_vpd_inquiryfrom mkp's patch here:http://marc.info/?l=linux-scsi&m=121264354724277&w=2
No need to have two copies of that ... but this will conflict with his code.
On to the self-criticism:
I don't intend the final version of this patch to include a printk for the RPM or even a printk to say we switched IO elevator. I think we're too verbose in SCSI as it is.
Quoted 9 lines
I think there's an opportunity to improve
sd_vpd_inquiry()to remove some of the duplicate code betweensd_set_elevator()andsd_block_limits, but it's not terribly important.The switching of the elevators isn't particularly nice. I assume that
elevator_init("noop")cannot fail, which isn't true. It would be nice to use the #if 0 block instead, but that causes a null ptr dereference inside sysfs -- I suspect something isn't set up correctly.I disagree with this approach. For now, lets just add a queue flag that says the device doesn't have a seek penalty and let the io schedulers do what they need to avoid that (it'd be a one-liner change to cfq and as). There's more to io scheduling than just seek reduction, so this is the wrong direction to take imo.
Quoted 6 lines
Quoted 5 lines
Very true - you still will get a significant win by coalescing IO's (say for example, to do larger, aligned writes to flash devices).
ric
And to not let HUGE writers hug the machine. A scheduler ...
Precisely, merging and fairness is a big part of it. Plus, doing this in sd is just a blatant layer violation.
Signature · 1 line
Jens Axboe
Quoted 2 lines · Jens Axboe
Precisely, merging and fairness is a big part of it. Plus, doing this in sd is just a blatant layer violation.
I'm fine with tweaking the elevators to know about low-latency seeks. But please stop describing it as "a blatant layering violation". It's nothing of the sort. It's setting a default at a point where we find out the information which would guide us.
I haven't looked into doing this in udev yet; I've got caught up in another project. In any case, it seems like there's no role for udev any more, so I'll probably not spend any more time on this unless there's some need.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 8 lines · Matthew Wilcox
Quoted 2 lines · Jens Axboe
Precisely, merging and fairness is a big part of it. Plus, doing this in sd is just a blatant layer violation.
I'm fine with tweaking the elevators to know about low-latency seeks. But please stop describing it as "a blatant layering violation". It's nothing of the sort. It's setting a default at a point where we find out the information which would guide us.
Uhm, but it IS "a blatant layering violation", it's doing things from the wrong side up :-)
Quoted 4 lines
I haven't looked into doing this in udev yet; I've got caught up in another project. In any case, it seems like there's no role for udev any more, so I'll probably not spend any more time on this unless there's some need.
I don't think udev is a particularly good idea either. My plan was to introduce device profiles for the queue, but it is probably a bad idea to over-engineer this. So I'll keep it simple, stick to a 'zero cost seek' flag instead and allow drivers to signal that. libata/ide needs to check the ID page word to detect SSD drives as well, so they need a few lines of change too.
I'll just stick the block bit in the 2.6.27 pending queue and let the other patches go through Jeff/James/Bart.
Signature · 1 line
Jens Axboe
Quoted 2 lines · Jens Axboe
Uhm, but it IS "a blatant layering violation", it's doing things from the wrong side up :-)
That's just "doing things from a spot Jens doesn't approve of". A layering violation would be SCSI knowing how elevators work.
Quoted 6 lines
I don't think udev is a particularly good idea either. My plan was to introduce device profiles for the queue, but it is probably a bad idea to over-engineer this. So I'll keep it simple, stick to a 'zero cost seek' flag instead and allow drivers to signal that. libata/ide needs to check the ID page word to detect SSD drives as well, so they need a few lines of change too.
One of the other patches in this mess was the libata piece to translate ATA8's rotation speed to SCSI's b1 VPD page, so ide is the only remaining work left to do (can't it die already?)
Quoted 2 lines
I'll just stick the block bit in the 2.6.27 pending queue and let the other patches go through Jeff/James/Bart.
Thanks.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
James Bottomley
maintainerQuoted 6 lines · Matthew Wilcox
Quoted 2 lines · Jens Axboe
Uhm, but it IS "a blatant layering violation", it's doing things from the wrong side up :-)
That's just "doing things from a spot Jens doesn't approve of". A layering violation would be SCSI knowing how elevators work.
When it was setting particular elevators, it was a layering violation (as I said at the time). If it's just setting a seek cost hint, that's acceptable.
Quoted 10 lines
Quoted 6 lines
I don't think udev is a particularly good idea either. My plan was to introduce device profiles for the queue, but it is probably a bad idea to over-engineer this. So I'll keep it simple, stick to a 'zero cost seek' flag instead and allow drivers to signal that. libata/ide needs to check the ID page word to detect SSD drives as well, so they need a few lines of change too.
One of the other patches in this mess was the libata piece to translate ATA8's rotation speed to SCSI's b1 VPD page, so ide is the only remaining work left to do (can't it die already?)
I don't think there is any IDE work to do ... the last I heard from the manufacturers, they were all not going to bother with PATA interfaces to SSDs ... unless this has changed?
Quoted 2 lines
Quoted 2 lines
I'll just stick the block bit in the 2.6.27 pending queue and let the other patches go through Jeff/James/Bart.
James
Quoted 11 lines · James Bottomley
Quoted 6 lines · Matthew Wilcox
Quoted 2 lines · Jens Axboe
Uhm, but it IS "a blatant layering violation", it's doing things from the wrong side up :-)
That's just "doing things from a spot Jens doesn't approve of". A layering violation would be SCSI knowing how elevators work.
When it was setting particular elevators, it was a layering violation (as I said at the time). If it's just setting a seek cost hint, that's acceptable.
You're both being silly. Interacting with a subsystem via its exposed interfaces is not a layering violation.
Quoted 3 lines
I don't think there is any IDE work to do ... the last I heard from the manufacturers, they were all not going to bother with PATA interfaces to SSDs ... unless this has changed?
There are people with PATA->SATA adapters. I think there's one in your P7120 (or is that one a PATA->SATA adapter?) In any case, I don't think we need to do anything until someone complains.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 9 lines · Matthew Wilcox
Quoted 8 lines · James Bottomley
Quoted 6 lines · Matthew Wilcox
Quoted 2 lines · Jens Axboe
Uhm, but it IS "a blatant layering violation", it's doing things from the wrong side up :-)
That's just "doing things from a spot Jens doesn't approve of". A layering violation would be SCSI knowing how elevators work.
Quoted 6 lines
Quoted 3 lines
When it was setting particular elevators, it was a layering violation (as I said at the time). If it's just setting a seek cost hint, that's acceptable.
You're both being silly. Interacting with a subsystem via its exposed interfaces is not a layering violation.
Dude, get a grip, your line is reasoning is getting silly. By that logic, as long as something is exported you can use it from wherever you want? Uhm right.
Signature · 1 line
Jens Axboe
James Bottomley
maintainerQuoted 9 lines · Matthew Wilcox
Quoted 8 lines · James Bottomley
Quoted 6 lines · Matthew Wilcox
Quoted 2 lines · Jens Axboe
Uhm, but it IS "a blatant layering violation", it's doing things from the wrong side up :-)
That's just "doing things from a spot Jens doesn't approve of". A layering violation would be SCSI knowing how elevators work.
Quoted 6 lines
Quoted 3 lines
When it was setting particular elevators, it was a layering violation (as I said at the time). If it's just setting a seek cost hint, that's acceptable.
You're both being silly. Interacting with a subsystem via its exposed interfaces is not a layering violation.
It depends on the interface: sys_open is an exposed interface, but any SCSI driver trying to use it will be caught and shot for egregious layering violations.
The elevator setting interface is a policy interface designed for users, not for lower layers of the I/O stack.
Quoted 6 lines
Quoted 3 lines
I don't think there is any IDE work to do ... the last I heard from the manufacturers, they were all not going to bother with PATA interfaces to SSDs ... unless this has changed?
There are people with
PATA->SATAadapters. I think there's one in your P7120 (or is that one aPATA->SATAadapter?)
No ... it's using an IHC6 with two interfaces, one SATA and one PATA.
Quoted 2 lines
In any case, I don't think
we need to do anything until someone complains.
Agreed.
James
Quoted 6 lines · Matthew Wilcox
Quoted 2 lines · Jens Axboe
Uhm, but it IS "a blatant layering violation", it's doing things from the wrong side up :-)
That's just "doing things from a spot Jens doesn't approve of". A layering violation would be SCSI knowing how elevators work.
And that's what the patch is doing, it knows that CFQ/AS idles and thus chooses noop. So it's even doing it wrong, since that disables other goodness as well. The whole sd_set_elevator() is a mess, I honestly can't see how you can even start to defend it.
I don't really appreciate the personal bit either.
Quoted 10 lines
Quoted 6 lines
I don't think udev is a particularly good idea either. My plan was to introduce device profiles for the queue, but it is probably a bad idea to over-engineer this. So I'll keep it simple, stick to a 'zero cost seek' flag instead and allow drivers to signal that. libata/ide needs to check the ID page word to detect SSD drives as well, so they need a few lines of change too.
One of the other patches in this mess was the libata piece to translate ATA8's rotation speed to SCSI's b1 VPD page, so ide is the only remaining work left to do (can't it die already?)
IDE is definitely low priority :-). I had a pata ssd drive, but IDE is only really interesting for the bits that can't be libata driven. So not very interesting.
ATA8 seems to have changed the SSD detection scheme. I don't have an SSD disk so I cannot check what the current models report.
Signature · 1 line
Jens Axboe
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Jens" == Jens Axboe <[email protected]> writes:
Jens> Uhm, but it IS "a blatant layering violation", it's doing things Jens> from the wrong side up :-)
I also disagree with mucking with the elevator directly in sd.c.
The whole point of my I/O hints work is to expose the I/O topology in a generic fashion so that upper layers can take advantage of them.
I'll post a new batch of those patches shortly...
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 11 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Jens" == Jens Axboe <[email protected]> writes:
Jens> Uhm, but it IS "a blatant layering violation", it's doing things Jens> from the wrong side up :-)
I also disagree with mucking with the elevator directly in
sd.c.The whole point of my I/O hints work is to expose the I/O topology in a generic fashion so that upper layers can take advantage of them.
I'll post a new batch of those patches shortly...
Good, that sounds useful! Sounds a lot like what I referred to as 'device profiles'.
Signature · 1 line
Jens Axboe
Quoted 6 lines · Martin K. Petersen
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
"Jens" == Jens Axboe <[email protected]> writes:
Jens> Uhm, but it IS "a blatant layering violation", it's doing things Jens> from the wrong side up :-)
Quoted 8 lines
I also disagree with mucking with the elevator directly in
sd.c.The whole point of my I/O hints work is to expose the I/O topology in a generic fashion so that upper layers can take advantage of them.
I'll post a new batch of those patches shortly...
One other thought - is there a way to give non-rotational devices also some indication of latency? (FLASH is slower than enterprise SSD is slower than DRAM ramdisk for example)?
ric
James Bottomley
maintainerQuoted 3 lines · Ric Wheeler
One other thought - is there a way to give non-rotational devices also some indication of latency? (FLASH is slower than enterprise SSD is slower than DRAM ramdisk for example)?
Not at the moment ... since libata uses SAT, we're bound by what the SBC mode pages can do for us, so the VPD only reports rotational speed (or a specific flag for non rotation) and form factor. There was once a flexible disk mode page that showed some of this, but it's been listed as obsolete since SBC-2.
There are, perhaps other VPD parameters we should be paying attention to, like Block Limits (B0) which shows optimal transfer length and granularity.
James
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Ric" == Ric Wheeler <[email protected]> writes:
Ric> One other thought - is there a way to give non-rotational devices Ric> also some indication of latency? (FLASH is slower than enterprise Ric> SSD is slower than DRAM ramdisk for example)?
The current SBC draft only distinguishes between rotating media speeds. There is only one classification for non-rotating media in the block device characteristics VPD.
For a mechanical disk drive the rpm isn't a terrible gauge for performance. But for a solid state device I think it will be hard to define a similar universal metric.
Ignoring SLC vs. MLC for a moment I think it's also safe to predict that the enterprise drive of today will be the consumer drive of tomorrow.
Maybe the ssd device could export the anticipated command response time for a request that matches the Optimal Transfer Length field in the block limits VPD?
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 6 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Ric" == Ric Wheeler <[email protected]> writes:
Ric> One other thought - is there a way to give non-rotational devices Ric> also some indication of latency? (FLASH is slower than enterprise Ric> SSD is slower than DRAM ramdisk for example)?
Quoted 7 lines
The current SBC draft only distinguishes between rotating media speeds. There is only one classification for non-rotating media in the block device characteristics VPD.
For a mechanical disk drive the rpm isn't a terrible gauge for performance. But for a solid state device I think it will be hard to define a similar universal metric.
rpm isn't a great gauge of performance either since the perf is a function of rpm * bit density.
Quoted 7 lines
Ignoring SLC vs. MLC for a moment I think it's also safe to predict that the enterprise drive of today will be the consumer drive of tomorrow.
Maybe the ssd device could export the anticipated command response time for a request that matches the Optimal Transfer Length field in the block limits VPD?
erase and/or write times could be exported as well somehow for SSDs if the FS (or other higher layer that wants to know) can't avoid garbage collection and erase cycles. I was just told today that flash devices have 10x higher write time than read time. erase is another order of magnitude higher. This doesn't include any garbage collection overhead.
I think new file systems should be tuned to work with SSDs before we worry so much about the differences between SSDs/flash technologies and vendors. And then prescribe a different FS for different storage technologies. This avoids the "layering violations" discussion and helps keep the FSs (testing and developement) substantially simpler.
grant
Quoted 33 lines · Grant Grundler
Quoted 6 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Ric" == Ric Wheeler <[email protected]> writes:
Ric> One other thought - is there a way to give non-rotational devices Ric> also some indication of latency? (FLASH is slower than enterprise Ric> SSD is slower than DRAM ramdisk for example)?
Quoted 7 lines
The current SBC draft only distinguishes between rotating media speeds. There is only one classification for non-rotating media in the block device characteristics VPD.
For a mechanical disk drive the rpm isn't a terrible gauge for performance. But for a solid state device I think it will be hard to define a similar universal metric.
rpm isn't a great gauge of performance either since the perf is a function of rpm * bit density.
Quoted 7 lines
Ignoring SLC vs. MLC for a moment I think it's also safe to predict that the enterprise drive of today will be the consumer drive of tomorrow.
Maybe the ssd device could export the anticipated command response time for a request that matches the Optimal Transfer Length field in the block limits VPD?
erase and/or write times could be exported as well somehow for SSDs if the FS (or other higher layer that wants to know) can't avoid garbage collection and erase cycles. I was just told today that flash devices have 10x higher write time than read time. erase is another order of magnitude higher. This doesn't include any garbage collection overhead.
Quoted 6 lines
I think new file systems should be tuned to work with SSDs before we worry so much about the differences between SSDs/flash technologies and vendors. And then prescribe a different FS for different storage technologies. This avoids the "layering violations" discussion and helps keep the FSs (testing and developement) substantially simpler.
Could performance patterns be encoded in some sort of per-product data structure ala the scsi_static_device_list?
Andrwe
Quoted 13 lines · Grant Grundler
Quoted 10 lines · Martin K. Petersen
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
"Ric" == Ric Wheeler <[email protected]> writes:
Ric> One other thought - is there a way to give non-rotational devices Ric> also some indication of latency? (FLASH is slower than enterprise Ric> SSD is slower than DRAM ramdisk for example)?
The current SBC draft only distinguishes between rotating media speeds. There is only one classification for non-rotating media in the block device characteristics VPD.
Quoted 8 lines
Quoted 4 lines
For a mechanical disk drive the rpm isn't a terrible gauge for performance. But for a solid state device I think it will be hard to define a similar universal metric.
rpm isn't a great gauge of performance either since the perf is a function of rpm * bit density.
Is this real rotational latency, or normalized? I think that the avg seek time is usually a bit more predictive of how well we do with the worst case load (fsck).
Quoted 10 lines
Quoted 9 lines
Ignoring SLC vs. MLC for a moment I think it's also safe to predict that the enterprise drive of today will be the consumer drive of tomorrow.
Maybe the ssd device could export the anticipated command response time for a request that matches the Optimal Transfer Length field in the block limits VPD?
Quoted 6 lines
erase and/or write times could be exported as well somehow for SSDs if the FS (or other higher layer that wants to know) can't avoid garbage collection and erase cycles. I was just told today that flash devices have 10x higher write time than read time. erase is another order of magnitude higher. This doesn't include any garbage collection overhead.
This is changing - they have various ways of getting them much closer together. On the other hand, a USB flash drive is much slower than a high end SSD which can hit 20,000 IOPS.
Quoted 8 lines
I think new file systems should be tuned to work with SSDs before we worry so much about the differences between SSDs/flash technologies and vendors. And then prescribe a different FS for different storage technologies. This avoids the "layering violations" discussion and helps keep the FSs (testing and developement) substantially simpler.
grant
If we have access to the parts, we will try to get them to self tune given whatever we can grab.
ric
Quoted 1 line · Ric Wheeler
Grant Grundler wrote:
...
Quoted 5 lines
Quoted 3 lines
rpm isn't a great gauge of performance either since the perf is a function of rpm * bit density.
Is this real rotational latency, or normalized?
Sorry... I was thinking throughput (MB/s) and not rotational latency. mkp is right that rotaional latency (ie rpm) is a significant part of total avg latency.
Quoted 3 lines
I think that the avg seek
time is usually a bit more predictive of how well we do with the worst case load (fsck).
*nod*
....
Quoted 11 lines
Quoted 7 lines
erase and/or write times could be exported as well somehow for SSDs if the FS (or other higher layer that wants to know) can't avoid garbage collection and erase cycles. I was just told today that flash devices
have 10x higher write time than read time. erase is another order of magnitude higher. This doesn't include any garbage collection overhead.This is changing - they have various ways of getting them much closer together. On the other hand, a USB flash drive is much slower than a high end SSD which can hit 20,000 IOPS.
The IOPS isn't the issue. It's the difference between having a rotation/seek latency and not. The efficiency (percent of theoretical throughput or IOPS) of either USB stick or enterprise SSD shouldn't be that different if the FS is aware of erase blocks and how it lays out the data. The USB stick will always be slower - the question is efficiency not comparing it to a faster SSD.
Quoted 7 lines
Quoted 7 lines
I think new file systems should be tuned to work with SSDs before we worry so much about the differences between SSDs/flash technologies and vendors. And then prescribe a different FS for different storage technologies. This avoids the "layering violations" discussion and helps keep the FSs (testing and developement) substantially simpler.
Quoted 5 lines
Quoted 2 lines
grant
If we have access to the parts, we will try to get them to self tune given whatever we can grab.
I'm just pointing out that "self tuning" is more complex, more work, and requires
more testing. Ultimately it will take longer to deploy and I don't think we have time for it. Just trying to defer this work until I see substantial evidence of at
least one FS that works well with SSDs and can be deployed.
I'm not against "self tuning". But I believe sufficiently good results are achievable by directly embedding the device attributes in the FS as we've done for conventional rotational media in ext2.
Maybe I'm being too skeptical that developments can't be done in parallel for new features.
thanks,
grants