On Tue, Apr 02, 2019 at 09:22:38PM -0400, Laine Stump wrote:
> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> > The current qemu driver code for changing bandwidth on a NIC first asks
> > the network driver if the change is supported, then changes the
> > bandwidth on the VIF, and then tells the network driver to update the
> > bandwidth on the bridge.
> >
> > This is potentially racing if a parallel API call causes the network
> > driver to allocate bandwidth on the bridge between the check and the
> > update phases.
> >
> > Change the code to just try to apply the network bridge update
> > immediately and rollback at the end if something failed.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > @@ -5391,6 +5334,7 @@ networkBandwidthUpdate(virDomainNetDefPtr iface,
> > unsigned long long tmp_floor_sum;
> > virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
> > unsigned long long new_rate = 0;
> > + unsigned long long old_floor, new_floor;
> > int plug_ret;
> > int ret = -1;
> > @@ -5400,7 +5344,21 @@ networkBandwidthUpdate(virDomainNetDefPtr iface,
> > return -1;
> > }
> > - if (!networkBandwidthGenericChecks(iface, newBandwidth))
> > + if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
> > + iface->data.network.actual->data.bridge.brname == NULL) {
> > + /* This is not an interface that's plugged into a network.
> > + * We don't care. Thus from our POV bandwidth change is allowed. */
>
>
> This comment is more confusing than it is helpful. Especially since it's
> wrong :-)
It was correct originally, then i made bandwidth stuff apply to any
bridged nic.
I'll s/into a network/into a bridge/
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index b81c411007..baf188ae40 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -11729,17 +11729,17 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
> > }
> > if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> > - !virDomainNetBandwidthChangeAllowed(net, newBandwidth))
> > + virDomainNetBandwidthUpdate(net, newBandwidth) < 0)
> > goto endjob;
> > if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
> > - !virDomainNetTypeSharesHostView(net)) < 0 ||
> > - (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> > - virDomainNetBandwidthUpdate(net, newBandwidth) < 0)) {
> > + !virDomainNetTypeSharesHostView(net)) < 0) {
> > ignore_value(virNetDevBandwidthSet(net->ifname,
> > net->bandwidth,
> > false,
> > !virDomainNetTypeSharesHostView(net)));
> > + ignore_value(virDomainNetBandwidthUpdate(net,
> > + net->bandwidth));
> > goto endjob;
> > }
>
>
> The new version of the code is calling virDomainNetBandwidthUpdate() and
> virNetDevBandwidthSet() in reverse order from what it previously did. If
> either of the functions were to modify newBandwidth's contents, then the
> results would change. Fortunately it looks like neither of them do that, so
> I think we're safe.
Yes, the previous code did a two phase change. It first checked if change
was allowed by the network, then changed the local NIC & updated the
network.
That's a time of check vs time of update race wrt to the network, so I
had to merge them into one step.
Which ever way we update the local NIC vs the network has unplesant
failure scenarios, where we can merely do a best effort to roll back.
I think this is as good as we'll get.
> Reviewed-by: Laine Stump <laine@xxxxxxxxx>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
