bonding: reset the slave's mtu when its be changed
Subsystems (3)
PATCH 1/1 bonding: reset the slave's mtu when its be changed
All slave should have the same mtu with mastet's, and the bond do it when enslave the slave, but the user could change the slave's mtu, it will cause the master and slave have different mtu, althrough in AB mode, it does not matter if the slave is not the current slave, but in other mode, it is incorrect, so reset the slave's mtu like the master set.
--- drivers/net/bonding/bond_main.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
drivers/net/bonding/bond_main.c +10 -11
@@ -2882,18 +2882,17 @@ static int bond_slave_netdev_event(unsigned long event,*/ break ;case NETDEV_CHANGEMTU : -/* -* TODO: Should slaves be allowed to -* independently alter their MTU? For -* an active-backup bond, slaves need -* not be the same type of device, so -* MTUs may vary. For other modes, -* slaves arguably should have the -* same MTUs. To do this, we'd need to -* take over the slave's change_mtu -* function for the duration of their -* servitude. +/* All slave should have the same mtu +* as master. */ +if (slave->dev->mtu != bond->dev->mtu) { +int res; + 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 for slave %s\n" , + res, slave->dev->name); + }break ;case NETDEV_CHANGENAME :/*
--
1.8.0
Quoted 5 lines · Ding Tianhong
All slave should have the same mtu with mastet's, and the bond do it when enslave the slave, but the user could change the slave's mtu, it will cause the master and slave have different mtu, althrough in AB mode, it does not matter if the slave is not the current slave, but in other mode, it is incorrect, so reset the slave's mtu like the master set.
Why "net"? It's not a bugfix, it's a feature, and really discussable.
Also, wrt the actual change - why do you think it's incorrect for slaves in bonding mode other than AB to have different MTU values? I don't see any reason for it, from the top of the head.
Quoted 21 lines
Cc Jay Vosburgh <[email protected]>Cc Veaceslav Falico <[email protected]>Signed-off-by Ding Tianhong <[email protected]>---
drivers/net/bonding/bond_main.c| 21 ++++++++++-----------1 file changed, 10 insertions(+), 11 deletions(-)drivers/net/bonding/bond_main.c +0
-5@@ -2882,18 +2882,17 @@ static int bond_slave_netdev_event(unsigned long event,*/ break ;case NETDEV_CHANGEMTU : -/* -* TODO: Should slaves be allowed to -* independently alter their MTU? For -* an active-backup bond, slaves need -* not be the same type of device, so
drivers/net/bonding/bond_main.c static int bond_slave_netdev_event(unsigned long event, · line 2888
Quoted 10 lines
drivers/net/bonding/bond_main.c +3
-6-* MTUs may vary. For other modes, -* slaves arguably should have the -* same MTUs. To do this, we'd need to -* take over the slave's change_mtu -* function for the duration of their -* servitude. +/* All slave should have the same mtu +* as master. */ +if (slave->dev->mtu != bond->dev->mtu) {
If we've got the event then it means it was changed to something different. No need to verify.
drivers/net/bonding/bond_main.c static int bond_slave_netdev_event(unsigned long event, · line 2890
Quoted 2 lines
drivers/net/bonding/bond_main.c +2
-0+int res; + slave->original_mtu = slave->dev->mtu;
If we're refusing to apply the *new* mtu, then why should we save it as the original? The original_mtu is the mtu that the slave had before it was enslaved.
drivers/net/bonding/bond_main.c static int bond_slave_netdev_event(unsigned long event, · line 2895
Quoted 5 lines
drivers/net/bonding/bond_main.c +5
-0+ res =dev_set_mtu (slave->dev, bond->dev->mtu); +if (res) +pr_debug ("Error %d calling dev_set_mtu for slave %s\n" , + res, slave->dev->name); + }
Also, bonding should be vocal about changing forcibly the mtu - otherwise we'd end up with silently dropping the changes:
ifconfig eth0 mtu 9000
echo $?
-> 0
ifconfig eth0
MTU: 1500
or something like that, it will pass it up, refusing changes:
drivers/net/bonding/bond_main.c +2 -13
@@ -2846,19 +2846,8 @@ static int bond_slave_netdev_event(unsigned long event,*/ break ;case NETDEV_CHANGEMTU : -/* -* TODO: Should slaves be allowed to -* independently alter their MTU? For -* an active-backup bond, slaves need -* not be the same type of device, so -* MTUs may vary. For other modes, -* slaves arguably should have the -* same MTUs. To do this, we'd need to -* take over the slave's change_mtu -* function for the duration of their -* servitude. -*/ -break ; +/* don't permit slaves to change their MTU */ +return NOTIFY_BAD ;case NETDEV_CHANGENAME :/* * TODO: handle changing the primary's name
Quoted 7 lines
drivers/net/bonding/bond_main.c +0
-1break ;case NETDEV_CHANGENAME :/* -- 1.8.0
Quoted 7 lines · Veaceslav Falico
Quoted 5 lines · Ding Tianhong
All slave should have the same mtu with mastet's, and the bond do it when enslave the slave, but the user could change the slave's mtu, it will cause the master and slave have different mtu, althrough in AB mode, it does not matter if the slave is not the current slave, but in other mode, it is incorrect, so reset the slave's mtu like the master set.
Quoted 6 lines
Why "net"? It's not a bugfix, it's a feature, and really discussable.
Also, wrt the actual change - why do you think it's incorrect for slaves in bonding mode other than AB to have different MTU values? I don't see any reason for it, from the top of the head.
Ok, I will test more situation for every mode when slave's mtu changed, I am not sure what will happened yet, if some links was interrupt, I thinks it is a bug.
Quoted 35 lines
Quoted 21 lines
Cc Jay Vosburgh <[email protected]>Cc Veaceslav Falico <[email protected]>Signed-off-by Ding Tianhong <[email protected]>---
drivers/net/bonding/bond_main.c| 21 ++++++++++-----------1 file changed, 10 insertions(+), 11 deletions(-)drivers/net/bonding/bond_main.c +0
-5@@ -2882,18 +2882,17 @@ static int bond_slave_netdev_event(unsigned long event,*/ break ;case NETDEV_CHANGEMTU : -/* -* TODO: Should slaves be allowed to -* independently alter their MTU? For -* an active-backup bond, slaves need -* not be the same type of device, so drivers/net/bonding/bond_main.c static int bond_slave_netdev_event(unsigned long event, · line 2888
Quoted 10 lines
drivers/net/bonding/bond_main.c +3
-6-* MTUs may vary. For other modes, -* slaves arguably should have the -* same MTUs. To do this, we'd need to -* take over the slave's change_mtu -* function for the duration of their -* servitude. +/* All slave should have the same mtu +* as master. */ +if (slave->dev->mtu != bond->dev->mtu) {If we've got the event then it means it was changed to something different. No need to verify.
Quoted 7 lines
drivers/net/bonding/bond_main.c static int bond_slave_netdev_event(unsigned long event, · line 2890
Quoted 2 lines
drivers/net/bonding/bond_main.c +2
-0+int res; + slave->original_mtu = slave->dev->mtu;If we're refusing to apply the *new* mtu, then why should we save it as the original? The
original_mtuis the mtu that the slave had before it was enslaved.
the bond always save the slave's old mtu and set new one, so I did it again, pls miss it, I think we should forbidden to change the mtu.
Quoted 44 lines
drivers/net/bonding/bond_main.c static int bond_slave_netdev_event(unsigned long event, · line 2895
Quoted 5 lines
drivers/net/bonding/bond_main.c +5
-0+ res =dev_set_mtu (slave->dev, bond->dev->mtu); +if (res) +pr_debug ("Error %d calling dev_set_mtu for slave %s\n" , + res, slave->dev->name); + }Also, bonding should be vocal about changing forcibly the mtu - otherwise we'd end up with silently dropping the changes:
ifconfig eth0 mtu 9000
echo $?
-> 0
ifconfig eth0
MTU: 1500or something like that, it will pass it up, refusing changes:
drivers/net/bonding/bond_main.c +2
-13@@ -2846,19 +2846,8 @@ static int bond_slave_netdev_event(unsigned long event,*/ break ;case NETDEV_CHANGEMTU : -/* -* TODO: Should slaves be allowed to -* independently alter their MTU? For -* an active-backup bond, slaves need -* not be the same type of device, so -* MTUs may vary. For other modes, -* slaves arguably should have the -* same MTUs. To do this, we'd need to -* take over the slave's change_mtu -* function for the duration of their -* servitude. -*/ -break ; +/* don't permit slaves to change their MTU */ +return NOTIFY_BAD ;case NETDEV_CHANGENAME :/* * TODO: handle changing the primary's name
Quoted 6 lines
Quoted 6 lines
drivers/net/bonding/bond_main.c +0
-0break ;case NETDEV_CHANGENAME :/* Signature · 1 line
1.8.0
Yes, no problem.
Regards
Ding
Quoted 4 lines
Quoted 1 line
.
Quoted 10 lines · Ding Tianhong
Quoted 9 lines · Veaceslav Falico
Quoted 5 lines · Ding Tianhong
All slave should have the same mtu with mastet's, and the bond do it when enslave the slave, but the user could change the slave's mtu, it will cause the master and slave have different mtu, althrough in AB mode, it does not matter if the slave is not the current slave, but in other mode, it is incorrect, so reset the slave's mtu like the master set.
Why "net"? It's not a bugfix, it's a feature, and really discussable.
Quoted 9 lines
Quoted 4 lines
Also, wrt the actual change - why do you think it's incorrect for slaves in bonding mode other than AB to have different MTU values? I don't see any reason for it, from the top of the head.
Ok, I will test more situation for every mode when slave's mtu changed, I am not sure what will happened yet, if some links was interrupt, I thinks it is a bug.
Quoted 1 line
Quoted 1 line
I have test several mode for bonding when the slave mtu changed:
RR(0) 0<mtu<1500 ok
AB(1) 0<mtu<1500 loss packets
XOR(2) 0<mtu<1500 ok
Broadcast(3) 0<mtu<1500 ok
LACP 0<mtu<1500 loss packets
so I think we should not let the mtu set for slave.
Quoted 23 lines · Ding Tianhong
Quoted 10 lines · Ding Tianhong
Quoted 9 lines · Veaceslav Falico
Quoted 5 lines · Ding Tianhong
All slave should have the same mtu with mastet's, and the bond do it when enslave the slave, but the user could change the slave's mtu, it will cause the master and slave have different mtu, althrough in AB mode, it does not matter if the slave is not the current slave, but in other mode, it is incorrect, so reset the slave's mtu like the master set.
Why "net"? It's not a bugfix, it's a feature, and really discussable.
Quoted 9 lines
Quoted 4 lines
Also, wrt the actual change - why do you think it's incorrect for slaves in bonding mode other than AB to have different MTU values? I don't see any reason for it, from the top of the head.
Ok, I will test more situation for every mode when slave's mtu changed, I am not sure what will happened yet, if some links was interrupt, I thinks it is a bug.
Quoted 1 line
Quoted 1 line
I have test several mode for bonding when the slave mtu changed:
Quoted 8 lines
RR(0) 0<mtu<1500 ok
AB(1) 0<mtu<1500 loss packets
XOR(2) 0<mtu<1500 ok
Broadcast(3) 0<mtu<1500 ok
LACP 0<mtu<1500 loss packetsso I think we should not let the mtu set for slave.
Why do you see lost packets?
Quoted 1 line
David Miller
maintainerFrom: Ding Tianhong <[email protected]>
Date: Fri, 10 Jan 2014 19:32:51 +0800
Quoted 1 line
All slave should have the same mtu with mastet's, and the bond do it when
^^^^^^
Typo, you mean "master"
Hello.
Quoted 1 line · Ding Tianhong
All slave should have the same mtu with mastet's, and the bond do it when
Only "master", already noted by Dave.
Quoted 2 lines
enslave the slave, but the user could change the slave's mtu, it will cause the master and slave have different mtu, althrough in AB mode, it does not
Only "although".
Quoted 24 lines
matter if the slave is not the current slave, but in other mode, it is incorrect, so reset the slave's mtu like the master set.
Cc Jay Vosburgh <[email protected]>Cc Veaceslav Falico <[email protected]>Signed-off-by Ding Tianhong <[email protected]>--- drivers/net/bonding/bond_main.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)drivers/net/bonding/bond_main.c +0
-6@@ -2882,18 +2882,17 @@ static int bond_slave_netdev_event(unsigned long event,*/ break ;case NETDEV_CHANGEMTU : -/* -* TODO: Should slaves be allowed to -* independently alter their MTU? For -* an active-backup bond, slaves need -* not be the same type of device, so -* MTUs may vary. For other modes,
drivers/net/bonding/bond_main.c static int bond_slave_netdev_event(unsigned long event, · line 2889
Quoted 10 lines
drivers/net/bonding/bond_main.c +4
-5-* slaves arguably should have the -* same MTUs. To do this, we'd need to -* take over the slave's change_mtu -* function for the duration of their -* servitude. +/* All slave should have the same mtu +* as master. */ +if (slave->dev->mtu != bond->dev->mtu) { +int res;
Please insert empty line after declaration.
drivers/net/bonding/bond_main.c static int bond_slave_netdev_event(unsigned long event, · line 2894
Quoted 5 lines
drivers/net/bonding/bond_main.c +5
-0+ 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 for slave %s\n" , + res, slave->dev->name);
{} wouldn't hurt around multi-line *if* arm.
drivers/net/bonding/bond_main.c static int bond_slave_netdev_event(unsigned long event, · line 2898
Quoted 4 lines
drivers/net/bonding/bond_main.c +1
-0+ }break ;case NETDEV_CHANGENAME :/*
WBR, Sergei