Re: [Qemu-devel] [PATCH] net: Update netdev peer on link change

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Apr 02, 2014 at 11:25:32AM -0400, Vlad Yasevich wrote:
> On 04/02/2014 11:03 AM, Michael S. Tsirkin wrote:
> > On Wed, Apr 02, 2014 at 10:57:08AM -0400, Vlad Yasevich wrote:
> >> On 04/02/2014 06:46 AM, Yan Vugenfirer wrote:
> >>>
> >>> On Apr 2, 2014, at 11:51 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>
> >>>> On Thu, Nov 21, 2013 at 09:05:51PM -0500, Vlad Yasevich wrote:
> >>>>> When a link change occurs on a backend (like tap), we currently do
> >>>>> not propage such change to the nic.  As a result, when someone turns
> >>>>> off a link on a tap device, for instance, then a guest doesn't see
> >>>>> that change and continues to try to send traffic or run DHCP even
> >>>>> though the lower-layer is disconnected.  This is OK when the network
> >>>>> is set up as a HUB since the the guest may be connected to other HUB
> >>>>> ports too, but when it's set up as a netdev, it makes thinkgs worse.
> >>>>>
> >>>>> The patch addresses this by setting the peers link down only when the
> >>>>> peer is not a HUBPORT device.  With this patch, in the following config
> >>>>>  -netdev tap,id=net0 -device e1000,mac=XXXXX,netdev=net0
> >>>>> when net0 link is turned off, the guest e1000 shows lower-layer link
> >>>>> down. This allows guests to boot much faster in such configurations.
> >>>>> With windows guest, it also allows the network to recover properly
> >>>>> since windows will not configure the link-local IPv4 address, and
> >>>>> when the link is turned on, the proper address address is configured.
> >>>>>
> >>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>>>> ---
> >>>>> net/net.c | 26 +++++++++++++++++---------
> >>>>> 1 file changed, 17 insertions(+), 9 deletions(-)
> >>>>>
> >>>>> diff --git a/net/net.c b/net/net.c
> >>>>> index 0a88e68..8a084bf 100644
> >>>>> --- a/net/net.c
> >>>>> +++ b/net/net.c
> >>>>> @@ -1065,15 +1065,23 @@ void qmp_set_link(const char *name, bool up, Error **errp)
> >>>>>         nc->info->link_status_changed(nc);
> >>>>>     }
> >>>>>
> >>>>> -    /* Notify peer. Don't update peer link status: this makes it possible to
> >>>>> -     * disconnect from host network without notifying the guest.
> >>>>> -     * FIXME: is disconnected link status change operation useful?
> >>>>> -     *
> >>>>> -     * Current behaviour is compatible with qemu vlans where there could be
> >>>>> -     * multiple clients that can still communicate with each other in
> >>>>> -     * disconnected mode. For now maintain this compatibility. */
> >>>>
> >>>> Hmm this went in without much discussion.
> >>>>
> >>>> But before this change went in, it was possible to control link state on NIC
> >>>> and on link separately, without guest noticing.
> >>>>
> >>>> Why did we decide to drop this functionality?
> >>>
> >>> Not sure there was a real need for the patch.
> >>>
> >>> On other hand Windows guest will not receive IP address from DHCP server if you start VM with tap link down and then set it to up without toggling link state of the guest device as well.
> >>
> >> Same for a linux guest.  It a fault scenario.  So we either handle
> >> it by propagating the link event, or we forbid it.  Doing neither
> >> allows for a bad state in common configuration.
> > 
> > It's how networking works.
> > If I turn off my router at home I can't get dhcp either.
> 
> And you device link will show that it has not carrier.

It doesn't as long as the switch it's connected to is up.


> > 
> > We had a way to disable networking  at link or at
> > the router, then removed the ability to turn it
> > off at the router and I'm asking why.
> 
> We didn't remove the ability to turn it off at router/switch.
> We just now propagate carrier loss correctly.
> 
> In fact, if you have a configuration where the VM is connected
> to a hub in qemu, turning off the link on the 'tap' connected
> to the same hub doesn't not change guest state.

So why is it a good idea to make -netdev inconsistent
with this?
It doesn't seem to add anything useful.
It might fix things for users who turned off link
at the wrong place by mistake but I doubt it's
a common scenario.

Where end users getting it wrong and complaining?


> > 
> >> This patch chose to propagate the event under certain configuration.
> >>
> >> -vlad
> > 
> > So don't do this then.
> > If you want guest to know that link is down,
> > put it down on guest side.
> > 
> > It's that simple.
> > 
> 
> Sorry, but that's not always possible.  The host administrator
> may not always be vm administrator and without this patch vm
> administrator has no idea what happened to his network.
> 
> -vlad
> > 
> > 
> > 
> >>>
> >>> Yan. 
> >>>
> >>>>
> >>>>
> >>>>
> >>>>> -    if (nc->peer && nc->peer->info->link_status_changed) {
> >>>>> -        nc->peer->info->link_status_changed(nc->peer);
> >>>>> +    if (nc->peer) {
> >>>>> +        /* Change peer link only if the peer is NIC and then notify peer.
> >>>>> +         * If the peer is a HUBPORT or a backend, we do not change the
> >>>>> +         * link status.
> >>>>> +         *
> >>>>> +         * This behavior is compatible with qemu vlans where there could be
> >>>>> +         * multiple clients that can still communicate with each other in
> >>>>> +         * disconnected mode. For now maintain this compatibility.
> >>>>> +         */
> >>>>> +        if (nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
> >>>>> +            for (i = 0; i < queues; i++) {
> >>>>> +                ncs[i]->peer->link_down = !up;
> >>>>> +            }
> >>>>> +        }
> >>>>> +        if (nc->peer->info->link_status_changed) {
> >>>>> +            nc->peer->info->link_status_changed(nc->peer);
> >>>>> +        }
> >>>>>     }
> >>>>> }
> >>>>>
> >>>>> -- 
> >>>>> 1.8.4.2
> >>>>>
> >>>>
> >>>