[PATCH net-next-2.6] bonding: move slave MTU handling from sysfs V2

[PATCH net-next-2.6] bonding: move slave MTU handling from sysfs V2

[Jiri Pirko]

V1->V2: corrected res/ret use

For some reason, MTU handling (storing, and restoring) is taking  place in
bond_sysfs. The correct place for this code is in bond_enslave, bond_release.
So move it there.

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/net/bonding/bond_main.c  |   15 ++++++++++++++-
 drivers/net/bonding/bond_sysfs.c |   22 ++--------------------
 2 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5e12462..2c3f9db 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1533,6 +1533,14 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	 */
 	new_slave->original_flags = slave_dev->flags;
 
+	/* Save slave's original mtu and then set it to match the bond */
+	new_slave->original_mtu = slave_dev->mtu;
+	res = dev_set_mtu(slave_dev, bond->dev->mtu);
+	if (res) {
+		pr_debug("Error %d calling dev_set_mtu\n", res);
+		goto err_free;
+	}
+
 	/*
 	 * Save slave's original ("permanent") mac address for modes
 	 * that need it, and for restoring it upon release, and then
@@ -1550,7 +1558,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		res = dev_set_mac_address(slave_dev, &addr);
 		if (res) {
 			pr_debug("Error %d calling set_mac_address\n", res);
-			goto err_free;
+			goto err_restore_mtu;
 		}
 	}
 
@@ -1785,6 +1793,9 @@ err_restore_mac:
 		dev_set_mac_address(slave_dev, &addr);
 	}
 
+err_restore_mtu:
+	dev_set_mtu(slave_dev, new_slave->original_mtu);
+
 err_free:
 	kfree(new_slave);
 
@@ -1969,6 +1980,8 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
 		dev_set_mac_address(slave_dev, &addr);
 	}
 
+	dev_set_mtu(slave_dev, slave->original_mtu);
+
 	slave_dev->priv_flags &= ~(IFF_MASTER_8023AD | IFF_MASTER_ALB |
 				   IFF_SLAVE_INACTIVE | IFF_BONDING |
 				   IFF_SLAVE_NEEDARP);
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 392e291..29a7a8a 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -220,7 +220,6 @@ static ssize_t bonding_store_slaves(struct device *d,
 	char command[IFNAMSIZ + 1] = { 0, };
 	char *ifname;
 	int i, res, ret = count;
-	u32 original_mtu;
 	struct slave *slave;
 	struct net_device *dev = NULL;
 	struct bonding *bond = to_bond(d);
@@ -281,18 +280,7 @@ static ssize_t bonding_store_slaves(struct device *d,
 			memcpy(bond->dev->dev_addr, dev->dev_addr,
 			       dev->addr_len);
 
-		/* Set the slave's MTU to match the bond */
-		original_mtu = dev->mtu;
-		res = dev_set_mtu(dev, bond->dev->mtu);
-		if (res) {
-			ret = res;
-			goto out;
-		}
-
 		res = bond_enslave(bond->dev, dev);
-		bond_for_each_slave(bond, slave, i)
-			if (strnicmp(slave->dev->name, ifname, IFNAMSIZ) == 0)
-				slave->original_mtu = original_mtu;
 		if (res)
 			ret = res;
 
@@ -301,23 +289,17 @@ static ssize_t bonding_store_slaves(struct device *d,
 
 	if (command[0] == '-') {
 		dev = NULL;
-		original_mtu = 0;
 		bond_for_each_slave(bond, slave, i)
 			if (strnicmp(slave->dev->name, ifname, IFNAMSIZ) == 0) {
 				dev = slave->dev;
-				original_mtu = slave->original_mtu;
 				break;
 			}
 		if (dev) {
 			pr_info("%s: Removing slave %s\n",
 				bond->dev->name, dev->name);
-				res = bond_release(bond->dev, dev);
-			if (res) {
+			res = bond_release(bond->dev, dev);
+			if (res)
 				ret = res;
-				goto out;
-			}
-			/* set the slave MTU to the default */
-			dev_set_mtu(dev, original_mtu);
 		} else {
 			pr_err("unable to remove non-existent slave %s for bond %s.\n",
 			       ifname, bond->dev->name);
-- 
1.6.6.1

Re: [PATCH net-next-2.6] bonding: move slave MTU handling from sysfs V2

[Jay Vosburgh]

Jiri Pirko <jpirko@redhat.com> wrote:

>V1->V2: corrected res/ret use
>
>For some reason, MTU handling (storing, and restoring) is taking  place in
>bond_sysfs. The correct place for this code is in bond_enslave, bond_release.
>So move it there.

	In principle this looks ok, as do the other patches, but none of
them apply to net-next-2.6 for me except for the "optimize
tlb_get_least_loaded_slave" patch.  It looks like you left out a patch,
see below.

>Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>---
> drivers/net/bonding/bond_main.c  |   15 ++++++++++++++-
> drivers/net/bonding/bond_sysfs.c |   22 ++--------------------
> 2 files changed, 16 insertions(+), 21 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 5e12462..2c3f9db 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1533,6 +1533,14 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> 	 */
> 	new_slave->original_flags = slave_dev->flags;
>
>+	/* Save slave's original mtu and then set it to match the bond */
>+	new_slave->original_mtu = slave_dev->mtu;
>+	res = dev_set_mtu(slave_dev, bond->dev->mtu);
>+	if (res) {
>+		pr_debug("Error %d calling dev_set_mtu\n", res);
>+		goto err_free;
>+	}
>+
> 	/*
> 	 * Save slave's original ("permanent") mac address for modes
> 	 * that need it, and for restoring it upon release, and then
>@@ -1550,7 +1558,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> 		res = dev_set_mac_address(slave_dev, &addr);
> 		if (res) {
> 			pr_debug("Error %d calling set_mac_address\n", res);
>-			goto err_free;
>+			goto err_restore_mtu;
> 		}
> 	}
>
>@@ -1785,6 +1793,9 @@ err_restore_mac:
> 		dev_set_mac_address(slave_dev, &addr);
> 	}
>
>+err_restore_mtu:
>+	dev_set_mtu(slave_dev, new_slave->original_mtu);
>+
> err_free:
> 	kfree(new_slave);
>
>@@ -1969,6 +1980,8 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
> 		dev_set_mac_address(slave_dev, &addr);
> 	}
>
>+	dev_set_mtu(slave_dev, slave->original_mtu);
>+
> 	slave_dev->priv_flags &= ~(IFF_MASTER_8023AD | IFF_MASTER_ALB |
> 				   IFF_SLAVE_INACTIVE | IFF_BONDING |
> 				   IFF_SLAVE_NEEDARP);
>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>index 392e291..29a7a8a 100644
>--- a/drivers/net/bonding/bond_sysfs.c
>+++ b/drivers/net/bonding/bond_sysfs.c
>@@ -220,7 +220,6 @@ static ssize_t bonding_store_slaves(struct device *d,
> 	char command[IFNAMSIZ + 1] = { 0, };
> 	char *ifname;
> 	int i, res, ret = count;
>-	u32 original_mtu;
> 	struct slave *slave;
> 	struct net_device *dev = NULL;
> 	struct bonding *bond = to_bond(d);

	This chunk doesn't apply to net-next-2.6 because your context
doesn't match; it looks like you've removed the variable "found" in your
"before" source.  On closer inspection, "found" isn't actually used
meaningfully, so I'm guessing you removed it in a prior patch but didn't
submit that patch.

	If that's the case, could you repost the whole series, with
sequence numbers?

	-J

>@@ -281,18 +280,7 @@ static ssize_t bonding_store_slaves(struct device *d,
> 			memcpy(bond->dev->dev_addr, dev->dev_addr,
> 			       dev->addr_len);
>
>-		/* Set the slave's MTU to match the bond */
>-		original_mtu = dev->mtu;
>-		res = dev_set_mtu(dev, bond->dev->mtu);
>-		if (res) {
>-			ret = res;
>-			goto out;
>-		}
>-
> 		res = bond_enslave(bond->dev, dev);
>-		bond_for_each_slave(bond, slave, i)
>-			if (strnicmp(slave->dev->name, ifname, IFNAMSIZ) == 0)
>-				slave->original_mtu = original_mtu;
> 		if (res)
> 			ret = res;
>
>@@ -301,23 +289,17 @@ static ssize_t bonding_store_slaves(struct device *d,
>
> 	if (command[0] == '-') {
> 		dev = NULL;
>-		original_mtu = 0;
> 		bond_for_each_slave(bond, slave, i)
> 			if (strnicmp(slave->dev->name, ifname, IFNAMSIZ) == 0) {
> 				dev = slave->dev;
>-				original_mtu = slave->original_mtu;
> 				break;
> 			}
> 		if (dev) {
> 			pr_info("%s: Removing slave %s\n",
> 				bond->dev->name, dev->name);
>-				res = bond_release(bond->dev, dev);
>-			if (res) {
>+			res = bond_release(bond->dev, dev);
>+			if (res)
> 				ret = res;
>-				goto out;
>-			}
>-			/* set the slave MTU to the default */
>-			dev_set_mtu(dev, original_mtu);
> 		} else {
> 			pr_err("unable to remove non-existent slave %s for bond %s.\n",
> 			       ifname, bond->dev->name);
>-- 
>1.6.6.1

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next-2.6] bonding: move slave MTU handling from sysfs V2

[Jiri Pirko]

Thu, May 20, 2010 at 02:07:41AM CEST, fubar@us.ibm.com wrote:
>Jiri Pirko <jpirko@redhat.com> wrote:
>
>>V1->V2: corrected res/ret use
>>
>>For some reason, MTU handling (storing, and restoring) is taking  place in
>>bond_sysfs. The correct place for this code is in bond_enslave, bond_release.
>>So move it there.
>
>	In principle this looks ok, as do the other patches, but none of
>them apply to net-next-2.6 for me except for the "optimize
>tlb_get_least_loaded_slave" patch.  It looks like you left out a patch,
>see below.
>
>>Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>>---
>> drivers/net/bonding/bond_main.c  |   15 ++++++++++++++-
>> drivers/net/bonding/bond_sysfs.c |   22 ++--------------------
>> 2 files changed, 16 insertions(+), 21 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 5e12462..2c3f9db 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1533,6 +1533,14 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> 	 */
>> 	new_slave->original_flags = slave_dev->flags;
>>
>>+	/* Save slave's original mtu and then set it to match the bond */
>>+	new_slave->original_mtu = slave_dev->mtu;
>>+	res = dev_set_mtu(slave_dev, bond->dev->mtu);
>>+	if (res) {
>>+		pr_debug("Error %d calling dev_set_mtu\n", res);
>>+		goto err_free;
>>+	}
>>+
>> 	/*
>> 	 * Save slave's original ("permanent") mac address for modes
>> 	 * that need it, and for restoring it upon release, and then
>>@@ -1550,7 +1558,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> 		res = dev_set_mac_address(slave_dev, &addr);
>> 		if (res) {
>> 			pr_debug("Error %d calling set_mac_address\n", res);
>>-			goto err_free;
>>+			goto err_restore_mtu;
>> 		}
>> 	}
>>
>>@@ -1785,6 +1793,9 @@ err_restore_mac:
>> 		dev_set_mac_address(slave_dev, &addr);
>> 	}
>>
>>+err_restore_mtu:
>>+	dev_set_mtu(slave_dev, new_slave->original_mtu);
>>+
>> err_free:
>> 	kfree(new_slave);
>>
>>@@ -1969,6 +1980,8 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
>> 		dev_set_mac_address(slave_dev, &addr);
>> 	}
>>
>>+	dev_set_mtu(slave_dev, slave->original_mtu);
>>+
>> 	slave_dev->priv_flags &= ~(IFF_MASTER_8023AD | IFF_MASTER_ALB |
>> 				   IFF_SLAVE_INACTIVE | IFF_BONDING |
>> 				   IFF_SLAVE_NEEDARP);
>>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>>index 392e291..29a7a8a 100644
>>--- a/drivers/net/bonding/bond_sysfs.c
>>+++ b/drivers/net/bonding/bond_sysfs.c
>>@@ -220,7 +220,6 @@ static ssize_t bonding_store_slaves(struct device *d,
>> 	char command[IFNAMSIZ + 1] = { 0, };
>> 	char *ifname;
>> 	int i, res, ret = count;
>>-	u32 original_mtu;
>> 	struct slave *slave;
>> 	struct net_device *dev = NULL;
>> 	struct bonding *bond = to_bond(d);
>
>	This chunk doesn't apply to net-next-2.6 because your context
>doesn't match; it looks like you've removed the variable "found" in your
>"before" source.  On closer inspection, "found" isn't actually used
>meaningfully, so I'm guessing you removed it in a prior patch but didn't
>submit that patch.
>
>	If that's the case, could you repost the whole series, with
>sequence numbers?

I don't think that's necessary for now. The patch removing found was posted as a
first one:
http://patchwork.ozlabs.org/patch/52795/

I tried it several times. Patches are cleanly applicable in order I posted it.

Jirka
>
>	-J
>
>>@@ -281,18 +280,7 @@ static ssize_t bonding_store_slaves(struct device *d,
>> 			memcpy(bond->dev->dev_addr, dev->dev_addr,
>> 			       dev->addr_len);
>>
>>-		/* Set the slave's MTU to match the bond */
>>-		original_mtu = dev->mtu;
>>-		res = dev_set_mtu(dev, bond->dev->mtu);
>>-		if (res) {
>>-			ret = res;
>>-			goto out;
>>-		}
>>-
>> 		res = bond_enslave(bond->dev, dev);
>>-		bond_for_each_slave(bond, slave, i)
>>-			if (strnicmp(slave->dev->name, ifname, IFNAMSIZ) == 0)
>>-				slave->original_mtu = original_mtu;
>> 		if (res)
>> 			ret = res;
>>
>>@@ -301,23 +289,17 @@ static ssize_t bonding_store_slaves(struct device *d,
>>
>> 	if (command[0] == '-') {
>> 		dev = NULL;
>>-		original_mtu = 0;
>> 		bond_for_each_slave(bond, slave, i)
>> 			if (strnicmp(slave->dev->name, ifname, IFNAMSIZ) == 0) {
>> 				dev = slave->dev;
>>-				original_mtu = slave->original_mtu;
>> 				break;
>> 			}
>> 		if (dev) {
>> 			pr_info("%s: Removing slave %s\n",
>> 				bond->dev->name, dev->name);
>>-				res = bond_release(bond->dev, dev);
>>-			if (res) {
>>+			res = bond_release(bond->dev, dev);
>>+			if (res)
>> 				ret = res;
>>-				goto out;
>>-			}
>>-			/* set the slave MTU to the default */
>>-			dev_set_mtu(dev, original_mtu);
>> 		} else {
>> 			pr_err("unable to remove non-existent slave %s for bond %s.\n",
>> 			       ifname, bond->dev->name);
>>-- 
>>1.6.6.1
>
>---
>	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next-2.6] bonding: move slave MTU handling from sysfs V2

[Jay Vosburgh]

Jiri Pirko <jpirko@redhat.com> wrote:

>Thu, May 20, 2010 at 02:07:41AM CEST, fubar@us.ibm.com wrote:
[...]
>>	This chunk doesn't apply to net-next-2.6 because your context
>>doesn't match; it looks like you've removed the variable "found" in your
>>"before" source.  On closer inspection, "found" isn't actually used
>>meaningfully, so I'm guessing you removed it in a prior patch but didn't
>>submit that patch.
>>
>>	If that's the case, could you repost the whole series, with
>>sequence numbers?
>
>I don't think that's necessary for now. The patch removing found was posted as a
>first one:
>http://patchwork.ozlabs.org/patch/52795/
>
>I tried it several times. Patches are cleanly applicable in order I posted it.

	Ok, I tracked down a copy (not sure where mine went).  Sequence
numbers do help in general, though, as a set of email messages aren't
always delivered in the same order they're sent.

	In any event, the patches all look ok to me (they do apply
cleanly and compile, now that I have the complete set), but none of them
are bug fixes, and should therefore probably wait until net-next
reopens.  

	So, for whenever the tree is open:

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next-2.6] bonding: move slave MTU handling from sysfs V2

[David Miller]

From: Jiri Pirko <jpirko@redhat.com>
Date: Tue, 18 May 2010 17:42:40 +0200

> V1->V2: corrected res/ret use
> 
> For some reason, MTU handling (storing, and restoring) is taking  place in
> bond_sysfs. The correct place for this code is in bond_enslave, bond_release.
> So move it there.
> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>

Applied.