scsi: associate bio write hint with WRITE CDB
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 10 lines · Christoph Hellwig
Quoted 9 lines · Randall Huang
In SPC-3, WRITE(10)/(16) support grouping function. Let's associate bio write hint with group number for enabling StreamID or Turbo Write feature.
Signed-off-by Randall Huang <[email protected]>--- drivers/scsi/sd.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
Quoted 16 lines
Quoted 5 lines
drivers/scsi/sd.c +0
-0drivers/scsi/sd.c static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt) · line 1208
Quoted 9 lines
drivers/scsi/sd.c +5
-1@@ -1201,7 +1201,12 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt) SCpnt->cmnd[11 ] = (unsigned char ) (this_count >>16 ) &0xff ; SCpnt->cmnd[12 ] = (unsigned char ) (this_count >>8 ) &0xff ; SCpnt->cmnd[13 ] = (unsigned char ) this_count &0xff ; - SCpnt->cmnd[14 ] = SCpnt->cmnd[15 ] =0 ; +if (rq_data_dir (rq) ==WRITE ) { + SCpnt->cmnd[14 ] = rq->bio->bi_write_hint &0x3f ; + }else { + SCpnt->cmnd[14 ] =0 ; + }No need for braces here.
Already send a new version
Quoted 4 lines
But what I'm more worried about is devices not recognizing the feature throwing up on the field. Can you check what SBC version first references these or come up with some other decently smart conditional?
My reference is SCSI Block Commands – 3 (SBC-3) Revision 25. Section 5.32 WRITE (10) and 5.34 WRITE (16)
Quoted 1 line
Maybe Martin has a good idea, too.
Quoted 27 lines · Randall Huang
Quoted 10 lines · Christoph Hellwig
Quoted 9 lines · Randall Huang
In SPC-3, WRITE(10)/(16) support grouping function. Let's associate bio write hint with group number for enabling StreamID or Turbo Write feature.
Signed-off-by Randall Huang <[email protected]>--- drivers/scsi/sd.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)Quoted 16 lines
Quoted 5 lines
drivers/scsi/sd.c +0
-0drivers/scsi/sd.c static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt) · line 1208
Quoted 9 lines
drivers/scsi/sd.c +5
-1@@ -1201,7 +1201,12 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt) SCpnt->cmnd[11 ] = (unsigned char ) (this_count >>16 ) &0xff ; SCpnt->cmnd[12 ] = (unsigned char ) (this_count >>8 ) &0xff ; SCpnt->cmnd[13 ] = (unsigned char ) this_count &0xff ; - SCpnt->cmnd[14 ] = SCpnt->cmnd[15 ] =0 ; +if (rq_data_dir (rq) ==WRITE ) { + SCpnt->cmnd[14 ] = rq->bio->bi_write_hint &0x3f ; + }else { + SCpnt->cmnd[14 ] =0 ; + }No need for braces here.
Quoted 10 lines
Already send a new version
Quoted 4 lines
But what I'm more worried about is devices not recognizing the feature throwing up on the field. Can you check what SBC version first references these or come up with some other decently smart conditional?
My reference is SCSI Block Commands – 3 (SBC-3) Revision 25. Section 5.32 WRITE (10) and 5.34 WRITE (16)
Quoted 1 line
Maybe Martin has a good idea, too.
That is the GROUP NUMBER field. Also found in READ(16) at the same location within its cdb. The proposed code deserves at least an explanatory comment.
Since it is relatively recent, perhaps the above should only be done iff:
- the REPORT SUPPORTED OPERATION CODES (RSOC) command is supported, and
- in the RSOC entry for WRITE(16), the CDB USAGE DATA field (a bit mask) indicates the GROUP NUMBER field is supported
That check can be done once, at disk attachment time where there is already code to fetch RSOC.
Is there a bi_read_hint ? If not then the bi_write_hint should also be applied to READ(16). Makes that variable naming look pretty silly though.
Doug Gilbert
Quoted 49 lines · Douglas Gilbert
Quoted 28 lines · Randall Huang
Quoted 10 lines · Christoph Hellwig
Quoted 9 lines · Randall Huang
In SPC-3, WRITE(10)/(16) support grouping function. Let's associate bio write hint with group number for enabling StreamID or Turbo Write feature.
Signed-off-by Randall Huang <[email protected]>--- drivers/scsi/sd.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)Quoted 16 lines
Quoted 5 lines
drivers/scsi/sd.c +0
-0drivers/scsi/sd.c static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt) · line 1208
Quoted 9 lines
drivers/scsi/sd.c +5
-1@@ -1201,7 +1201,12 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt) SCpnt->cmnd[11 ] = (unsigned char ) (this_count >>16 ) &0xff ; SCpnt->cmnd[12 ] = (unsigned char ) (this_count >>8 ) &0xff ; SCpnt->cmnd[13 ] = (unsigned char ) this_count &0xff ; - SCpnt->cmnd[14 ] = SCpnt->cmnd[15 ] =0 ; +if (rq_data_dir (rq) ==WRITE ) { + SCpnt->cmnd[14 ] = rq->bio->bi_write_hint &0x3f ; + }else { + SCpnt->cmnd[14 ] =0 ; + }No need for braces here.
Quoted 10 lines
Already send a new version
Quoted 4 lines
But what I'm more worried about is devices not recognizing the feature throwing up on the field. Can you check what SBC version first references these or come up with some other decently smart conditional?
My reference is SCSI Block Commands – 3 (SBC-3) Revision 25. Section 5.32 WRITE (10) and 5.34 WRITE (16)
Quoted 1 line
Maybe Martin has a good idea, too.
That is the GROUP NUMBER field. Also found in READ(16) at the same location within its cdb. The proposed code deserves at least an explanatory comment.
Since it is relatively recent, perhaps the above should only be done iff:
- the REPORT SUPPORTED OPERATION CODES (RSOC) command is supported, and
- in the RSOC entry for WRITE(16), the CDB USAGE DATA field (a bit mask) indicates the GROUP NUMBER field is supported
Quoted 9 lines
That check can be done once, at disk attachment time where there is already code to fetch RSOC.
Is there a
bi_read_hint? If not then thebi_write_hintshould also be applied to READ(16). Makes that variable naming look pretty silly though.Doug Gilbert
SBC-5 says that support for the grouping function is indicated by the GROUP_SUP bit in the Extended Inquiry VPD page (86h). I'm not sure how many devices actually support that page though. Probably most don't.
What devices actually DO support the grouping and do something with it?
We'd probably need a blacklist flag to turn this off and/or some code in the error path to discontinue setting the field if the device returns INVALID FIELD IN CDB or something, like we do for disabling discard commands if they don't actually work in sd_done().
-Ewan
Ewan,
Quoted 4 lines
SBC-5 says that support for the grouping function is indicated by the
GROUP_SUPbit in the Extended Inquiry VPD page (86h). I'm not sure how many devices actually support that page though. Probably most don't.
Several devices support it, albeit for various different purposes. It's one of these wonderful features whose interpretation was left outside the scope of the spec for a long time.
So even though we absolutely and positively need to make setting GROUP NUMBER conditional on GROUP_SUP being reported, we also need additional information from the storage about how the field should be interpreted.
The official way to report hinting is for the device to implement the IO Advice Hints Grouping mode page. I wrote some code to support that but no vendors that I know of ended up actually shipping an implementation. A few implemented my older I/O class proposal but didn't ship that either despite really convincing performance results.
If Randall has access to a device which implements hinting, I'd love to know more.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 16 lines · Martin K. Petersen
Ewan,
Quoted 4 lines
SBC-5 says that support for the grouping function is indicated by the
GROUP_SUPbit in the Extended Inquiry VPD page (86h). I'm not sure how many devices actually support that page though. Probably most don't.Several devices support it, albeit for various different purposes. It's one of these wonderful features whose interpretation was left outside the scope of the spec for a long time.
So even though we absolutely and positively need to make setting GROUP NUMBER conditional on
GROUP_SUPbeing reported, we also need additional information from the storage about how the field should be interpreted.
Quoted 8 lines
The official way to report hinting is for the device to implement the IO Advice Hints Grouping mode page. I wrote some code to support that but no vendors that I know of ended up actually shipping an implementation. A few implemented my older I/O class proposal but didn't ship that either despite really convincing performance results.
If Randall has access to a device which implements hinting, I'd love to know more.
I am working on Android phone.
The idea is to enable write hint for Turbo write UFS feature. Turbo write feature in UFS 3.x is under discussion in JEDEC JC-64. This patch is the under-lying framework for supporting this feature.
Quoted 3 lines
Signature · 2 lines
-- Martin K. Petersen Oracle Linux Engineering
Hi Randall,
Quoted 5 lines
I am working on Android phone.
The idea is to enable write hint for Turbo write UFS feature. Turbo write feature in UFS 3.x is under discussion in JEDEC JC-64. This patch is the under-lying framework for supporting this feature.
OK, but we can't blindly go setting GROUP NUMBER to a non-zero value. That'll break a massive amount of devices which will fail READ/WRITE commands with INVALID FIELD IN CDB.
So aside from requiring the device to report GROUP_SUP=1, we'll need some sort of indication that this device supports the UFS Turbo Write feature. If you are engaged with JEDEC on this, please tell them we'll need a VPD page, a mode page, or something similar to use as trigger to entertain enabling this feature.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Hi Randall,
Quoted 23 lines · Randall Huang
Quoted 22 lines · Martin K. Petersen
Ewan,
Quoted 4 lines
SBC-5 says that support for the grouping function is indicated by the
GROUP_SUPbit in the Extended Inquiry VPD page (86h). I'm not sure how many devices actually support that page though. Probably most don't.Several devices support it, albeit for various different purposes. It's one of these wonderful features whose interpretation was left outside the scope of the spec for a long time.
So even though we absolutely and positively need to make setting GROUP NUMBER conditional on
GROUP_SUPbeing reported, we also need additional information from the storage about how the field should be interpreted.The official way to report hinting is for the device to implement the IO Advice Hints Grouping mode page. I wrote some code to support that but no vendors that I know of ended up actually shipping an implementation. A few implemented my older I/O class proposal but didn't ship that either despite really convincing performance results.
Quoted 6 lines
Quoted 2 lines
If Randall has access to a device which implements hinting, I'd love to know more.
I am working on Android phone.
The idea is to enable write hint for Turbo write UFS feature. Turbo write feature in UFS 3.x is under discussion in JEDEC JC-64. This patch is the under-lying framework for supporting this feature.
As far as I know, there are more than one Turbo Write feature proposals in JEDEC, which are currently under discussion. For now, not all proposals are using CDB bytes for enabling TurboWrite.
So, maybe making this change in SCSI is still too early.
Quoted 3 lines
Quoted 3 lines
Signature · 2 lines
-- Martin K. Petersen Oracle Linux Engineering