[PATCH] alsactl: Skip restore during the lock

[Jaroslav Kysela]

Dne 11. 12. 20 v 19:44 Takashi Iwai napsal(a):
> On Fri, 11 Dec 2020 18:56:56 +0100,
> Jaroslav Kysela wrote:
>>
>> Dne 11. 12. 20 v 18:37 Takashi Iwai napsal(a):
>>> On Fri, 11 Dec 2020 18:23:03 +0100,
>>> Jaroslav Kysela wrote:
>>>>
>>>> Dne 11. 12. 20 v 18:06 Takashi Iwai napsal(a):
>>>>> On Fri, 11 Dec 2020 17:59:05 +0100,
>>>>> Takashi Iwai wrote:
>>>>>>
>>>>>> On Fri, 11 Dec 2020 17:45:45 +0100,
>>>>>> Jaroslav Kysela wrote:
>>>>>>>
>>>>>>> Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a):
>>>>>>>> Currently alsactl-restore tries to initialize the device when an error
>>>>>>>> is found for restore action.  But this isn't the right behavior in the
>>>>>>>> case where the lock is held; it implies that another alsactl is
>>>>>>>> running concurrently, hence you shouldn't initialize the card at the
>>>>>>>> same time.  The situation is found easily when two alsactls get
>>>>>>>> started by both udev and systemd (note that those two invocations are
>>>>>>>> the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules
>>>>>>>> for details).
>>>>>>>>
>>>>>>>> This patch changes load_state() not to handle the initialization if
>>>>>>>> the locking fails.
>>>>>>>
>>>>>>> The operation should serialize in this case (there's limit of 10 seconds which
>>>>>>> should be enough to finish the initialization). The state_lock() function
>>>>>>> should return -EBUSY when the file is locked (and I'm fine to change the
>>>>>>> behaviour from 'init' to 'skip' for this lock state).
>>>>>>>
>>>>>>> It seems that -EEXIST is returned when the lock file exists, but the
>>>>>>> open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access
>>>>>>> this file when another user owns the file.
>>>>>>>
>>>>>>> But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and
>>>>>>> /lib/systemd/system/alsa-restore.service should be run as root, right?
>>>>>>
>>>>>> Yes, it should be root.
>>>>>>
>>>>>> I also wondered how EEXIST comes, too.  Maybe it's also the race
>>>>>> between the first open(O_RDWR) and the second
>>>>>> open(O_RDWR|O_CREAT|O_EXCL)?  If so, it'd be better to go back again
>>>>>> to the normal open(O_RDWR)?
>>>>>
>>>>> ... something like below
>>>>>
>>>>>
>>>>> diff --git a/alsactl/lock.c b/alsactl/lock.c
>>>>> index 4a485392b3bd..c1c30f0c5eee 100644
>>>>> --- a/alsactl/lock.c
>>>>> +++ b/alsactl/lock.c
>>>>> @@ -64,6 +64,9 @@ static int state_lock_(const char *file, int lock, int timeout, int _fd)
>>>>>  				if (errno == EBUSY || errno == EAGAIN) {
>>>>>  					sleep(1);
>>>>>  					timeout--;
>>>>> +				} if (errno == EEXIST){
>>>>> +					/* race at creating a lock, try again */
>>>>> +					continue;
>>>>>  				} else {
>>>>>  					err = -errno;
>>>>>  					goto out;
>>>>
>>>> If we don't use the sleep call and the timeout counter, there's endless CPU
>>>> busy loop when the root creates the lock file and user wants to access it for
>>>> example. It's better to add EEXIST to the previous errno condition.
>>>
>>> The timeout is decreased in the while condition.
>>
>> It seems not correct. It decreases the wait time to half. My fault :-(
>>
>> What about this straight change:
>>
>> --- a/alsactl/lock.c
>> +++ b/alsactl/lock.c
>> @@ -63,11 +63,15 @@ static int state_lock_(const char *file, int lock, int
>> timeout, int _fd)
>>                         if (fd < 0) {
>>                                 if (errno == EBUSY || errno == EAGAIN) {
>>                                         sleep(1);
>> -                                       timeout--;
>> -                               } else {
>> -                                       err = -errno;
>> -                                       goto out;
>> +                                       continue;
>>                                 }
>> +                               if (errno == EEXIST) {
>> +                                       fd = open(nfile, O_RDWR);
>> +                                       if (fd >= 0)
>> +                                               break;
>> +                               }
>> +                               err = -errno;
>> +                               goto out;
>>                         }
>>                 }
>>         }
> 
> Yes, this should work.  Shall I resubmit?  I'd split to two, one to
> correct doubly timeout decreases and another to handle EEXIST.

Yes, thanks.

				Jaroslav

-- 
Jaroslav Kysela <perex at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.