On Wed, 2009-11-25 at 16:53 -0800, Joe Eykholt wrote:
> My point was that after the link flap occurs, DCBX will restablish PFC
> and then set enabled, which may not be what the user wanted. Basically,
> with fcoemon, the user needs to tell fcoemon what's wanted, and not
> directly use /sys.
>
I see, I meant only fcoemon will use added enable/disable and for that
fcoemon changes yet to be done and those changes will determine when to
use enable/disable from fcoemon, any case user is not suppose to
directly use sysfs and infact fcoemon will be the only sole user to use
sysfs for create/destroy also. The fcoeadm will tell fcoemon to create
and destroy.
Use of netlink could prevent sysfs exposure for enable/disable but that
would work against WIP to add fcoe container to have fcoe specific
attributes in FC transport class which is more sysfs centric.
> > So added lport->enabled is similar to lport->link_up but this time to
> > track user enable/disable state, perhaps can be worked around by
> > introducing additional states in lport FSM.
>
> I mentioned this because I added an enabled flag before along with my rport
> changes, the goal was to not re-login after fabric_logoff was called
> while shutting down. Robert suggested I use states instead, and I
> added the disabled state to cover that.
>
I see, I'll also try to use disable state here.
> > I'll look into eliminating lport->enabled, perhaps by adding next state
> > to fc_lport_enter_reset and additional check to not transition lport on
> > link flap from certain states.
> >
> > <snip>
> >>>> +
> >>>> + rtnl_lock();
> >>>> + fcoe = fcoe_hostlist_lookup_port(netdev);
> >>>> + if (!fcoe) {
> >>>> + rtnl_unlock();
> >>>> + rc = -ENODEV;
> >>>> + goto out_putdev;
> >>>> + }
> >>>> + rtnl_unlock();
> >> You could drop rtnl_lock() before the if-statement.
> >
> > I'll do that.
> >
> >> Actually, the list can't change because fcoe_config_mutex is held,
> >> so rtnl_lock isn't needed.
> >>
> >
> > You are suggesting to add a bug here by removing rtnl lock:-) though
> > with very very unlikly race. In my understanding the fcoe_config_mutex
> > is not held on NETDEV_UNREGISTER event processing which could remove
> > fcoe instance under rtnl_lock.
>
> If that's the case, then the fcoe can go away as soon as you drop the
> rtnl_lock,
> and you have a bug anyway. NETDEV_UNREGISTER queues a work_struct which
> gets the fcoe_config_mutex. I verified that fcoe_config_mutex is always
> held when the list is changed. Let me know if you still think I missed
> something.
>
The code snippet below removes fcoe instance only under rtnl_lock w/o
fcoe_config_mutex held.
case NETDEV_UNREGISTER:
list_del(&fcoe->list);
port = lport_priv(fcoe->ctlr.lp);
fcoe_interface_cleanup(fcoe);
schedule_work(&port->destroy_work);
So in this case work is scheduled after fcoe is removed which later
grabs fcoe_config_mutex. You are right it would be a bug to drop
rtnl_lock sooner in my case. I'll fix that.
Thanks
Vasu
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
