Re: [PATCH] alsactl: Skip restore during the lock

[Takashi Iwai <tiwai@suse.de>]

On Fri, 11 Dec 2020 18:20:50 +0100,
Jaroslav Kysela wrote:
> 
> Dne 11. 12. 20 v 17:59 Takashi Iwai napsal(a):
> > 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)?
> 
> Maybe. It seems enough to add EEXIST errno check to the "if (errno == EBUSY ||
> errno == EAGAIN)" condition to repeat the open sequence. The -EBUSY will be
> returned correctly then. The one second delay is harmless in my eyes for the
> second task.

I'm afraid that a significant delay can be confusing.
And this should be the race only once, so no need to add the
artificial delay, IMO.

(BTW, now I noticed that we decrease timeout twice :)


Takashi