[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virnetdevmacvlan.c: Introduce mutex for macvlan creation

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 {
-        retries = 5;
+        if (virNetDevMacVLanCreateMutexInitialize() < 0) {
+            virReportSystemError(errno, "%s", _("unable to init mutext"));


+            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;
+                }

                  if (do_retry && --retries)
-                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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]