Re: [PATCH v3 net-next 6/6] bonding: add an option to fail when any of arp_ip_target is inaccessible

[Nikolay Aleksandrov <nikolay@redhat.com>]

On 06/21/2013 03:11 PM, Veaceslav Falico wrote:
> @@ -590,10 +633,11 @@ static ssize_t bonding_store_arp_targets(struct device *d,
>   					 struct device_attribute *attr,
>   					 const char *buf, size_t count)
>   {
> -	__be32 newtarget;
> -	int i = 0, ret = -EINVAL;
>   	struct bonding *bond = to_bond(d);
> -	__be32 *targets;
> +	struct slave *slave;
> +	__be32 newtarget, *targets;
> +	unsigned long *targets_rx;
> +	int ind, i, j, ret = -EINVAL;
>
>   	targets = bond->params.arp_targets;
>   	newtarget = in_aton(buf + 1);
> @@ -611,8 +655,8 @@ static ssize_t bonding_store_arp_targets(struct device *d,
>   			goto out;
>   		}
>
> -		i = bond_get_targets_ip(targets, 0); /* first free slot */
> -		if (i == -1) {
> +		ind = bond_get_targets_ip(targets, 0); /* first free slot */
> +		if (ind == -1) {
>   			pr_err("%s: ARP target table is full!\n",
>   			       bond->dev->name);
>   			goto out;
> @@ -620,7 +664,12 @@ static ssize_t bonding_store_arp_targets(struct device *d,
>
>   		pr_info("%s: adding ARP target %pI4.\n", bond->dev->name,
>   			 &newtarget);
> -		targets[i] = newtarget;
> +		/* not to race with bond_arp_rcv */
> +		write_lock_bh(&bond->lock);
> +		bond_for_each_slave(bond, slave, i)
> +			slave->target_last_arp_rx[ind] = jiffies;
> +		targets[ind] = newtarget;
> +		write_unlock_bh(&bond->lock);
>   	} else if (buf[0] == '-')	{
>   		if ((newtarget == 0) || (newtarget == htonl(INADDR_BROADCAST))) {
>   			pr_err("%s: invalid ARP target %pI4 specified for removal\n",
> @@ -628,18 +677,32 @@ static ssize_t bonding_store_arp_targets(struct device *d,
>   			goto out;
>   		}
>
> -		i = bond_get_targets_ip(targets, newtarget);
> -		if (i == -1) {
> -			pr_info("%s: unable to remove nonexistent ARP target %pI4.\n",
> +		ind = bond_get_targets_ip(targets, newtarget);
> +		if (ind == -1) {
> +			pr_err("%s: unable to remove nonexistent ARP target %pI4.\n",
>   				bond->dev->name, &newtarget);
>   			goto out;
>   		}
>
> +		if (ind == 0 && !targets[1] && bond->params.arp_validate)
> +			pr_warn("%s: removing last arp target with arp_validate on\n",
> +				bond->dev->name);
> +
>   		pr_info("%s: removing ARP target %pI4.\n", bond->dev->name,
>   			&newtarget);
> -		for (; (i < BOND_MAX_ARP_TARGETS-1) && targets[i+1]; i++)
> +
> +		write_lock_bh(&bond->lock);
> +		bond_for_each_slave(bond, slave, i) {
> +			targets_rx = slave->target_last_arp_rx;
> +			j = ind;
> +			for (; (j < BOND_MAX_ARP_TARGETS-1) && targets[j+1]; j++)
> +				targets_rx[j] = targets_rx[j+1];
> +			targets_rx[j] = 0;
> +		}
> +		for (i = ind; (i < BOND_MAX_ARP_TARGETS-1) && targets[i+1]; i++)
>   			targets[i] = targets[i+1];
>   		targets[i] = 0;
> +		write_unlock_bh(&bond->lock);
>   	} else {
>   		pr_err("no command found in arp_ip_targets file for bond %s. Use +<addr> or -<addr>.\n",
>   		       bond->dev->name);
> @@ -649,6 +712,7 @@ static ssize_t bonding_store_arp_targets(struct device *d,
>
>   	ret = count;
>   out:
> +	rtnl_unlock();
A leftover rtnl_unlock here.

Nik

>   	return ret;
>   }
>   static DEVICE_ATTR(arp_ip_target, S_IRUGO | S_IWUSR , bonding_show_arp_targets, bonding_store_arp_targets);
> @@ -1623,6 +1687,7 @@ static struct attribute *per_bond_attrs[] = {
>   	&dev_attr_mode.attr,
>   	&dev_attr_fail_over_mac.attr,
>   	&dev_attr_arp_validate.attr,
> +	&dev_attr_arp_all_targets.attr,
>   	&dev_attr_arp_interval.attr,
>   	&dev_attr_arp_ip_target.attr,
>   	&dev_attr_downdelay.attr,
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index 486e532..3fb73cc 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -144,6 +144,7 @@ struct bond_params {
>   	u8 num_peer_notif;
>   	int arp_interval;
>   	int arp_validate;
> +	int arp_all_targets;
>   	int use_carrier;
>   	int fail_over_mac;
>   	int updelay;
> @@ -179,6 +180,7 @@ struct slave {
>   	int    delay;
>   	unsigned long jiffies;
>   	unsigned long last_arp_rx;
> +	unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
>   	s8     link;    /* one of BOND_LINK_XXXX */
>   	s8     new_link;
>   	u8     backup:1,   /* indicates backup slave. Value corresponds with
> @@ -322,6 +324,9 @@ static inline bool bond_is_active_slave(struct slave *slave)
>   #define BOND_FOM_ACTIVE			1
>   #define BOND_FOM_FOLLOW			2
>
> +#define BOND_ARP_TARGETS_ANY		0
> +#define BOND_ARP_TARGETS_ALL		1
> +
>   #define BOND_ARP_VALIDATE_NONE		0
>   #define BOND_ARP_VALIDATE_ACTIVE	(1 << BOND_STATE_ACTIVE)
>   #define BOND_ARP_VALIDATE_BACKUP	(1 << BOND_STATE_BACKUP)
> @@ -334,11 +339,31 @@ static inline int slave_do_arp_validate(struct bonding *bond,
>   	return bond->params.arp_validate & (1 << bond_slave_state(slave));
>   }
>
> +/* Get the oldest arp which we've received on this slave for bond's
> + * arp_targets.
> + */
> +static inline unsigned long slave_oldest_target_arp_rx(struct bonding *bond,
> +						       struct slave *slave)
> +{
> +	int i = 1;
> +	unsigned long ret = slave->target_last_arp_rx[0];
> +
> +	for (; (i < BOND_MAX_ARP_TARGETS) && bond->params.arp_targets[i]; i++)
> +		if (time_before(slave->target_last_arp_rx[i], ret))
> +			ret = slave->target_last_arp_rx[i];
> +
> +	return ret;
> +}
> +
>   static inline unsigned long slave_last_rx(struct bonding *bond,
>   					struct slave *slave)
>   {
> -	if (slave_do_arp_validate(bond, slave))
> -		return slave->last_arp_rx;
> +	if (slave_do_arp_validate(bond, slave)) {
> +		if (bond->params.arp_all_targets == BOND_ARP_TARGETS_ALL)
> +			return slave_oldest_target_arp_rx(bond, slave);
> +		else
> +			return slave->last_arp_rx;
> +	}
>
>   	return slave->dev->last_rx;
>   }
> @@ -486,6 +511,7 @@ extern const struct bond_parm_tbl bond_lacp_tbl[];
>   extern const struct bond_parm_tbl bond_mode_tbl[];
>   extern const struct bond_parm_tbl xmit_hashtype_tbl[];
>   extern const struct bond_parm_tbl arp_validate_tbl[];
> +extern const struct bond_parm_tbl arp_all_targets_tbl[];
>   extern const struct bond_parm_tbl fail_over_mac_tbl[];
>   extern const struct bond_parm_tbl pri_reselect_tbl[];
>   extern struct bond_parm_tbl ad_select_tbl[];
>