| Message ID | 20201211083804.800-1-tiwai@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | alsactl: Skip restore during the lock | expand |
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)? thanks, Takashi
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. Jaroslav
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. Takashi
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
diff --git a/alsactl/state.c b/alsactl/state.c index ea1d3bcaaddf..8d1d814341bd 100644 --- a/alsactl/state.c +++ b/alsactl/state.c @@ -1663,7 +1663,12 @@ int load_state(const char *file, const char *initfile, int initflags, err = snd_input_stdio_attach(&in, stdin, 0); } else { lock_fd = state_lock(file, 10); - err = lock_fd >= 0 ? snd_input_stdio_open(&in, file, "r") : lock_fd; + /* skip restore if already locked; i.e. concurrent accesses */ + if (lock_fd < 0) { + err = 0; + goto out_global; + } + err = snd_input_stdio_open(&in, file, "r"); } if (err >= 0) { err = snd_config_load(config, in); @@ -1781,6 +1786,7 @@ single: err = finalerr; out: snd_config_delete(config); +out_global: snd_config_update_free_global(); return err; }
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. BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1179904 Signed-off-by: Takashi Iwai <tiwai@suse.de> --- alsactl/state.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)