On 02/28/13 22:27, Eric Blake wrote:
On 02/28/2013 08:25 AM, Michal Privoznik wrote:
> Currently, after we removed the qemu driver lock, it may happen
> that two or more threads will start up a machine with macvlan and
> race over virNetDevMacVLanCreateWithVPortProfile(). However,
> there's a racy section in which we are generating a sequence of
> possible device names and detecting if they exits. If we found
> one which doesn't we try to create a device with that name.
> However, the other thread is doing just the same. Assume it will
> succeed and we must therefore fail. If this happens more than 5
> times (which in massive parallel startup surely will) we return
> -1 without any error reported. This patch is a simple hack to
> both of these problems. It introduces a mutex, so only one thread
> will enter the section, and if it runs out of possibilities,
> error is reported. Moreover, the number of retries is raised to 20.
> ---
>
> @@ -870,23 +880,39 @@ int virNetDevMacVLanCreateWithVPortProfile(const char
*tgifname,
> return -1;
> } else {
> create_name:
> - retries = 5;
> + if (virNetDevMacVLanCreateMutexInitialize() < 0) {
> + virReportSystemError(errno, "%s", _("unable to init
mutext"));
s/mutext/mutex/
> + return -1;
> + }
> +
> + retries = 20;
Do we really need to bump retries up, if the point of the mutex was to
prevent the need for that?
This will increase the chance we will succeed in a possible race with a
non-libvirt competitor, but it's not really needed for this fix.
On the other hand, as this fix is considered temporary and just done to
get the release out, I don't mind this change.
> + virMutexLock(&virNetDevMacVLanCreateMutex);
> for (c = 0; c < 8192; c++) {
The loop here is anyways trying a lot of stuff for possible existing
interfaces so the bump of the retry count can't hurt performance really
much.
> snprintf(ifname, sizeof(ifname), pattern, c);
> - if ((ret = virNetDevExists(ifname)) < 0)
> + if ((ret = virNetDevExists(ifname)) < 0) {
> + virMutexUnlock(&virNetDevMacVLanCreateMutex);
> return -1;
> + }
> if (!ret) {
> rc = virNetDevMacVLanCreate(ifname, type, macaddress, linkdev,
> macvtapMode, &do_retry);
> - if (rc == 0)
> + if (rc == 0) {
> + cr_ifname = ifname;
> break;
> + }
>
> if (do_retry && --retries)
> continue;
> - return -1;
> + break;
> }
> }
> - cr_ifname = ifname;
> +
> + virMutexUnlock(&virNetDevMacVLanCreateMutex);
> + if (!cr_ifname) {
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("Unable to create macvlan device"));
> + return -1;
> + }
Seems reasonable, but you might want a second reviewer since we are so
close to release.
The (temporary) fix seems okay to me. ACK if this gets cleaned up
properly after the release
Peter