On 12/10/19 3:29 PM, Martin Wilck wrote:
> On Tue, 2019-12-10 at 12:57 -0500, Bart Van Assche wrote:
>> On 12/10/19 6:47 AM, Martin Wilck wrote:
>>> blk_mq_request_started() returns true for requests in
>>> MQ_RQ_COMPLETE
>>> state. Is this really an equivalent condition?
>>>
>>> That said, the condition in the current code is sort of strange, as
>>> it's equivalent to !(sp->completed && sp->aborted). I'm wondering
>>> what
>>> it means if a command is both completed and aborted. Naïvely
>>> thinking
>>> (and inferring from the current code) this condition could never be
>>> met, and thus its negation would hold for every command. Perhaps
>>> Quinn
>>> meant "!(sp->completed || sp->aborted)" ?
>>
>> Hi Martin,
>>
>> The only caller of qla2x00_abort_srb() is qla2x00_abort_all_cmds().
>> That
>> function should only be called after completion interrupts have been
>> disabled. In other words, I don't think that we have to worry about
>> blk_mq_request_started() encountering the MQ_RQ_COMPLETE state. No
>> request should have that state when qla2x00_abort_all_cmds() is
>> called.
>
> I thought avoiding a race between completion and abort was the whole
> point of f45bca8c5052 ("scsi: qla2xxx: Fix double scsi_done for abort
> path"), which introduced the code that you're now changing. But I must
> be overlooking something then, as Himanshu has acked this.
Hi Martin,
My understanding of commit f45bca8c5052 ("scsi: qla2xxx: Fix double
scsi_done for abort path") is as follows:
* A long time ago a scsi_done() call was introduced in
qla2xxx_eh_abort(). Maybe this commit introduced that call: 083a469db4ec
("[SCSI] qla2xxx: Correct use-after-free oops seen during EH-abort.").
* Calling scsi_done() from qla2xxx_eh_abort() is only fine if the
firmware does not report a completion after having processed an abort
request. My conclusion from commit f45bca8c5052 is that the firmware
does report a completion after having processed an abort request. Hence
the removal of that scsi_done call from qla2xxx_eh_abort().
Bart.
