Re: [PATCH] libsas: fix/amend device gone notification in sas_deform_port()

[David Milburn <dmilburn@redhat.com>]

Dan Williams wrote:
> On 2/17/2011 3:36 PM, David Milburn wrote:
>> Dan Williams wrote:
>>> Commit 56dd2c06 "libsas: Don't issue commands to devices that have been
>>> hot-removed" edited Darrick's original patch to remove setting 'gone' in
>>> the sas_deform_port() path because that prevented scsi sync cache
>>> commands from being issued when the driver was unloaded.  However, this
>>> allows true device gone notifications (as signaled port phy events) to
>>> trigger sync cache commands to devices that are known to be unreachable.
>>>
>>> Teach libsas which sas_deform_port() invocations are likely device gone
>>> events.
>>>
>>> This patch also introduces sas_device_gone() which hopefully allows
>>> subtle/tricky locking to be dropped from lldd drivers, like the
>>> following in mvsas which is broken if the ata path is ever converted to
>>> call lldd_execute_task() with irqs enabled:
>>>
>>>     flags_libsas = 0;
>>>     [...]
>>>     spin_unlock_irqrestore(dev->sata_dev.ap->lock, flags_libsas);
>>>     spin_unlock_irqrestore(&mvi->lock, flags);
>>>     t->task_done(t);
>>>     spin_lock_irqsave(&mvi->lock, flags);
>>>     spin_lock_irqsave(dev->sata_dev.ap->lock, flags_libsas);
>>>
>>> Cc: Darrick J. Wong<djwong@us.ibm.com>
>>> Cc: Haipao Fan<haipao.fan@intel.com>
>>> Cc: Maciej Trela<maciej.trela@intel.com>
>>> Cc: David Milburn<dmilburn@redhat.com>
>>> Signed-off-by: Dan Williams<dan.j.williams@intel.com>
>>> ---
>>>
>>> The sas_device_gone() change could be split into its own patch for 
>>> bisection
>>> purposes, let me know if you want me to resubmit.  This is being 
>>> driven by a
>>> lockdep error in isci as it calls task->done() from lldd_execute_task()
>>> when it finds lldd_dev == NULL for the ata domain_device.
>>
>> Dan,
>>
>> So, you are seeing sas_ata_task_done() trying to get the ata_port lock,
>> but it has already been taken earlier in the code path?
>>
>> ata_scsi_queuecmd (spin_lock_irqsave(ap->lock, irq_flags)
>>     __ata_scsi_queuecmd
>>       ata_scsi_translate
>>         ata_qc_issue
>>           sas_ata_qc_issue
>>             isci_task_execute_task
>>               isci_task_complete_for_upper_layer
>>                 sas_ata_task_done
>> (spin_lock_irqsave(dev->sata_dev.ap->lock, flags)
> 
> Exactly.
> 
> sas_device_gone() should prevent an lldd from ever attempting an i/o to 
> a missing sata device (one that has been notified via lldd_dev_gone). 
> Although, I have only had time to verify the simple unplug case and 
> sync-cache commands at driver unload.  We'll still need to handle 
> missing ssp devices, but there are no lldd-external locking concerns in 
> that path.
> 

Dan,

One other question, should sas_device_gone() be sync'ing with sata io
thru ata port lock instead of the Scsi_Host lock?

Looking at sas_queuecommand, the host_lock is released and the ata
port lock is taken before calling ata_sas_queuecmd, and ata port lock is 
held
down the __ata_scsi_queuecmd path.

Should sas_device_gone get the ap->lock from the domain_device->
sata_dev to better sync against sata io?

Thanks,
David



> -- 
> Dan