Re: [PATCH] fc_user_scan correction

[James Smart <James.Smart@Emulex.Com>]

Mike Christie wrote:
> Mike Christie wrote:
>> Is this is the same as if you did not implement the user_scan callout? 
>> scsi_sysfs.c will call
>>
>> scsi_scan_host_selected(shost, channel, id, lun, 1);
>>
>> I thought we added the user_scan callback because the transport 
>> classes had to pass in the device struct between the host and target 
>> so we got
>>
>> .../host/rport/target/scsi_device
>>
>> instead of
>>
>> .../host/target/scsi_device
>>
>> qla4xxx has the same problem. Do not look at it for help :( It added a 
>> mutex and does not deadlock because like the FC class it stats the 
>> removal of the rport/session then device so the cache sync always 
>> fails (the check ready function always returns DID_NO_CONNECT so the 
>> cache sync fails). iscsi tcp/iser/bnx2i works because it has userspace 
>> helping out with the removal and shutdown and does it in two stages.
>>
> 
> I think we need some loop + locking + refcounting similar to how the 
> shost_for_each_device loops over devices.
> 

For FC, I don't believe there's any advantage to looping/locking. There's
miniscule advantages of not scanning targets that are just returned back
by the driver as not being present.

Taking another look at the user_scan sysfs routine, I can only come up with
a few reasons why it exists at all:
  - some transports/LLDs, which do target enumeration and auto-scan, can't
    handle directed scans to targets that don't exist. I have a hard time
    believing this is true.
  - There's some performance advantage for walking the transport target
    list rather than cycling on the target ids. But, this interface can't
    performance sensitive. This is the only reason I can see why user_scan
    exists (to filter out non-existent targets).
  - The "rescan" flag needs to be clean.  For transports that auto-scan,
    they have the best knowledge of when rescan should be 0 or 1. This
    protects against a race between the user scan and the 1st-time target
    discovery.

Hmm... this last point gives me concern, as I didn't fix it either.
Thoughts ?  I'm happy leaving it as the default, but the rescan bothers me.

-- james s