Commit b0929915 authored by dingtianhong's avatar dingtianhong Committed by David S. Miller

bonding: Fix RTNL: assertion failed at net/core/rtnetlink.c for ab arp monitor

Veaceslav has reported and fix this problem by commit f2ebd477
(bonding: restructure locking of bond_ab_arp_probe()). According Jay's
opinion, the current solution is not very well, because the notification
is to indicate that the interface has actually changed state in a meaningful
way, but these calls in the ab ARP monitor are internal settings of the flags
to allow the ARP monitor to search for a slave to become active when there are
no active slaves. The flag setting to active or backup is to permit the ARP
monitor's response logic to do the right thing when deciding if the test
slave (current_arp_slave) is up or not.

So the best way to fix the problem is that we should not send a notification
when the slave is in testing state, and check the state at the end of the
monitor, if the slave's state recover, avoid to send pointless notification
twice. And RTNL is really a big lock, hold it regardless the slave's state
changed or not when the current_active_slave is null will loss performance
(every 100ms), so we should hold it only when the slave's state changed and
need to notify.

I revert the old commit and add new modifications.

Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: default avatarDing Tianhong <dingtianhong@huawei.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 5e5b0665
...@@ -2130,13 +2130,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work) ...@@ -2130,13 +2130,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
read_unlock(&bond->lock); read_unlock(&bond->lock);
if (should_notify_rtnl && rtnl_trylock()) { if (should_notify_rtnl && rtnl_trylock()) {
bond_for_each_slave(bond, slave, iter) { bond_slave_state_notify(bond);
if (slave->should_notify) {
rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0,
GFP_KERNEL);
slave->should_notify = 0;
}
}
rtnl_unlock(); rtnl_unlock();
} }
queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks); queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
......
...@@ -2623,17 +2623,17 @@ static void bond_ab_arp_commit(struct bonding *bond) ...@@ -2623,17 +2623,17 @@ static void bond_ab_arp_commit(struct bonding *bond)
/* /*
* Send ARP probes for active-backup mode ARP monitor. * Send ARP probes for active-backup mode ARP monitor.
*
* Called with rcu_read_lock hold.
*/ */
static bool bond_ab_arp_probe(struct bonding *bond) static bool bond_ab_arp_probe(struct bonding *bond)
{ {
struct slave *slave, *before = NULL, *new_slave = NULL, struct slave *slave, *before = NULL, *new_slave = NULL,
*curr_arp_slave, *curr_active_slave; *curr_arp_slave = rcu_dereference(bond->current_arp_slave),
*curr_active_slave = rcu_dereference(bond->curr_active_slave);
struct list_head *iter; struct list_head *iter;
bool found = false; bool found = false;
bool should_notify_rtnl = BOND_SLAVE_NOTIFY_LATER;
rcu_read_lock();
curr_arp_slave = rcu_dereference(bond->current_arp_slave);
curr_active_slave = rcu_dereference(bond->curr_active_slave);
if (curr_arp_slave && curr_active_slave) if (curr_arp_slave && curr_active_slave)
pr_info("PROBE: c_arp %s && cas %s BAD\n", pr_info("PROBE: c_arp %s && cas %s BAD\n",
...@@ -2642,32 +2642,23 @@ static bool bond_ab_arp_probe(struct bonding *bond) ...@@ -2642,32 +2642,23 @@ static bool bond_ab_arp_probe(struct bonding *bond)
if (curr_active_slave) { if (curr_active_slave) {
bond_arp_send_all(bond, curr_active_slave); bond_arp_send_all(bond, curr_active_slave);
rcu_read_unlock(); return should_notify_rtnl;
return true;
} }
rcu_read_unlock();
/* if we don't have a curr_active_slave, search for the next available /* if we don't have a curr_active_slave, search for the next available
* backup slave from the current_arp_slave and make it the candidate * backup slave from the current_arp_slave and make it the candidate
* for becoming the curr_active_slave * for becoming the curr_active_slave
*/ */
if (!rtnl_trylock())
return false;
/* curr_arp_slave might have gone away */
curr_arp_slave = ACCESS_ONCE(bond->current_arp_slave);
if (!curr_arp_slave) { if (!curr_arp_slave) {
curr_arp_slave = bond_first_slave(bond); curr_arp_slave = bond_first_slave_rcu(bond);
if (!curr_arp_slave) { if (!curr_arp_slave)
rtnl_unlock(); return should_notify_rtnl;
return true;
}
} }
bond_set_slave_inactive_flags(curr_arp_slave, BOND_SLAVE_NOTIFY_NOW); bond_set_slave_inactive_flags(curr_arp_slave, BOND_SLAVE_NOTIFY_LATER);
bond_for_each_slave(bond, slave, iter) { bond_for_each_slave_rcu(bond, slave, iter) {
if (!found && !before && IS_UP(slave->dev)) if (!found && !before && IS_UP(slave->dev))
before = slave; before = slave;
...@@ -2686,7 +2677,7 @@ static bool bond_ab_arp_probe(struct bonding *bond) ...@@ -2686,7 +2677,7 @@ static bool bond_ab_arp_probe(struct bonding *bond)
slave->link_failure_count++; slave->link_failure_count++;
bond_set_slave_inactive_flags(slave, bond_set_slave_inactive_flags(slave,
BOND_SLAVE_NOTIFY_NOW); BOND_SLAVE_NOTIFY_LATER);
pr_info("%s: backup interface %s is now down.\n", pr_info("%s: backup interface %s is now down.\n",
bond->dev->name, slave->dev->name); bond->dev->name, slave->dev->name);
...@@ -2698,26 +2689,31 @@ static bool bond_ab_arp_probe(struct bonding *bond) ...@@ -2698,26 +2689,31 @@ static bool bond_ab_arp_probe(struct bonding *bond)
if (!new_slave && before) if (!new_slave && before)
new_slave = before; new_slave = before;
if (!new_slave) { if (!new_slave)
rtnl_unlock(); goto check_state;
return true;
}
new_slave->link = BOND_LINK_BACK; new_slave->link = BOND_LINK_BACK;
bond_set_slave_active_flags(new_slave, BOND_SLAVE_NOTIFY_NOW); bond_set_slave_active_flags(new_slave, BOND_SLAVE_NOTIFY_LATER);
bond_arp_send_all(bond, new_slave); bond_arp_send_all(bond, new_slave);
new_slave->jiffies = jiffies; new_slave->jiffies = jiffies;
rcu_assign_pointer(bond->current_arp_slave, new_slave); rcu_assign_pointer(bond->current_arp_slave, new_slave);
rtnl_unlock();
return true; check_state:
bond_for_each_slave_rcu(bond, slave, iter) {
if (slave->should_notify) {
should_notify_rtnl = BOND_SLAVE_NOTIFY_NOW;
break;
}
}
return should_notify_rtnl;
} }
static void bond_activebackup_arp_mon(struct work_struct *work) static void bond_activebackup_arp_mon(struct work_struct *work)
{ {
struct bonding *bond = container_of(work, struct bonding, struct bonding *bond = container_of(work, struct bonding,
arp_work.work); arp_work.work);
bool should_notify_peers = false, should_commit = false; bool should_notify_peers = false;
bool should_notify_rtnl = false;
int delta_in_ticks; int delta_in_ticks;
delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval); delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);
...@@ -2726,11 +2722,12 @@ static void bond_activebackup_arp_mon(struct work_struct *work) ...@@ -2726,11 +2722,12 @@ static void bond_activebackup_arp_mon(struct work_struct *work)
goto re_arm; goto re_arm;
rcu_read_lock(); rcu_read_lock();
should_notify_peers = bond_should_notify_peers(bond); should_notify_peers = bond_should_notify_peers(bond);
should_commit = bond_ab_arp_inspect(bond);
rcu_read_unlock();
if (should_commit) { if (bond_ab_arp_inspect(bond)) {
rcu_read_unlock();
/* Race avoidance with bond_close flush of workqueue */ /* Race avoidance with bond_close flush of workqueue */
if (!rtnl_trylock()) { if (!rtnl_trylock()) {
delta_in_ticks = 1; delta_in_ticks = 1;
...@@ -2739,23 +2736,28 @@ static void bond_activebackup_arp_mon(struct work_struct *work) ...@@ -2739,23 +2736,28 @@ static void bond_activebackup_arp_mon(struct work_struct *work)
} }
bond_ab_arp_commit(bond); bond_ab_arp_commit(bond);
rtnl_unlock(); rtnl_unlock();
rcu_read_lock();
} }
if (!bond_ab_arp_probe(bond)) { should_notify_rtnl = bond_ab_arp_probe(bond);
/* rtnl locking failed, re-arm */ rcu_read_unlock();
delta_in_ticks = 1;
should_notify_peers = false;
}
re_arm: re_arm:
if (bond->params.arp_interval) if (bond->params.arp_interval)
queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks); queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks);
if (should_notify_peers) { if (should_notify_peers || should_notify_rtnl) {
if (!rtnl_trylock()) if (!rtnl_trylock())
return; return;
call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
if (should_notify_peers)
call_netdevice_notifiers(NETDEV_NOTIFY_PEERS,
bond->dev);
if (should_notify_rtnl)
bond_slave_state_notify(bond);
rtnl_unlock(); rtnl_unlock();
} }
} }
......
...@@ -335,6 +335,19 @@ static inline void bond_slave_state_change(struct bonding *bond) ...@@ -335,6 +335,19 @@ static inline void bond_slave_state_change(struct bonding *bond)
} }
} }
static inline void bond_slave_state_notify(struct bonding *bond)
{
struct list_head *iter;
struct slave *tmp;
bond_for_each_slave(bond, tmp, iter) {
if (tmp->should_notify) {
rtmsg_ifinfo(RTM_NEWLINK, tmp->dev, 0, GFP_KERNEL);
tmp->should_notify = 0;
}
}
}
static inline int bond_slave_state(struct slave *slave) static inline int bond_slave_state(struct slave *slave)
{ {
return slave->backup; return slave->backup;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment