[PATCH] firewire: sbp2: allow WRITE SAME and REPORT SUPPORTED OPERATION CODES

[PATCH] firewire: sbp2: allow WRITE SAME and REPORT SUPPORTED OPERATION CODES

[Stefan Richter]

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.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/sbp2.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index bb1b392..1162d6b 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -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;
-- 
1.7.12



-- 
Stefan Richter
-=====-===-- =-== ==--=
http://arcgraph.de/sr/

Re: [PATCH] firewire: sbp2: allow WRITE SAME and REPORT SUPPORTED OPERATION CODES

[Martin K. Petersen]

>>>>> "Stefan" == Stefan Richter <stefanr@s5r6.in-berlin.de> 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 :)

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] firewire: sbp2: allow WRITE SAME and REPORT SUPPORTED OPERATION CODES

[Stefan Richter]

On Nov 26 Martin K. Petersen wrote:
> >>>>> "Stefan" == Stefan Richter <stefanr@s5r6.in-berlin.de> 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]
> 
> 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.

> 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 :)

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.
-- 
Stefan Richter
-=====-===-- =-== ==-==
http://arcgraph.de/sr/

Re: [PATCH] firewire: sbp2: allow WRITE SAME and REPORT SUPPORTED OPERATION CODES

[Stefan Richter]

On Dec 02 2012 Stefan Richter wrote:
> On Nov 26 Martin K. Petersen wrote:
> > >>>>> "Stefan" == Stefan Richter <stefanr@s5r6.in-berlin.de> 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]
> > 
> > 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.
> 
> > 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 :)
> 
> 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.)

> 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.)
-- 
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 v3.13-rc3] firewire: sbp2: bring back WRITE SAME support

[Stefan Richter]

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.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: stable@kernel.org
---
 drivers/firewire/sbp2.c | 1 -
 1 file changed, 1 deletion(-)

--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -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 <krh@bitplanet.net>");


-- 
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] firewire: sbp2: allow WRITE SAME and REPORT SUPPORTED OPERATION CODES

[Douglas Gilbert]

On 13-12-15 09:52 AM, Stefan Richter wrote:
> On Dec 02 2012 Stefan Richter wrote:
>> On Nov 26 Martin K. Petersen wrote:
>>>>>>>> "Stefan" == Stefan Richter <stefanr@s5r6.in-berlin.de> 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]
>>>
>>> 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.
>>
>>> 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 :)
>>
>> 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.)
>
>> 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

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

> 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.)
>

Re: [PATCH v3.13-rc3] firewire: sbp2: bring back WRITE SAME support

[Martin K. Petersen]

>>>>> "Stefan" == Stefan Richter <stefanr@s5r6.in-berlin.de> 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...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] firewire: sbp2: allow WRITE SAME and REPORT SUPPORTED OPERATION CODES

[Stefan Richter]

On Dec 16 Douglas Gilbert wrote:
> On 13-12-15 09:52 AM, Stefan Richter wrote:
> > 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.
> >
> > $ 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.

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.
-- 
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