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

[Douglas Gilbert <dgilbert@interlog.com>]

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