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

[Peter Krempa]

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