firewire: sbp2: allow WRITE SAME and REPORT SUPPORTED OPERATION CODES
PATCH 1/1 firewire: sbp2: allow WRITE SAME and REPORT SUPPORTED OPERATION CODES
The commits
3c6bdaeab4fd "[SCSI] Add a report opcode helper" 5db44863b6eb "[SCSI] sd: Implement support for WRITE SAME"
introduced in-kernel uses of the mentioned commands but cautiously blacklisted for any IEEE 1394 (SBP-2/3) targets and some other transports.
I looked through a range of SBP devices and found that the blacklist flags can be removed:
The kernel never attempts these commands if the device's INQUIRY data claim a SCSI revision of less than 0x05. This is the case with all SBP devices that I checked, except for two devices (both based on the OXUF936QSE chip but having different firmwares) which claimed a revision of 0x05 i.e. conformance with SPC-3.
I tried "sg_opcodes" from sg3_utils on these two devices and several of the other ones and did not encounter any apparent firmware bug with it. (All of them returned Illegal Request/ Invalid command operation code and carried on.)
I did not try "sg_write_same" on any of the devices; but since the two SPC-3 devices are correctly identified as "fully provisioned", won't issue WRITE SAME to them either.
Hence let's remove the no_report_opcodes and no_write_same blacklist flags so that these commands can be used on respectively capable targets. I guess the Linux sbp-target could be such a target.
--- drivers/firewire/sbp2.c | 2 -- 1 file changed, 2 deletions(-)
drivers/firewire/sbp2.c +0 -2
@@ -1546,8 +1546,6 @@ static int sbp2_scsi_slave_configure(struct scsi_device *sdev)struct sbp2_logical_unit *lu = sdev->hostdata; sdev->use_10_for_rw =1 ; - sdev->no_report_opcodes =1 ; - sdev->no_write_same =1 ;if (sbp2_param_exclusive_login) sdev->manage_start_stop =1 ;
Signature · 8 lines
1.7.12 -- Stefan Richter -=====-===-- =-== ==--= http://arcgraph.de/sr/
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Stefan" == Stefan Richter <[email protected]> writes:
Stefan,
Stefan> I did not try "sg_write_same" on any of the devices; but since Stefan> the two SPC-3 devices are correctly identified as "fully Stefan> provisioned", won't issue WRITE SAME to them either.
What if you have an SSD behind one of them?
Stefan> Hence let's remove the no_report_opcodes and no_write_same Stefan> blacklist flags so that these commands can be used on Stefan> respectively capable targets.
I just erred on the side of caution. If you are happy without belt and suspenders that's perfectly ok with me :)
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 4 lines · On Nov 26 Martin K. Petersen wrote:
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Stefan" == Stefan Richter <[email protected]> writes:
Stefan> I did not try "
sg_write_same" on any of the devices; but since Stefan> the two SPC-3 devices are correctly identified as "fully Stefan> provisioned", won't issue WRITE SAME to them either.
^[the kernel]
Quoted 2 lines
What if you have an SSD behind one of them?
At the moment I only have a single old SSD available which does not implement ATA TRIM as far as I recall.
And the two mentioned OXUF936QSE based SPC-3 devices are four-bay SATA disk enclosures whose firmwares only support various RAID modes and require at least two bays to be populated. I.e. I can't test them with the SSD for now. But I suspect that they don't implement thin provisioning anyway, particularly translation of WRITE SAME with UNMAP to ATA TRIM.
But now I found another SPC-3 compliant device in my stash; a dual SATA bridge based on OXUF934DSB which supports JBOD with 1...2 disks alternatively to striping/ spanning/ mirroring over 2 disks. I attached the old SSD to it, and its thin_provisioning sysfs attribute was shown as 0 as well. "sg_write_same -U ..." on this device in the 10 and 16 byte variants ended with Illegal Request/ Invalid command operation code, but otherwise without discernible malfunction.
Quoted 6 lines
Stefan> Hence let's remove the
no_report_opcodesandno_write_sameStefan> blacklist flags so that these commands can be used on Stefan> respectively capable targets.I just erred on the side of caution. If you are happy without belt and suspenders that's perfectly ok with me :)
Blacklisting at first was definitely the right approach. But now that I looked at a variety of older and newer devices, I am confident that the general Inquiry_Data.Version >= SPC-3 test keeps the wackier among the SBP-2 devices safe enough.
Of course it remains to be seen what happens with ATA TRIM enabled SSDs behind the newer SPC-3 compliant bridges, but at this time the risk with those seems low.
Signature · 3 lines
Stefan Richter -=====-===-- =-== ==-== http://arcgraph.de/sr/
Quoted 30 lines · On Dec 02 2012 Stefan Richter wrote:
Quoted 4 lines · On Nov 26 Martin K. Petersen wrote:
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Stefan" == Stefan Richter <[email protected]> writes:
Stefan> I did not try "
sg_write_same" on any of the devices; but since Stefan> the two SPC-3 devices are correctly identified as "fully Stefan> provisioned", won't issue WRITE SAME to them either.^[the kernel]
Quoted 2 lines
What if you have an SSD behind one of them?
At the moment I only have a single old SSD available which does not implement ATA TRIM as far as I recall.
And the two mentioned OXUF936QSE based SPC-3 devices are four-bay SATA disk enclosures whose firmwares only support various RAID modes and require at least two bays to be populated. I.e. I can't test them with the SSD for now. But I suspect that they don't implement thin provisioning anyway, particularly translation of WRITE SAME with UNMAP to ATA TRIM.
But now I found another SPC-3 compliant device in my stash; a dual SATA bridge based on OXUF934DSB which supports JBOD with 1...2 disks alternatively to striping/ spanning/ mirroring over 2 disks. I attached the old SSD to it, and its
thin_provisioningsysfs attribute was shown as 0 as well. "sg_write_same-U ..." on this device in the 10 and 16 byte variants ended with Illegal Request/ Invalid command operation code, but otherwise without discernible malfunction.Quoted 4 lines
Stefan> Hence let's remove the
no_report_opcodesandno_write_sameStefan> blacklist flags so that these commands can be used on Stefan> respectively capable targets.
Quoted 7 lines
Quoted 2 lines
I just erred on the side of caution. If you are happy without belt and suspenders that's perfectly ok with me :)
Blacklisting at first was definitely the right approach. But now that I looked at a variety of older and newer devices, I am confident that the general Inquiry_Data.Version >= SPC-3 test keeps the wackier among the SBP-2 devices safe enough.
(I followed up with https://git.kernel.org/linus/b0ea5f19d3d8.)
Quoted 3 lines
Of course it remains to be seen what happens with ATA TRIM enabled SSDs behind the newer SPC-3 compliant bridges, but at this time the risk with those seems low.
I now tested
- ONNTO dataTale RSM4QO (OXUF936QSE) with two Samsung 840 Pro in RAID 0,
- ONNTO dataTale RSM4QO (OXUF936QSE) with two Samsung 840 Pro in RAID 1,
- IOI FWBU2-DSATA12 (OXUF934DSB) with one Samsung 840 Pro
and kernel 3.9.
$ grep . /sys/class/scsi_disk/42\:0\:0\:0/*prov* /sys/class/scsi_disk/42:0:0:0/provisioning_mode:full /sys/class/scsi_disk/42:0:0:0/thin_provisioning:0 # sg_opcodes /dev/sdg Ext Hard Disk Peripheral device type: disk Report supported operation codes: operation not supported # sg_write_same --10 --lba=1 /dev/sdg Write same(10) command not supported # sg_write_same --16 --lba=1 /dev/sdg Write same(16) command not supported # sg_write_same --32 --lba=1 /dev/sdg Write same: pass through os error: Invalid argument Write same(32) command failed
I will send a patch which reverts the drivers/firewire/sbp2.c hunk of
https://git.kernel.org/linus/54b2b50c20a6 "[SCSI] Disable WRITE SAME for RAID and virtual host adapter drivers". (As an aside, sbp2.c implements a transport, not a virtual host adapter.)
Signature · 10 lines
Stefan Richter -=====-===-= ==-- -==== http://arcgraph.de/sr/ ------------------------------------------------------------------------------ Rapidly troubleshoot problems before they affect your business. Most IT organizations don't have a clear picture of how application performance affects their revenue. With AppDynamics, you get 100% visibility into your Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro! http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
PATCH 1/1 firewire: sbp2: bring back WRITE SAME support
Commit 54b2b50c20a6 "[SCSI] Disable WRITE SAME for RAID and virtual host adapter drivers" disabled WRITE SAME support for all SBP-2 attached targets. But as described in the changelog of commit b0ea5f19d3d8 "firewire: sbp2: allow WRITE SAME and REPORT SUPPORTED OPERATION CODES", it is not required to blacklist WRITE SAME.
Bring the feature back by reverting the sbp2.c hunk of commit 54b2b50c20a6.
--- drivers/firewire/sbp2.c | 1 - 1 file changed, 1 deletion(-)
drivers/firewire/sbp2.c +0 -1
@@ -1623,7 +1623,6 @@ static struct scsi_host_template scsi_driver_template = { .cmd_per_lun = 1, .can_queue = 1, .sdev_attrs = sbp2_scsi_sysfs_attrs, - .no_write_same = 1, };MODULE_AUTHOR ("Kristian Hoegsberg <[email protected]>" );
Signature · 10 lines
Stefan Richter -=====-===-= ==-- -==== http://arcgraph.de/sr/ ------------------------------------------------------------------------------ Rapidly troubleshoot problems before they affect your business. Most IT organizations don't have a clear picture of how application performance affects their revenue. With AppDynamics, you get 100% visibility into your Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro! http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
Re: [PATCH v3.13-rc3] firewire: sbp2: bring back WRITE SAME support
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Stefan" == Stefan Richter <[email protected]> writes:
Stefan> Commit 54b2b50c20a6 "[SCSI] Disable WRITE SAME for RAID and Stefan> virtual host adapter drivers" disabled WRITE SAME support for Stefan> all SBP-2 attached targets. But as described in the changelog Stefan> of commit b0ea5f19d3d8 "firewire: sbp2: allow WRITE SAME and Stefan> REPORT SUPPORTED OPERATION CODES", it is not required to Stefan> blacklist WRITE SAME.
Stefan> Bring the feature back by reverting the sbp2.c hunk of commit Stefan> 54b2b50c20a6.
That's entirely your call and perfectly fine by me.
I just get a lot of complaints about "WRITE SAME failed. Manually zeroing". But I guess I can only blame myself for not making it clear that that's a warning and not an error...
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 51 lines · Stefan Richter
Quoted 30 lines · On Dec 02 2012 Stefan Richter wrote:
Quoted 4 lines · On Nov 26 Martin K. Petersen wrote:
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Stefan" == Stefan Richter <[email protected]> writes:
Stefan> I did not try "
sg_write_same" on any of the devices; but since Stefan> the two SPC-3 devices are correctly identified as "fully Stefan> provisioned", won't issue WRITE SAME to them either.^[the kernel]
Quoted 2 lines
What if you have an SSD behind one of them?
At the moment I only have a single old SSD available which does not implement ATA TRIM as far as I recall.
And the two mentioned OXUF936QSE based SPC-3 devices are four-bay SATA disk enclosures whose firmwares only support various RAID modes and require at least two bays to be populated. I.e. I can't test them with the SSD for now. But I suspect that they don't implement thin provisioning anyway, particularly translation of WRITE SAME with UNMAP to ATA TRIM.
But now I found another SPC-3 compliant device in my stash; a dual SATA bridge based on OXUF934DSB which supports JBOD with 1...2 disks alternatively to striping/ spanning/ mirroring over 2 disks. I attached the old SSD to it, and its
thin_provisioningsysfs attribute was shown as 0 as well. "sg_write_same-U ..." on this device in the 10 and 16 byte variants ended with Illegal Request/ Invalid command operation code, but otherwise without discernible malfunction.Quoted 4 lines
Stefan> Hence let's remove the
no_report_opcodesandno_write_sameStefan> blacklist flags so that these commands can be used on Stefan> respectively capable targets.Quoted 7 lines
Quoted 2 lines
I just erred on the side of caution. If you are happy without belt and suspenders that's perfectly ok with me :)
Blacklisting at first was definitely the right approach. But now that I looked at a variety of older and newer devices, I am confident that the general Inquiry_Data.Version >= SPC-3 test keeps the wackier among the SBP-2 devices safe enough.
(I followed up with https://git.kernel.org/linus/b0ea5f19d3d8.)
Quoted 3 lines
Of course it remains to be seen what happens with ATA TRIM enabled SSDs behind the newer SPC-3 compliant bridges, but at this time the risk with those seems low.
I now tested
- ONNTO dataTale RSM4QO (OXUF936QSE) with two Samsung 840 Pro in RAID 0,
- ONNTO dataTale RSM4QO (OXUF936QSE) with two Samsung 840 Pro in RAID 1,
- IOI FWBU2-DSATA12 (OXUF934DSB) with one Samsung 840 Pro
and kernel 3.9.
Quoted 14 lines
$ grep . /sys/class/scsi_disk/42\:0\:0\:0/*prov* /sys/class/scsi_disk/42:0:0:0/provisioning_mode:full /sys/class/scsi_disk/42:0:0:0/thin_provisioning:0 # sg_opcodes /dev/sdg Ext Hard Disk Peripheral device type: disk Report supported operation codes: operation not supported # sg_write_same --10 --lba=1 /dev/sdg Write same(10) command not supported # sg_write_same --16 --lba=1 /dev/sdg Write same(16) command not supported # sg_write_same --32 --lba=1 /dev/sdg Write same: pass through os error: Invalid argument Write same(32) command failed
The sg driver does not support SCSI commands of greater than 16 bytes. That is the reason for the "pass-through os error" on your '--32' variant above. For now you need to send the '--32' variant to the bsg driver instead.
I have presented a patch to relax the 16 byte cdb limit on the sg driver:
http://www.spinics.net/lists/linux-scsi/msg70283.html
Like most of my patches that one seems to be residing in James' /dev/null file for later consideration.
Doug Gilbert
Quoted 5 lines
I will send a patch which reverts the
drivers/firewire/sbp2.chunk of
https://git.kernel.org/linus/54b2b50c20a6 "[SCSI] Disable WRITE SAME for RAID and virtual host adapter drivers". (As an aside,sbp2.cimplements a transport, not a virtual host adapter.)
Quoted 32 lines · On Dec 16 Douglas Gilbert wrote:
Quoted 11 lines · Stefan Richter
Quoted 3 lines · On Dec 02 2012 Stefan Richter wrote:
Of course it remains to be seen what happens with ATA TRIM enabled SSDs behind the newer SPC-3 compliant bridges, but at this time the risk with those seems low.
I now tested
- ONNTO dataTale RSM4QO (OXUF936QSE) with two Samsung 840 Pro in RAID 0,
- ONNTO dataTale RSM4QO (OXUF936QSE) with two Samsung 840 Pro in RAID 1,
- IOI FWBU2-DSATA12 (OXUF934DSB) with one Samsung 840 Pro
and kernel 3.9.Quoted 14 lines
$ grep . /sys/class/scsi_disk/42\:0\:0\:0/*prov* /sys/class/scsi_disk/42:0:0:0/provisioning_mode:full /sys/class/scsi_disk/42:0:0:0/thin_provisioning:0 # sg_opcodes /dev/sdg Ext Hard Disk Peripheral device type: disk Report supported operation codes: operation not supported # sg_write_same --10 --lba=1 /dev/sdg Write same(10) command not supported # sg_write_same --16 --lba=1 /dev/sdg Write same(16) command not supported # sg_write_same --32 --lba=1 /dev/sdg Write same: pass through os error: Invalid argument Write same(32) command failedThe sg driver does not support SCSI commands of greater than 16 bytes. That is the reason for the "pass-through os error" on your '--32' variant above. For now you need to send the '--32' variant to the bsg driver instead.
Quoted 5 lines
I have presented a patch to relax the 16 byte cdb limit on the sg driver:
http://www.spinics.net/lists/linux-scsi/msg70283.htmlLike most of my patches that one seems to be residing in James'
/dev/nullfile for later consideration.
Ah, thanks for the pointer. Plus, I now looked back into the code and saw that while SBP-2 (the encapsulation protocol) does not impose any limit on CDB length, firewire-sbp2 (the driver) does. This implementation limit was last changed by https://git.kernel.org/linus/af2719415a5c. It could be lifted though, should a need arise.
# sg_write_same --16 --lba=1 /dev/bsg/79\:0\:0\:0 Write same(16) command not supported # sg_write_same --32 --lba=1 /dev/bsg/79\:0\:0\:0 Write same: transport: Host_status=0x05 [DID_ABORT] Driver_status=0x00 [DRIVER_OK, SUGGEST_OK]
Write same(32) command failed
I only tested the OXUF934DSB this time. Like in the previous tests, the target firmware keeps working properly after it rejected WRITE SAME 10 or 16 without or with UNMAP bit.
PS:
For those who wonder about older and buggier SBP-2 targets than those which I tested here: Due to the check for sdev->scsi_level of at least SCSI_SPC_3 in drivers/scsi/scsi.c::scsi_report_opcode(), and subsequent use of this function by sd.c to determine support of WRITE SAME 10 and/or 16, the kernel will never send these opcodes to those targets unless userland explicitly tells it to do so. Those kinds of SBP-2 targets all show themselves with a lower scsi_level.
PPS:
Reminder to self: Apropos commit af2719415a5c, I need to fix up struct sbp2_command_orb for better portability of dma_map_single(..., &orb->request, ...) and to obey the target's Unit_Characteristics.ORB_size.
Signature · 10 lines
Stefan Richter -=====-===-= ==-- =-==- http://arcgraph.de/sr/ ------------------------------------------------------------------------------ Rapidly troubleshoot problems before they affect your business. Most IT organizations don't have a clear picture of how application performance affects their revenue. With AppDynamics, you get 100% visibility into your Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro! http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk