Hi,
On 08/28/2014 02:10 PM, Hannes Reinecke wrote:
> On 08/26/2014 09:19 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 08/26/2014 08:34 PM, James Bottomley wrote:
>>> On Tue, 2014-08-26 at 10:13 +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 08/25/2014 05:41 PM, James Bottomley wrote:
>>>>> On Mon, 2014-08-25 at 13:39 +0200, Paolo Bonzini wrote:
>>>>>> Il 25/08/2014 13:26, Hans de Goede ha scritto:
>>>>>>> Thanks Bart and Paolo, your insights into this are greatly
>>>>>>> appreciated.
>>>>>>>
>>>>>>> So with uas there are separate usb transaction for cmd, data
>>>>>>> in, data out
>>>>>>> and sense for each tag. At the time of abort, usually one of
>>>>>>> data in / data
>>>>>>> out and a sense usb transaction will be outstanding.
>>>>>>>
>>>>>>> There already is logic in the driver to kill the data in / out
>>>>>>> transactions
>>>>>>> if a sense gets returned (usually with an error) before they
>>>>>>> are done.
>>>>>>>
>>>>>>> So if I'm reading this correctly, then on a successful abort,
>>>>>>> the sense
>>>>>>> transaction (if not already completed by the target) should be
>>>>>>> cancelled as
>>>>>>> it will never complete, correct ?
>>>>>>
>>>>>> Yes. More precisely, once the response IU comes back for the
>>>>>> abort,
>>>>>> there should be no more IUs for that task. Figure 13
>>>>>> ("Multiple Command
>>>>>> Example") in the UAS spec actually shows that.
>>>>>>
>>>>>> At least, that's what it should be like in theory. I suspect
>>>>>> firmware
>>>>>> bugs will abound in this area, but at least you shouldn't be
>>>>>> actively
>>>>>> expecting an IU for an aborted task.
>>>>>
>>>>> Just a note on this. The abort area in all devices is where we
>>>>> have
>>>>> scope for spec compliance issues. Also in the old days abort
>>>>> itself
>>>>> could trigger a firmware crash on some devices (including
>>>>> arrays). The
>>>>> problem is actually that the system is usually groaning because
>>>>> it's out
>>>>> of memory within the controller. That actually means that
>>>>> sending in
>>>>> another task (the abort) causes greater pressure. For this
>>>>> reason, some
>>>>> device drivers choose to skip the abort step and head straight
>>>>> to reset.
>>>>> For reset, you guarantee that all outstanding tasks for the unit
>>>>> are
>>>>> killed.
>>>>
>>>> Hmm, I like this idea, given the finickiness of the abort path in
>>>> the uas
>>>> driver, and that:
>>>>
>>>> 1) We really have no proper way to test this
>>>> 2) We already have some known issues there (we don't kill sense
>>>> urbs atm,
>>>> and I've a note somewhere about a double free on some corner
>>>> case
>>>> where an urb submit fails)
>>>> 3)
>>>> 3) In all the cases where I've managed to trip op an uas device
>>>> the only
>>>> thing which actually worked to recover things was doing a usb
>>>> reset
>>>> 4) Aborts should not happen in the first place, so using a big
>>>> hammer
>>>> when they do is not really a big problem, instead we should
>>>> focus
>>>> on figuring out why they happen and fix the cause
>>>>
>>>> I think that just dropping abort handling altogether is a good
>>>> idea actually,
>>>> so if there are no objections I'm just going to do that.
>>>>
>>>> I can simply not set eh_abort_handler (aka set it to NULL), right ?
>>>
>>> Just so you know, if you do this, error handling becomes much more
>>> painful. The abort handler can now handle aborting and retrying
>>> without
>>> pausing the host. The reset definitely stops the host and causes
>>> a big
>>> and noticeable burp in processing. However, there are hosts which
>>> have
>>> to do it.
>>
>> I understand, but shouldn't aborts be something which rarely happens.
>> I guess that with a faulty drive they happen more often, but at this
>> point in time the uas driver's error handling problems are mostly with
>> dealing with timeouts during probing, which are likely caused by
>> the device not grokking some command we've send, and responding to
>> this in a bad manner (hanging mostly).
>>
>> So I'm tempted to just remove the abort handling, and first focus on
>> getting uas stable under all conditions. Once it is stable we can
>> look into making it deal more graciously with errors.
>>
> Sounds like a reasonable plan.
>
> [ .. ]
>>>
>>> Wait, could this also be your abort problem? In the running abort
>>> handler, we could get a scenario where we abort a bunch of
>>> commands at the same time.
>>
>> I don't think so, the uas code does not return from eh_abort_handler
>> until the abort has either succeeded or timedout (for which a 3 second
>> timeout is used). AFAIK the scsi core will always issue multiple aborts
>> sequentially, right. And if the abort succeeds then the tag is free
>> again for the next abort. Only if an abort fails (times out, which is
>> what I've seen in all the error conditions which I've encountered so
>> far),
>> then will subsequent aborts, and the logical unit reset, fail due to
>> there not being a free tag to use for them.
>>
> If the logic for command/task abort and logical unit reset is similar then there is no point in implementing both.
The logic is different in that when an abort gets issued other commands may
(on paper) still complete normally, where as with a lun reset all commands
on that lun (and usually there is only one lun) are toast.
> So by what I've seen I would first implement target reset (and host reset :-)
With uas target == host, and that is already implemented and so far seems
to be the only thing which actually works (if bugs in eh_abort don't cause
an oops first).
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
