| Message ID | 1427932538-16059-1-git-send-email-vyasevic@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 01, 2015 at 07:55:38PM -0400, Vladislav Yasevich wrote: > Commit 02d38fcb2caa4454cf4ed728d5908c3cc9ba47be > net: Update netdev peer on link change > > introduced a link state dependency between the backend > netdev and the nic. If the admin turned off the link > on the backend, the nic link state was set to down because > the link had no access to the network at all. This created > some inconsitet behavior for someone who wanted to play > around the links states. > 1) Turning off the nic link and then turning on the backend > link (even if it was already on) would turn on the nic link > again. > 2) Turning off the backend link and then turning on the nic > link would turn on the link in the VM, but would not change > the backend state resulting in a broken/unreachable network. > > This patch attempts to correct these behaviors. The patch treats > the nic-backend relationship as two ends of a wire. Each end tracks > the administratively set link state in addition to actual link > state. Thus is is possible to set link down at each end effectively > emulating plugging/unplugging the wire at either end. The patch > continues to preserve the old behavior where setting just > nic side down does NOT impact the backend. > > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> I never understood the point of 02d38fcb2caa4454cf4ed728d5908c3cc9ba47be. If you want to tell guest link is down, just set the nic link down. How about we just revert it? Then one can change link state on each end independently, with no notification, and no state to track. > --- > include/net/net.h | 1 + > net/net.c | 25 +++++++++++++++++++++++-- > roms/ipxe | 2 +- > roms/openbios | 2 +- > roms/seabios | 2 +- > 5 files changed, 27 insertions(+), 5 deletions(-) > > diff --git a/include/net/net.h b/include/net/net.h > index 50ffcb9..8d04861 100644 > --- a/include/net/net.h > +++ b/include/net/net.h > @@ -78,6 +78,7 @@ typedef struct NetClientInfo { > struct NetClientState { > NetClientInfo *info; > int link_down; > + int admin_link_down; > QTAILQ_ENTRY(NetClientState) next; > NetClientState *peer; > NetQueue *incoming_queue; > diff --git a/net/net.c b/net/net.c > index 0be084d..be8de61 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -1158,6 +1158,26 @@ void qmp_set_link(const char *name, bool up, Error **errp) > } > nc = ncs[0]; > > + /* Always set the administratively requested link state */ > + nc->admin_link_down = !up; > + > + /* If link status will not change, don't bother doing the work */ > + if (nc->link_down == !up) { > + return; > + } > + > + /* If this is a NIC device and the user is trying to turn the link on, > + * look at our peer link state first. > + * If the peer link state is down, then the peer was unpluged and > + * turning on the link state on this NIC will result in a broken network. > + * Report an error to the user. > + */ > + if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC && up && > + nc->peer && nc->peer->link_down) { > + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, name); > + return; > + } > + > for (i = 0; i < queues; i++) { > ncs[i]->link_down = !up; > } > @@ -1166,8 +1186,9 @@ void qmp_set_link(const char *name, bool up, Error **errp) > nc->info->link_status_changed(nc); > } > > - if (nc->peer) { > - /* Change peer link only if the peer is NIC and then notify peer. > + if (nc->peer && !nc->peer->admin_link_down) { > + /* Change peer link only if the peer is a NIC that has not been forced > + * down and then notify peer. > * If the peer is a HUBPORT or a backend, we do not change the > * link status. > * > diff --git a/roms/ipxe b/roms/ipxe > index 35c5379..5de37e1 160000 > --- a/roms/ipxe > +++ b/roms/ipxe > @@ -1 +1 @@ > -Subproject commit 35c5379760aa1fea5e38f7a78b090f92bb7813ee > +Subproject commit 5de37e124fd21c8f918f3fe26fc33a764709ead4 > diff --git a/roms/openbios b/roms/openbios > index 5d3db90..d9e38ba 160000 > --- a/roms/openbios > +++ b/roms/openbios > @@ -1 +1 @@ > -Subproject commit 5d3db901435ef5a114c9d89461dd0f6d1ef1d44f > +Subproject commit d9e38ba2ffd2d2cdfa840ea9bc7dd4a64472f2c7 > diff --git a/roms/seabios b/roms/seabios > index 4adadbd..67d1fbe 160000 > --- a/roms/seabios > +++ b/roms/seabios > @@ -1 +1 @@ > -Subproject commit 4adadbde6904807de2e990c0af839ad0cc977806 > +Subproject commit 67d1fbef0f630e1e823f137d1bae7fa5790bcf4e > -- > 1.9.3
On Thu, Apr 02, 2015 at 08:11:15AM +0200, Michael S. Tsirkin wrote: > On Wed, Apr 01, 2015 at 07:55:38PM -0400, Vladislav Yasevich wrote: > > Commit 02d38fcb2caa4454cf4ed728d5908c3cc9ba47be > > net: Update netdev peer on link change > > > > introduced a link state dependency between the backend > > netdev and the nic. If the admin turned off the link > > on the backend, the nic link state was set to down because > > the link had no access to the network at all. This created > > some inconsitet behavior for someone who wanted to play > > around the links states. > > 1) Turning off the nic link and then turning on the backend > > link (even if it was already on) would turn on the nic link > > again. > > 2) Turning off the backend link and then turning on the nic > > link would turn on the link in the VM, but would not change > > the backend state resulting in a broken/unreachable network. > > > > This patch attempts to correct these behaviors. The patch treats > > the nic-backend relationship as two ends of a wire. Each end tracks > > the administratively set link state in addition to actual link > > state. Thus is is possible to set link down at each end effectively > > emulating plugging/unplugging the wire at either end. The patch > > continues to preserve the old behavior where setting just > > nic side down does NOT impact the backend. > > > > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > > I never understood the point of > 02d38fcb2caa4454cf4ed728d5908c3cc9ba47be. > > If you want to tell guest link is down, > just set the nic link down. > > How about we just revert it? > > Then one can change link state on each end > independently, with no notification, > and no state to track. I agree. The simple solution would be nice, unless you are aware of a use case where it causes problems. Stefan
On Wed, Apr 01, 2015 at 07:55:38PM -0400, Vladislav Yasevich wrote: > diff --git a/roms/ipxe b/roms/ipxe > index 35c5379..5de37e1 160000 > --- a/roms/ipxe > +++ b/roms/ipxe > @@ -1 +1 @@ > -Subproject commit 35c5379760aa1fea5e38f7a78b090f92bb7813ee > +Subproject commit 5de37e124fd21c8f918f3fe26fc33a764709ead4 > diff --git a/roms/openbios b/roms/openbios > index 5d3db90..d9e38ba 160000 > --- a/roms/openbios > +++ b/roms/openbios > @@ -1 +1 @@ > -Subproject commit 5d3db901435ef5a114c9d89461dd0f6d1ef1d44f > +Subproject commit d9e38ba2ffd2d2cdfa840ea9bc7dd4a64472f2c7 > diff --git a/roms/seabios b/roms/seabios > index 4adadbd..67d1fbe 160000 > --- a/roms/seabios > +++ b/roms/seabios > @@ -1 +1 @@ > -Subproject commit 4adadbde6904807de2e990c0af839ad0cc977806 > +Subproject commit 67d1fbef0f630e1e823f137d1bae7fa5790bcf4e Oops
On 04/02/2015 05:21 AM, Stefan Hajnoczi wrote: > On Thu, Apr 02, 2015 at 08:11:15AM +0200, Michael S. Tsirkin wrote: >> On Wed, Apr 01, 2015 at 07:55:38PM -0400, Vladislav Yasevich wrote: >>> Commit 02d38fcb2caa4454cf4ed728d5908c3cc9ba47be >>> net: Update netdev peer on link change >>> >>> introduced a link state dependency between the backend >>> netdev and the nic. If the admin turned off the link >>> on the backend, the nic link state was set to down because >>> the link had no access to the network at all. This created >>> some inconsitet behavior for someone who wanted to play >>> around the links states. >>> 1) Turning off the nic link and then turning on the backend >>> link (even if it was already on) would turn on the nic link >>> again. >>> 2) Turning off the backend link and then turning on the nic >>> link would turn on the link in the VM, but would not change >>> the backend state resulting in a broken/unreachable network. >>> >>> This patch attempts to correct these behaviors. The patch treats >>> the nic-backend relationship as two ends of a wire. Each end tracks >>> the administratively set link state in addition to actual link >>> state. Thus is is possible to set link down at each end effectively >>> emulating plugging/unplugging the wire at either end. The patch >>> continues to preserve the old behavior where setting just >>> nic side down does NOT impact the backend. >>> >>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> >> >> I never understood the point of >> 02d38fcb2caa4454cf4ed728d5908c3cc9ba47be. >> >> If you want to tell guest link is down, >> just set the nic link down. >> >> How about we just revert it? >> >> Then one can change link state on each end >> independently, with no notification, >> and no state to track. > > I agree. The simple solution would be nice, unless you are aware of a > use case where it causes problems. It causes problems when the user turns off link on the backend. Problems range from much longer boot up times on older VMs to Windows networks not working. The problem is the if you turn off the link on the backend, the link to the VM stays up but can't really reach the network. Now, you could say "Don't do this" and document it, but it's still ugly and causes issues. We could disable the link state change on backends and only allow it on NIC type devices. That would also solve this. -vlad > > Stefan >
On Thu, Apr 02, 2015 at 09:09:35AM -0400, Vlad Yasevich wrote: > On 04/02/2015 05:21 AM, Stefan Hajnoczi wrote: > > On Thu, Apr 02, 2015 at 08:11:15AM +0200, Michael S. Tsirkin wrote: > >> On Wed, Apr 01, 2015 at 07:55:38PM -0400, Vladislav Yasevich wrote: > >>> Commit 02d38fcb2caa4454cf4ed728d5908c3cc9ba47be > >>> net: Update netdev peer on link change > >>> > >>> introduced a link state dependency between the backend > >>> netdev and the nic. If the admin turned off the link > >>> on the backend, the nic link state was set to down because > >>> the link had no access to the network at all. This created > >>> some inconsitet behavior for someone who wanted to play > >>> around the links states. > >>> 1) Turning off the nic link and then turning on the backend > >>> link (even if it was already on) would turn on the nic link > >>> again. > >>> 2) Turning off the backend link and then turning on the nic > >>> link would turn on the link in the VM, but would not change > >>> the backend state resulting in a broken/unreachable network. > >>> > >>> This patch attempts to correct these behaviors. The patch treats > >>> the nic-backend relationship as two ends of a wire. Each end tracks > >>> the administratively set link state in addition to actual link > >>> state. Thus is is possible to set link down at each end effectively > >>> emulating plugging/unplugging the wire at either end. The patch > >>> continues to preserve the old behavior where setting just > >>> nic side down does NOT impact the backend. > >>> > >>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > >> > >> I never understood the point of > >> 02d38fcb2caa4454cf4ed728d5908c3cc9ba47be. > >> > >> If you want to tell guest link is down, > >> just set the nic link down. > >> > >> How about we just revert it? > >> > >> Then one can change link state on each end > >> independently, with no notification, > >> and no state to track. > > > > I agree. The simple solution would be nice, unless you are aware of a > > use case where it causes problems. > > It causes problems when the user turns off link on the backend. Problems range from much > longer boot up times on older VMs to Windows networks not working. > > The problem is the if you turn off the link on the backend, the link to the VM stays > up but can't really reach the network. IIUC the point of turning the link on the backend off was to do this for short periods of time. This allows e.g. simulating network outages. > Now, you could say "Don't do this" and document it, but it's still ugly and causes > issues. > > We could disable the link state change on backends and only allow it on NIC type devices. > That would also solve this. > > -vlad It's too late for 2.3 though. > > > > Stefan > >
diff --git a/include/net/net.h b/include/net/net.h index 50ffcb9..8d04861 100644 --- a/include/net/net.h +++ b/include/net/net.h @@ -78,6 +78,7 @@ typedef struct NetClientInfo { struct NetClientState { NetClientInfo *info; int link_down; + int admin_link_down; QTAILQ_ENTRY(NetClientState) next; NetClientState *peer; NetQueue *incoming_queue; diff --git a/net/net.c b/net/net.c index 0be084d..be8de61 100644 --- a/net/net.c +++ b/net/net.c @@ -1158,6 +1158,26 @@ void qmp_set_link(const char *name, bool up, Error **errp) } nc = ncs[0]; + /* Always set the administratively requested link state */ + nc->admin_link_down = !up; + + /* If link status will not change, don't bother doing the work */ + if (nc->link_down == !up) { + return; + } + + /* If this is a NIC device and the user is trying to turn the link on, + * look at our peer link state first. + * If the peer link state is down, then the peer was unpluged and + * turning on the link state on this NIC will result in a broken network. + * Report an error to the user. + */ + if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC && up && + nc->peer && nc->peer->link_down) { + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, name); + return; + } + for (i = 0; i < queues; i++) { ncs[i]->link_down = !up; } @@ -1166,8 +1186,9 @@ void qmp_set_link(const char *name, bool up, Error **errp) nc->info->link_status_changed(nc); } - if (nc->peer) { - /* Change peer link only if the peer is NIC and then notify peer. + if (nc->peer && !nc->peer->admin_link_down) { + /* Change peer link only if the peer is a NIC that has not been forced + * down and then notify peer. * If the peer is a HUBPORT or a backend, we do not change the * link status. * diff --git a/roms/ipxe b/roms/ipxe index 35c5379..5de37e1 160000 --- a/roms/ipxe +++ b/roms/ipxe @@ -1 +1 @@ -Subproject commit 35c5379760aa1fea5e38f7a78b090f92bb7813ee +Subproject commit 5de37e124fd21c8f918f3fe26fc33a764709ead4 diff --git a/roms/openbios b/roms/openbios index 5d3db90..d9e38ba 160000 --- a/roms/openbios +++ b/roms/openbios @@ -1 +1 @@ -Subproject commit 5d3db901435ef5a114c9d89461dd0f6d1ef1d44f +Subproject commit d9e38ba2ffd2d2cdfa840ea9bc7dd4a64472f2c7 diff --git a/roms/seabios b/roms/seabios index 4adadbd..67d1fbe 160000 --- a/roms/seabios +++ b/roms/seabios @@ -1 +1 @@ -Subproject commit 4adadbde6904807de2e990c0af839ad0cc977806 +Subproject commit 67d1fbef0f630e1e823f137d1bae7fa5790bcf4e
Commit 02d38fcb2caa4454cf4ed728d5908c3cc9ba47be net: Update netdev peer on link change introduced a link state dependency between the backend netdev and the nic. If the admin turned off the link on the backend, the nic link state was set to down because the link had no access to the network at all. This created some inconsitet behavior for someone who wanted to play around the links states. 1) Turning off the nic link and then turning on the backend link (even if it was already on) would turn on the nic link again. 2) Turning off the backend link and then turning on the nic link would turn on the link in the VM, but would not change the backend state resulting in a broken/unreachable network. This patch attempts to correct these behaviors. The patch treats the nic-backend relationship as two ends of a wire. Each end tracks the administratively set link state in addition to actual link state. Thus is is possible to set link down at each end effectively emulating plugging/unplugging the wire at either end. The patch continues to preserve the old behavior where setting just nic side down does NOT impact the backend. Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> --- include/net/net.h | 1 + net/net.c | 25 +++++++++++++++++++++++-- roms/ipxe | 2 +- roms/openbios | 2 +- roms/seabios | 2 +- 5 files changed, 27 insertions(+), 5 deletions(-)