Re: [BUG][bisected 270065e] linux-next fails to boot on powerpc

[Brian King]

On 08/18/2017 04:41 PM, Bart Van Assche wrote:
> On Fri, 2017-08-18 at 16:04 -0500, Brian King wrote:
>> I think I have an understanding what is going on and why Bart's patch is causing problems for ipr.
>> I can work around the boot hang in ipr, but ultimately I think we need to figure out a fix
>> in scsi / block. I added some tracing and confirmed its not a matter of commands getting stuck
>> in ipr. The issue is we are retrying failed commands until we finally run out of time. This is
>> what I see:
>>
>> 1. sd_revalidate_disk calls scsi_report_opcode
>> 2. ipr RAID arrays don't support MAINTENANCE_IN / MI_REPORT_SUPPORTED_OPERATION_CODES
>> 3. ipr returns the command with DID_ERROR
>> 4. scsi_decide_disposition goes to maybe_retry, increments scmd->retries, and returns NEEDS_RETRY
>> 5. scsi_softirq_done calls scsi_queue_insert to requeue the command, which calls scsi_mq_requeue_cmd
>> 6. With Bart's change, we then clear RQF_DONTPREP in this path, while prior we did not
>> 7. This results in the command getting scmd->retries zeroed out when it gets re-queued,
>>    since we go through prep again and we lose our retry counter, resulting in lots and lots of retries.
>> 8. Since the default command timeout for an ipr RAID array is 120 seconds, these retries go on for
>>    quite a long time...
>> 9. Finally, the command has been retried so long we trip over the overall retry timer
>>    in scsi_softirq_done and we timeout the command.
>>
>> I'll follow up with a patch to ipr to workaround the hang, but I think we need to somehow preserve
>> the retry counter in the scsi command, as this will likely cause issues with other drivers. 
> 
> Hello Brian,
> 
> Thanks for the detailed analysis. This is very helpful. Have you considered
> to change the ipr driver such that it terminates REPORT SUPPORTED OPERATION
> CODES commands with the appropriate check condition code instead of DID_ERROR?

Yes. That data is actually in the sense buffer, but since I'm also setting DID_ERROR,
scsi_decide_disposition isn't using it. I've got a patch to do just as you suggest,
to stop setting DID_ERROR when there is more detailed error data available, 
but it will need some additional testing before I submit, as it will impact much
more than just this case. 

To add to my analysis above, #9 should not be there... It looks like
jiffies_at_alloc would also be getting reinitialized in this case, resulting in
a perpetual retry, which is what I was seeing.

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center