Robert Love wrote:
> On Tue, 2009-05-26 at 16:21 -0700, Joe Eykholt wrote:
>> Hi all,
>>
>> I'm working on some remote port changes and would like some
>> suggestions and opinions about the best way to proceed.
>>
>> Today's issue is where the list of remote port list lives. Bottom line:
>> I'd like to move it out of struct fc_disc to struct fc_lport, and protect
>> it with lp_mutex instead of disc_mutex (eliminating disc_mutex).
>> Some of the lookups already hold lp_mutex, note especially that all
>> incoming ELS request handlers have it held.
>>
>> Currently the list is part of the discovery structure in the local port, and
>> protected under the discovery lock. For correctness, that means the
>> discovery lock should be taken when looking up a remote port (part of
>> my changes). Also, a callback from the rport module to disc is used
>> to remove an rport from the list when it logs off.
>>
> This is OK, right? We may hold both the lport and disc locks, but this
> isn't I/O so it's not a big concern.
It's OK, but separate locks makes correctness harder to verify.
Using just one lets me eliminate a work queue for the discovery callback
into lport. That eliminates one place where we had to drop the lock and
re-acquire it, which bothered me.
I hope you'll like it when you see the patches. I can send them out for
comments in a day or so, but a lot of testing is needed.
>> When in point-to-point mode, we don't need dNS, but we need a
>> single remote port, and we just put it on the list of known remote ports
>> for convenience. In this case it would be nice to have the list outside
>> of the discovery module.
>>
> I don't understand this point. How does the ptp_rport being in the list
> push you towards moving the list out of the fc_disc? So that you don't
> have to "go through" the disc layer to get at the ptp_rport? Again, not
> I/O so not a big deal.
It was in the list before and still will be, but the list will be in the
rport module.
>> When receiving an PLOGI from a remote port we haven't discovered, we need
>> a way of adding the rport.
>>
> Can't we just add routines to fc_disc.c to add this support?
Yes, but they don't have anything to do with discovery.
>> All that seems to be a good motivation to moving the list outside of
>> discovery,
>> and to put the access to the list into the rport module.
>>
>> Any thoughts on this?
>>
> Not saying, "no," I'm just playing the devil's advocate to understand
> the motivation.
>
> The only reason that we even have a fc_disc object and lock is because
> we were trying to have a "discovery engine" that might be used by some
> other code (i.e. someone else's driver).
It's still separate, but uses the lport lock, and the list is in the
rport module.
> * I was going to comment on passing lports around in the disc layer, but
> it looks like we do pass the lport to disc_start, disc_stop, etc... *
>
> Will this cause much bouncing back and forth between the lport and disc
> blocks? For example, whenever the disc layer needs to inspect the list
> (RSCN?) it would need to call back to the lport layer. I guess we'd
> already have the lport lock held because the lport layer would receive
> the RSCN before calling into the disc layer.
Right. No, this doesn't add more bouncing except that rport lookup/create
is always through the tt function pointer now. There is some new code
for GPN_ID, ADISC, etc., but the existing code gets smaller.
> Are there other times where we need to lock the lport from the disc
> layer? I'm trying to consider locking problems.
Yes, some of the places where we currently take the disc_mutex, but in
other places we used to take the disc_mutex but don't need to since we
already had the lport mutex.
>> Here are the other related things I'm working on:
>>
>> - handling RSCNs without logging out of unaffected remote ports
>> - using ADISC to reverify login status after an RSCN
>> - getting rid of the separate rogue rport list
>> - correcting the rport lookup to lock and do device_get().
>> - handle incoming PLOGI requests correctly
>> - handle incoming ADISC requests.
>>
> All of this would be wonderful!
Thanks.
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
