[PATCHv3] teamd: add port_master_ifindex_changed for teamd_event_watch_ops
diff mbox series

Message ID 1533718612-20619-1-git-send-email-liuhangbin@gmail.com
State New
Headers show
Series
  • [PATCHv3] teamd: add port_master_ifindex_changed for teamd_event_watch_ops
Related show

Commit Message

Hangbin Liu Aug. 8, 2018, 8:56 a.m. UTC
When we add port to new active-backup teams with multi threads. After
the port is set to link up and trigger function obj_input_newlink(),
it's possible that there is no master index in rtnl link info. So the
team slave's master_ifindex is not updated.

On the other hand, the port is up and trigger functions like
- teamd_link_watch_check_link_up()
  - teamd_event_port_link_changed()
    - ab_event_watch_port_link_changed()
      - ab_link_watch_handler()
        - teamd_for_each_tdport()
	  - teamd_get_next_tdport()
	    - teamd_port_present()

Here the teamd_port_present() failed as the port master ifindex is not
update to team ifindex yet. Finally we get nothing and no active port
is set.

Here is the reproducer:

\#bin/bash
if [ -z $1 ] || [ -z $2 ]; then
	echo "Usage: $0 iface1 iface2"
	exit 1
else
	iface1=$1
	iface2=$2
fi

WAIT=2
COUNT=0

start_team()
{
	local num=$1
	local iface=$2
	teamd -o -n -U -d -t team$num -c '{"runner": {"name": "activebackup"},"link_watch": {"name": "ethtool"}}' -gg
	teamdctl team$num port add $iface
}

while :; do
	echo "-----------------------------------------------------------"
	let "COUNT++"
	echo "Loop $COUNT"

	teamd -k -t team1
	teamd -k -t team2
	sleep "$WAIT"
	start_team 1 $iface1 &
	start_team 2 $iface2 &
	sleep "$WAIT"

	if teamdctl team1 state | grep -q "active port: $iface1" && \
		teamdctl team2 state | grep -q "active port: $iface2"; then
		echo "Pass"
        else
		echo "FAIL"
		exit 1
        fi
done

Failure as follows:

]# teamdctl teamX state dump
    "runner": {
        "active_port": ""
    },

Currently we only reproduced this with active-backup mode(I could reproduce it
in VM easily, but hard to reproduce it on physical machines).

Fix it by adding a new teamd_event_watch_ops port_master_ifindex_changed for
active-backup mode.

V2: update commit description from Jamie Bainbridge's reply.
v3: update description and reproducer.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 teamd/teamd.h                     |  5 +++++
 teamd/teamd_events.c              | 19 +++++++++++++++++++
 teamd/teamd_ifinfo_watch.c        |  5 +++++
 teamd/teamd_runner_activebackup.c |  8 ++++++++
 4 files changed, 37 insertions(+)

Comments

Jiri Pirko Aug. 8, 2018, 9:17 a.m. UTC | #1
Wed, Aug 08, 2018 at 10:56:52AM CEST, liuhangbin@gmail.com wrote:
>When we add port to new active-backup teams with multi threads. After
>the port is set to link up and trigger function obj_input_newlink(),
>it's possible that there is no master index in rtnl link info. So the
>team slave's master_ifindex is not updated.
>
>On the other hand, the port is up and trigger functions like
>- teamd_link_watch_check_link_up()
>  - teamd_event_port_link_changed()
>    - ab_event_watch_port_link_changed()
>      - ab_link_watch_handler()
>        - teamd_for_each_tdport()
>	  - teamd_get_next_tdport()
>	    - teamd_port_present()
>
>Here the teamd_port_present() failed as the port master ifindex is not
>update to team ifindex yet. Finally we get nothing and no active port
>is set.
>
>Here is the reproducer:
>
>\#bin/bash
>if [ -z $1 ] || [ -z $2 ]; then
>	echo "Usage: $0 iface1 iface2"
>	exit 1
>else
>	iface1=$1
>	iface2=$2
>fi
>
>WAIT=2
>COUNT=0
>
>start_team()
>{
>	local num=$1
>	local iface=$2
>	teamd -o -n -U -d -t team$num -c '{"runner": {"name": "activebackup"},"link_watch": {"name": "ethtool"}}' -gg
>	teamdctl team$num port add $iface
>}
>
>while :; do
>	echo "-----------------------------------------------------------"
>	let "COUNT++"
>	echo "Loop $COUNT"
>
>	teamd -k -t team1
>	teamd -k -t team2
>	sleep "$WAIT"
>	start_team 1 $iface1 &
>	start_team 2 $iface2 &
>	sleep "$WAIT"
>
>	if teamdctl team1 state | grep -q "active port: $iface1" && \
>		teamdctl team2 state | grep -q "active port: $iface2"; then
>		echo "Pass"
>        else
>		echo "FAIL"
>		exit 1
>        fi
>done
>
>Failure as follows:
>
>]# teamdctl teamX state dump
>    "runner": {
>        "active_port": ""
>    },
>
>Currently we only reproduced this with active-backup mode(I could reproduce it
>in VM easily, but hard to reproduce it on physical machines).
>
>Fix it by adding a new teamd_event_watch_ops port_master_ifindex_changed for
>active-backup mode.
>
>V2: update commit description from Jamie Bainbridge's reply.
>v3: update description and reproducer.
>
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

applied, thanks!

Patch
diff mbox series

diff --git a/teamd/teamd.h b/teamd/teamd.h
index 5dbfb9b..3934fc2 100644
--- a/teamd/teamd.h
+++ b/teamd/teamd.h
@@ -189,6 +189,9 @@  struct teamd_event_watch_ops {
 				   struct teamd_port *tdport, void *priv);
 	int (*port_ifname_changed)(struct teamd_context *ctx,
 				   struct teamd_port *tdport, void *priv);
+	int (*port_master_ifindex_changed)(struct teamd_context *ctx,
+					   struct teamd_port *tdport,
+					   void *priv);
 	int (*option_changed)(struct teamd_context *ctx,
 			      struct team_option *option, void *priv);
 	char *option_changed_match_name;
@@ -208,6 +211,8 @@  int teamd_event_ifinfo_hwaddr_changed(struct teamd_context *ctx,
 				      struct team_ifinfo *ifinfo);
 int teamd_event_ifinfo_ifname_changed(struct teamd_context *ctx,
 				      struct team_ifinfo *ifinfo);
+int teamd_event_ifinfo_master_ifindex_changed(struct teamd_context *ctx,
+					      struct team_ifinfo *ifinfo);
 int teamd_event_ifinfo_admin_state_changed(struct teamd_context *ctx,
 					   struct team_ifinfo *ifinfo);
 int teamd_events_init(struct teamd_context *ctx);
diff --git a/teamd/teamd_events.c b/teamd/teamd_events.c
index 1a95974..65aa46a 100644
--- a/teamd/teamd_events.c
+++ b/teamd/teamd_events.c
@@ -167,6 +167,25 @@  int teamd_event_ifinfo_ifname_changed(struct teamd_context *ctx,
 	return 0;
 }
 
+int teamd_event_ifinfo_master_ifindex_changed(struct teamd_context *ctx,
+					      struct team_ifinfo *ifinfo)
+{
+	struct event_watch_item *watch;
+	uint32_t ifindex = team_get_ifinfo_ifindex(ifinfo);
+	struct teamd_port *tdport = teamd_get_port(ctx, ifindex);
+	int err;
+
+	list_for_each_node_entry(watch, &ctx->event_watch_list, list) {
+		if (watch->ops->port_master_ifindex_changed && tdport) {
+			err = watch->ops->port_master_ifindex_changed(ctx, tdport,
+								      watch->priv);
+			if (err)
+				return err;
+		}
+	}
+	return 0;
+}
+
 int teamd_event_ifinfo_admin_state_changed(struct teamd_context *ctx,
 					   struct team_ifinfo *ifinfo)
 {
diff --git a/teamd/teamd_ifinfo_watch.c b/teamd/teamd_ifinfo_watch.c
index f334ff6..6a19532 100644
--- a/teamd/teamd_ifinfo_watch.c
+++ b/teamd/teamd_ifinfo_watch.c
@@ -59,6 +59,11 @@  static int ifinfo_change_handler_func(struct team_handle *th, void *priv,
 			if (err)
 				return err;
 		}
+		if (team_is_ifinfo_master_ifindex_changed(ifinfo)) {
+			err = teamd_event_ifinfo_master_ifindex_changed(ctx, ifinfo);
+			if (err)
+				return err;
+		}
 	}
 	return 0;
 }
diff --git a/teamd/teamd_runner_activebackup.c b/teamd/teamd_runner_activebackup.c
index 8a3447f..f92d341 100644
--- a/teamd/teamd_runner_activebackup.c
+++ b/teamd/teamd_runner_activebackup.c
@@ -520,6 +520,13 @@  static int ab_event_watch_port_link_changed(struct teamd_context *ctx,
 	return ab_link_watch_handler(ctx, priv);
 }
 
+static int ab_event_watch_port_master_ifindex_changed(struct teamd_context *ctx,
+						      struct teamd_port *tdport,
+						      void *priv)
+{
+	return ab_link_watch_handler(ctx, priv);
+}
+
 static int ab_event_watch_prio_option_changed(struct teamd_context *ctx,
 					      struct team_option *option,
 					      void *priv)
@@ -532,6 +539,7 @@  static const struct teamd_event_watch_ops ab_event_watch_ops = {
 	.port_hwaddr_changed = ab_event_watch_port_hwaddr_changed,
 	.port_added = ab_event_watch_port_added,
 	.port_link_changed = ab_event_watch_port_link_changed,
+	.port_master_ifindex_changed = ab_event_watch_port_master_ifindex_changed,
 	.option_changed = ab_event_watch_prio_option_changed,
 	.option_changed_match_name = "priority",
 };