Re: [PATCH 3/4] libata: fix handling of race between timeout and completion

[Tejun <htejun@gmail.com>]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> If a qc completes after SCSI timer expires but before libata EH kicks
>> in, the qc gets completed but the scsicmd still gets passed to libata
>> EH resulting in ->eng_timeout invocation with NULL qc.  Currently none
>> of ->eng_timeout callbacks handles this properly.  This patch makes
>> ata_scsi_error() bypass ->eng_timeout and handle this rare case.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> 
> OK in general (I acknowledge the problem you point out), but NAK for 
> this patch.
> 
> 
>> +        scmd = list_entry(host->eh_cmd_q.next,
>> +                  struct scsi_cmnd, eh_entry);
>> +        sb = scmd->sense_buffer;
>> +
>> +        /* Timeout, fake parity for now */
>> +        scmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
>> +        sb[0] = 0x70;
>> +        sb[7] = 0x0a;
>> +        sb[2] = ABORTED_COMMAND;
>> +        sb[12] = 0x47;
>> +        sb[13] = 0x00;
>> +
>> +        printk(KERN_WARNING "ata%u: interrupt and timer raced for "
>> +               "scsicmd %p\n", ap->id, scmd);
>> +
>> +        scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
> 
> 
> OK in general, but I disagree with the handling of the qc==NULL case.
> 
> If you hit the "if scsi timer already fired" shortcut in scsi_done(), 
> that demonstrates clear intent to complete the scsi command.  Thus, when 
> libata EH handling starts, our only task for that scsi command is to 
> complete it.
> 
> Signalling an aborted command stomps all over the current, valid SCSI 
> command results.

Good day, Jeff.

I tried that but the problem is that if scsi timeout expires and then qc 
completes before eh kicks in, we lost some of completion information and 
thus I figured aborting (thus retrying) the commands is the way to go, 
but you're right.  The scsi status and stuff are recorded in scmd and we 
should honor those.  Thanks for pointing out.

> As a side note, this area of code is part of the reason why I was 
> thinking I wanted ...FLAG_EH_TIMEOUT.  My thought was that libata sets 
> that in ->eh_timed_out(). ata_qc_complete() would check that flag, and 
> refuse to call __ata_qc_complete() if it was set.  Doing so causes both 
> the qc and the scsi command to be completed inside the EH handler.  But 
> that's just an off-the-cuff thought...

Hmmm... right.  I'm not very sure about how the synchronization should 
be done, but if it can be done that way, that sounds much better than my 
dangling scmd handling hack.  I'll give it a try.

-- 
tejun