Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[James Bottomley]

On Mon, 2012-07-02 at 11:52 -0700, Linus Torvalds wrote:
> On Mon, Jul 2, 2012 at 12:10 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > So since 3.2 already tries READ_CAPACITY(10) first, this patch is
> > not needed. It does not harm either, and if the scsi level reporting
> > change ever finds its way into 3.2 stable (which it should not
> > IMHO), then this patch will be needed.
> 
> Yeah, it should definitely not make it into stable. In fact, I'd
> rather remove it from mainline too. The whole idea to do
> READ_CAPACITY_16 by default was moronic.
> 
> James? Why does the SCSI subsystem always do these idiotic defaults?
> We *know* that devices are buggy. Sure, USB storage tends to be
> buggier than most, but the whole idea of "let's use the new way of
> reporting that gets almost no testing" is just wrong.

It's not idiotic.  It's, unfortunately, required by the standard to
check for protection capable devices.  It's been this way for three
years or so.  Martin Petersen did extensive testing of devices when we
changed it and doing RC16 first is hedged around by claiming support not
only for SCSI_3 but also for SBC_2 in your INQUIRY data.

The problem seems to be that when all the testing was done, USB was
still always returning SCSI_2 in the inquiry data, but this got changed
to allow pass through of the actual vendor information.

> I don't think the USB layer should have needed to work around this. I
> don't think there should be a "try_10b" flag at all. I think the SCSI
> code should just *never* do the 16b version unless it has done the 10b
> version first and determined that the size doesn't fit.

If we did that, we'd regress on all the enterprise class drives with DIF
and DIX.

> And the code already had that logic got the read_10b case. The whole
> logic to do 16b by default was crap. It *used* to at least only do it
> for disks that were marked as having "protection", now it wants to do
> it for anything that should be able to handle it. Which is bogus. The
> whole "let's assume devices get it right" mindset is broken.
> 
> The mindset should be "let's assume devices are buggy, and be careful".

It still is, that's why we did extensive testing three years ago before
changing it.

If you have a solution that doesn't break all the enterprise devices but
works better for USB, I'm listening.  I think the best thing might be to
whitelist the scsi revision pass through in USB, so only the UAS devices
which really, really claim to know what they're doing advertise advanced
features.

James

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[Linus Torvalds]

On Mon, Jul 2, 2012 at 1:39 PM, James Bottomley
<jbottomley@parallels.com> wrote:
>
> It's not idiotic.  It's, unfortunately, required by the standard to
> check for protection capable devices.
 [.. dleted ]
>  Martin Petersen did extensive testing of devices when we
> changed it and doing RC16 first is hedged around by claiming support not
> only for SCSI_3 but also for SBC_2 in your INQUIRY data.

Umm. That just smells like BS to me.

The "extensive testing" part was clearly not true, and it seems that
SCSI people sometimes forget that the biggest user (by *far*) is the
USB storage layer.

Also, your protection claim seems to be invalidated by the actual
code. Yes, it checks if the device claims to support protection. But
it *also* says "let's do that 16b command if "scsi_level >
SCSI_SPC_2". So your claim that it hedges around it by looking at the
inquiry data is pure crap. It's simply not true. Just look at the
code:

        if (sdp->host->max_cmd_len < 16)
                return 0;
        if (sdp->try_rc_10_first)
                return 0;
        if (sdp->scsi_level > SCSI_SPC_2)
                return 1;
        if (scsi_device_protection(sdp))
                return 1;
        return 0;

Notice how for high enough scsi_levels, it never even gets to that
scsi_device_protection() check, which does indeed look into protection
data.

So no, I simply don't see the careful testing or the checking that you
claim exists.

                 Linus

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[Matthew Wilcox]

On Mon, Jul 02, 2012 at 03:23:18PM -0700, Linus Torvalds wrote:
> >  Martin Petersen did extensive testing of devices when we
> > changed it and doing RC16 first is hedged around by claiming support not
> > only for SCSI_3 but also for SBC_2 in your INQUIRY data.
> 
> Umm. That just smells like BS to me.
> 
> The "extensive testing" part was clearly not true, and it seems that
> SCSI people sometimes forget that the biggest user (by *far*) is the
> USB storage layer.

As James said, *at the time*, USB overrode the SBC_2 claims from USB
devices and forced them to be SCSI_2.  So no amount of testing we did
would have uncovered this.

> Also, your protection claim seems to be invalidated by the actual
> code. Yes, it checks if the device claims to support protection. But
> it *also* says "let's do that 16b command if "scsi_level >
> SCSI_SPC_2". So your claim that it hedges around it by looking at the
> inquiry data is pure crap. It's simply not true. Just look at the
> code:

I wrote this code ... James' memory is off.  What happened is that T10
in their infinite wisdom decided to put things like "supports TRIM" and
"is actually a 4k block size but fakes 512 byte blocks" in the Read
Capacity 16 results.  So if we want to support those kinds of things
(and I think we do), then we need to send Read Capacity 16 to devices.

It's not about "enterprise features" at all, but about supporting the
next generation of standard consumer drives.  I'm tempted to say the
USB Storage driver needs to go back to the way things were, because I
don't see any other way to fix this.

I have no idea what Windows is doing to support these features.  That
might be a fruitful course of investigation.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"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."

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[James Bottomley]

On Mon, 2012-07-02 at 18:41 -0600, Matthew Wilcox wrote:
> On Mon, Jul 02, 2012 at 03:23:18PM -0700, Linus Torvalds wrote:
> > >  Martin Petersen did extensive testing of devices when we
> > > changed it and doing RC16 first is hedged around by claiming support not
> > > only for SCSI_3 but also for SBC_2 in your INQUIRY data.
> > 
> > Umm. That just smells like BS to me.
> > 
> > The "extensive testing" part was clearly not true, and it seems that
> > SCSI people sometimes forget that the biggest user (by *far*) is the
> > USB storage layer.
> 
> As James said, *at the time*, USB overrode the SBC_2 claims from USB
> devices and forced them to be SCSI_2.  So no amount of testing we did
> would have uncovered this.
> 
> > Also, your protection claim seems to be invalidated by the actual
> > code. Yes, it checks if the device claims to support protection. But
> > it *also* says "let's do that 16b command if "scsi_level >
> > SCSI_SPC_2". So your claim that it hedges around it by looking at the
> > inquiry data is pure crap. It's simply not true. Just look at the
> > code:
> 
> I wrote this code ... James' memory is off.

Yes, sorry about that ... should have looked in the git log instead of
going by memory at 01:00 after a long flight ...

>   What happened is that T10
> in their infinite wisdom decided to put things like "supports TRIM" and
> "is actually a 4k block size but fakes 512 byte blocks" in the Read
> Capacity 16 results.  So if we want to support those kinds of things
> (and I think we do), then we need to send Read Capacity 16 to devices.
> 
> It's not about "enterprise features" at all, but about supporting the
> next generation of standard consumer drives.  I'm tempted to say the
> USB Storage driver needs to go back to the way things were, because I
> don't see any other way to fix this.

But anyway, we're stuck ... we have to send RC16 first to support these
features.  We did protest to T10 at the time, but to no avail.

> I have no idea what Windows is doing to support these features.  That
> might be a fruitful course of investigation.

Hopefully one of the USB people can do this.

I still think a whitelist of USB devices sending proper SCSI level
information in the inquiry might be the best way forward.

James

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[Alan Stern]

On Tue, 3 Jul 2012, James Bottomley wrote:

> >   What happened is that T10
> > in their infinite wisdom decided to put things like "supports TRIM" and
> > "is actually a 4k block size but fakes 512 byte blocks" in the Read
> > Capacity 16 results.  So if we want to support those kinds of things
> > (and I think we do), then we need to send Read Capacity 16 to devices.
> > 
> > It's not about "enterprise features" at all, but about supporting the
> > next generation of standard consumer drives.  I'm tempted to say the
> > USB Storage driver needs to go back to the way things were, because I
> > don't see any other way to fix this.
> 
> But anyway, we're stuck ... we have to send RC16 first to support these
> features.  We did protest to T10 at the time, but to no avail.

Does it have to be sent _first_?

Or would it be okay to send _both_ commands and believe the RC10
capacity rather than the RC16 capacity if they differ?

> > I have no idea what Windows is doing to support these features.  That
> > might be a fruitful course of investigation.
> 
> Hopefully one of the USB people can do this.
> 
> I still think a whitelist of USB devices sending proper SCSI level
> information in the inquiry might be the best way forward.

I'm doubtful.  It wouldn't be at all surprising for devices to claim 
they support a particular level when in fact they support some but not 
all of the required commands.  Then what do you do?  Put them on the 
whitelist because of the commands they support, or leave them off 
because of the other ones?  What happens later on when you decide to 
use more of the required commands?

Alan Stern

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[Matthew Wilcox]

On Tue, Jul 03, 2012 at 11:49:00AM -0400, Alan Stern wrote:
> On Tue, 3 Jul 2012, James Bottomley wrote:
> 
> > >   What happened is that T10
> > > in their infinite wisdom decided to put things like "supports TRIM" and
> > > "is actually a 4k block size but fakes 512 byte blocks" in the Read
> > > Capacity 16 results.  So if we want to support those kinds of things
> > > (and I think we do), then we need to send Read Capacity 16 to devices.
> > 
> > But anyway, we're stuck ... we have to send RC16 first to support these
> > features.  We did protest to T10 at the time, but to no avail.
> 
> Does it have to be sent _first_?
> 
> Or would it be okay to send _both_ commands and believe the RC10
> capacity rather than the RC16 capacity if they differ?

I have no problem with doing that (and believing RC16 over RC10 if RC10
claims 0xffffffff, naturally).  The problem, as I understand it, is that
some devices crash upon receiving RC16 rather than just returning nonsense.

> > I still think a whitelist of USB devices sending proper SCSI level
> > information in the inquiry might be the best way forward.
> 
> I'm doubtful.  It wouldn't be at all surprising for devices to claim 
> they support a particular level when in fact they support some but not 
> all of the required commands.  Then what do you do?  Put them on the 
> whitelist because of the commands they support, or leave them off 
> because of the other ones?  What happens later on when you decide to 
> use more of the required commands?

SCSI has a fairly extensive collection of black/white list flags already
... see scsi_devinfo.[ch].

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"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."

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[Alan Stern]

On Tue, 3 Jul 2012, Matthew Wilcox wrote:

> On Tue, Jul 03, 2012 at 11:49:00AM -0400, Alan Stern wrote:
> > On Tue, 3 Jul 2012, James Bottomley wrote:
> > 
> > > >   What happened is that T10
> > > > in their infinite wisdom decided to put things like "supports TRIM" and
> > > > "is actually a 4k block size but fakes 512 byte blocks" in the Read
> > > > Capacity 16 results.  So if we want to support those kinds of things
> > > > (and I think we do), then we need to send Read Capacity 16 to devices.
> > > 
> > > But anyway, we're stuck ... we have to send RC16 first to support these
> > > features.  We did protest to T10 at the time, but to no avail.
> > 
> > Does it have to be sent _first_?
> > 
> > Or would it be okay to send _both_ commands and believe the RC10
> > capacity rather than the RC16 capacity if they differ?
> 
> I have no problem with doing that (and believing RC16 over RC10 if RC10
> claims 0xffffffff, naturally).  The problem, as I understand it, is that
> some devices crash upon receiving RC16 rather than just returning nonsense.

No, that's not the problem we're talking about here.  While it is true
that some devices crash upon receiving RC16, so far we haven't been
sending RC16 to those devices, so they don't matter for the purposes of
this discussion.

In fact, usb-storage already has a NO_READ_CAPACITY_16 flag.  It was
introduced because of a card reader that mistakenly attempts to return
an actual capacity value even when no card is present: It takes the
number of sectors -- which is 0 -- and subtracts 1 as usual.  You can
imagine what happens next.  :-)  That's the only device which
currently has this flag.

The reason for the try_rc_10_first flag is that some devices return
bogus data in response to RC16.  Like, an 800 GB device claiming to
have 3 exabytes.

Alan Stern

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[James Bottomley]

On Tue, 2012-07-03 at 15:50 -0400, Alan Stern wrote:
> On Tue, 3 Jul 2012, Matthew Wilcox wrote:
> 
> > On Tue, Jul 03, 2012 at 11:49:00AM -0400, Alan Stern wrote:
> > > On Tue, 3 Jul 2012, James Bottomley wrote:
> > > 
> > > > >   What happened is that T10
> > > > > in their infinite wisdom decided to put things like "supports TRIM" and
> > > > > "is actually a 4k block size but fakes 512 byte blocks" in the Read
> > > > > Capacity 16 results.  So if we want to support those kinds of things
> > > > > (and I think we do), then we need to send Read Capacity 16 to devices.
> > > > 
> > > > But anyway, we're stuck ... we have to send RC16 first to support these
> > > > features.  We did protest to T10 at the time, but to no avail.
> > > 
> > > Does it have to be sent _first_?
> > > 
> > > Or would it be okay to send _both_ commands and believe the RC10
> > > capacity rather than the RC16 capacity if they differ?
> > 
> > I have no problem with doing that (and believing RC16 over RC10 if RC10
> > claims 0xffffffff, naturally).  The problem, as I understand it, is that
> > some devices crash upon receiving RC16 rather than just returning nonsense.
> 
> No, that's not the problem we're talking about here.  While it is true
> that some devices crash upon receiving RC16, so far we haven't been
> sending RC16 to those devices, so they don't matter for the purposes of
> this discussion.
> 
> In fact, usb-storage already has a NO_READ_CAPACITY_16 flag.  It was
> introduced because of a card reader that mistakenly attempts to return
> an actual capacity value even when no card is present: It takes the
> number of sectors -- which is 0 -- and subtracts 1 as usual.  You can
> imagine what happens next.  :-)  That's the only device which
> currently has this flag.
> 
> The reason for the try_rc_10_first flag is that some devices return
> bogus data in response to RC16.  Like, an 800 GB device claiming to
> have 3 exabytes.

So we could easily send both and only believe RC10 if the device is
under 2TB.  However, what about all the extra flags we read out of RC16,
like trim, large sector size and DIF capability?  If the device lies
about its capacity, won't we get bogus values for those as well, which
is going to cause other screw ups?

James

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[Alan Stern]

On Wed, 4 Jul 2012, James Bottomley wrote:

> > The reason for the try_rc_10_first flag is that some devices return
> > bogus data in response to RC16.  Like, an 800 GB device claiming to
> > have 3 exabytes.
> 
> So we could easily send both and only believe RC10 if the device is
> under 2TB.

More precisely, if RC10 indicates the device is under 2 TB then ignore 
the RC16 capacity.  Or maybe issue a warning if it disagrees with the 
RC10 capacity.

>  However, what about all the extra flags we read out of RC16,
> like trim, large sector size and DIF capability?  If the device lies
> about its capacity, won't we get bogus values for those as well, which
> is going to cause other screw ups?

That seems quite likely.  I don't know the answer for any individual 
cases, however.

For the greatest safety, I suppose we could disbelieve all the fields 
in the RC16 response if the RC16 capacity disagrees with the RC10 
capacity and the RC10 capacity is under 2 TB.

Alan Stern

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[Matthew Wilcox]

On Wed, Jul 04, 2012 at 12:07:52AM +0400, James Bottomley wrote:
> > The reason for the try_rc_10_first flag is that some devices return
> > bogus data in response to RC16.  Like, an 800 GB device claiming to
> > have 3 exabytes.
> 
> So we could easily send both and only believe RC10 if the device is
> under 2TB.  However, what about all the extra flags we read out of RC16,
> like trim, large sector size and DIF capability?  If the device lies
> about its capacity, won't we get bogus values for those as well, which
> is going to cause other screw ups?

I think the necessary algorithm is simpler than that:

Send RC10 (unless the device supports PI, in which case it's probably
	enterprisey and well-tested)
Send RC16
If RC10 capacity agrees with RC16 capacity, use extra RC16 data.
(for values of "agrees with" that include the "-1 to use RC16" indicator)

Sure, it's one extra command, but really, who cares?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"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."

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[Alan Stern]

On Tue, 3 Jul 2012, Matthew Wilcox wrote:

> I think the necessary algorithm is simpler than that:
> 
> Send RC10 (unless the device supports PI, in which case it's probably
> 	enterprisey and well-tested)
> Send RC16
> If RC10 capacity agrees with RC16 capacity, use extra RC16 data.
> (for values of "agrees with" that include the "-1 to use RC16" indicator)
> 
> Sure, it's one extra command, but really, who cares?

Well, some USB devices might care.  Right now we don't send RC16 to 
them at all because they get RC10 first and it indicates a capacity 
below 2 TB.

In the end, usb-storage may be forced to use the NO_READ_CAPACITY_16 
flag with all devices except those we can positively identify as a 
USB-(S)ATA bridge.

Alan Stern

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[Matthew Wilcox]

On Thu, Jul 05, 2012 at 05:40:45PM -0400, Alan Stern wrote:
> On Tue, 3 Jul 2012, Matthew Wilcox wrote:
> 
> > I think the necessary algorithm is simpler than that:
> > 
> > Send RC10 (unless the device supports PI, in which case it's probably
> > 	enterprisey and well-tested)
> > Send RC16
> > If RC10 capacity agrees with RC16 capacity, use extra RC16 data.
> > (for values of "agrees with" that include the "-1 to use RC16" indicator)
> > 
> > Sure, it's one extra command, but really, who cares?
> 
> Well, some USB devices might care.  Right now we don't send RC16 to 
> them at all because they get RC10 first and it indicates a capacity 
> below 2 TB.

Mmm.  I think I forgot a step:

Send RC10 (unless the device supports PI, in which case it's probably
	enterprisey and well-tested)
If the device indicates a level < SPC_2, stop here
Send RC16
If RC10 capacity agrees with RC16 capacity, use extra RC16 data.
 (for values of "agrees with" that include the "-1 to use RC16" indicator)

> In the end, usb-storage may be forced to use the NO_READ_CAPACITY_16 
> flag with all devices except those we can positively identify as a 
> USB-(S)ATA bridge.

Is that really an improvement over nobbling the IDENTIFY results to
force SCSI_2?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"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."

Re: [ 38/48] SCSI & usb-storage: add try_rc_10_first flag

[Alan Stern]

On Thu, 5 Jul 2012, Matthew Wilcox wrote:

> > > Sure, it's one extra command, but really, who cares?
> > 
> > Well, some USB devices might care.  Right now we don't send RC16 to 
> > them at all because they get RC10 first and it indicates a capacity 
> > below 2 TB.
> 
> Mmm.  I think I forgot a step:
> 
> Send RC10 (unless the device supports PI, in which case it's probably
> 	enterprisey and well-tested)
> If the device indicates a level < SPC_2, stop here
> Send RC16
> If RC10 capacity agrees with RC16 capacity, use extra RC16 data.
>  (for values of "agrees with" that include the "-1 to use RC16" indicator)

That might or might not work.  I don't know for certain, but it's 
possible that some USB devices do claim a level > SPC_2 and yet can't 
handle RC16.  Of course, we can always set the NO_READ_CAPACITY_16 
flags for these devices as they crop up.

> > In the end, usb-storage may be forced to use the NO_READ_CAPACITY_16 
> > flag with all devices except those we can positively identify as a 
> > USB-(S)ATA bridge.
> 
> Is that really an improvement over nobbling the IDENTIFY results to
> force SCSI_2?

Yes.  There was a good reason for getting rid of that change to the
INQUIRY result (some devices don't like it when the LUN bits are
stuffed into the second byte of each CDB), and I don't want to go back.

Alan Stern