On Tue, Nov 6, 2012 at 11:32 AM, Leonardo Chiquitto
<leonardo.lists@xxxxxxxxx> wrote:
> On Wed, Oct 10, 2012 at 5:29 AM, Ian Kent <raven@xxxxxxxxxx> wrote:
>> On Thu, 2012-10-04 at 22:42 -0300, Leonardo Chiquitto wrote:
>>> Hi,
>>>
>>> From time to time I stumble with an instance of the automounter logging
>>> more messages than it normally does. These messages are the result of
>>> the expiration thread running and should appear every timeout/4 seconds
>>> once for each automounted path:
>>>
>>> automount[8430]: st_expire: state 1 path /s
>>> automount[8430]: expire_proc: exp_proc = 140472501561088 path /s
>>> automount[8430]: expire_cleanup: got thid 140472501561088 path /s stat 0
>>> automount[8430]: expire_cleanup: sigchld: exp 140472501561088
>>> finished, switching from 2 to 1
>>> automount[8430]: st_ready: st_ready(): state = 2 path /s
>>>
>>> In the cases I've found, the frequency of execution of the expiration thread
>>> is much higher than usual. I've debugged this today and discovered that
>>> every time a SIGHUP is sent to the daemon, a new timeout alarm is added.
>>> This alarm will then run forever every timeout/4 seconds. If you send many
>>> SIGHUPs you'll notice that soon enough the automounter will be running
>>> the expiration thread all the time.
>>>
>>> The code that adds this alarm is in do_readmap_cleanup():
>>>
>>> 323 static void do_readmap_cleanup(void *arg)
>>> 324 {
>>> 325 struct readmap_args *ra;
>>> 326 struct autofs_point *ap;
>>> 327
>>> 328 ra = (struct readmap_args *) arg;
>>> 329
>>> 330 ap = ra->ap;
>>> 331
>>> 332 st_mutex_lock();
>>> 333
>>> 334 ap->readmap_thread = 0;
>>> 335 st_set_done(ap);
>>> 336 if (!ap->submount)
>>> 337 alarm_add(ap, ap->exp_runfreq); <= here
>>> 338 st_ready(ap);
>>> 339
>>> 340 st_mutex_unlock();
>>> 341
>>> 342 free(ra);
>>> 343
>>> 344 return;
>>> 345 }
>>>
>>> Besides the SIGHUP case, this problem can also be triggered by adding a
>>> new key to a file map and then trying to mount it. The lookup failure will
>>> cause a map re-read and then a new alarm will be added.
>>>
>>> This part of the code is very old and I couldn't figure out why it needs to
>>> add the alarm at that point. Ian, could you help?
>>
>> But nevertheless there should be an alarm delete somewhere in there but
>> there isn't and the same applies to the prune (SIGUSR1) at least.
>
> Ian,
>
> What about the patch below?
>
> Leonardo
>
> Don't schedule new alarms after handling SIGHUP and SIGUSR1
>
> Currently, a new alarm is scheduled every time the daemon receives
> a SIGHUP (map re-read) or SIGUSR1 (forced expiration). Besides that,
> map re-reads started on demand when a map is found to be outdated
> also generate a new alarm.
>
> Once added, these alarms are never deleted and hence increase the
> number of times the daemon wakes up to run the expiration procedure.
> After a couple of months, in setups with many mount points, it's
> normal to see automount waking up every second to handle the
> expiration timer.
>
> This patch removes the alarm scheduling from the readmap cleanup
> routine and makes sure the alarm is re-added after the expiration
> process only when it was not triggered by SIGUSR1.
>
> I couldn't think of any use case to justify keeping these alarms:
> it's critical to have the alarm ticking every timeout/4 seconds,
> but more than one periodic alarm running doesn't seem to make
> sense.
>
> Index: autofs-5.0.7/daemon/state.c
> ===================================================================
> --- autofs-5.0.7.orig/daemon/state.c
> +++ autofs-5.0.7/daemon/state.c
> @@ -144,7 +144,7 @@ void expire_cleanup(void *arg)
> ap->submount = 2;
> }
>
> - if (!ap->submount)
> + if (ap->state == ST_EXPIRE && !ap->submount)
> alarm_add(ap, ap->exp_runfreq);
>
> /* FALLTHROUGH */
> @@ -326,17 +326,12 @@ static void do_readmap_cleanup(void *arg
> struct autofs_point *ap;
>
> ra = (struct readmap_args *) arg;
> -
> ap = ra->ap;
>
> st_mutex_lock();
> -
> ap->readmap_thread = 0;
> st_set_done(ap);
> - if (!ap->submount)
> - alarm_add(ap, ap->exp_runfreq);
> st_ready(ap);
> -
> st_mutex_unlock();
>
> free(ra);
> Index: autofs-5.0.7/CHANGELOG
> ===================================================================
> --- autofs-5.0.7.orig/CHANGELOG
> +++ autofs-5.0.7/CHANGELOG
> @@ -15,6 +15,7 @@
> - fix recursive mount deadlock.
> - increase file map read buffer size.
> - handle new location of systemd.
> +- don't reschedule alarm after signals.
>
> 25/07/2012 autofs-5.0.7
> =======================
Hello Ian,
Any comment on this patch?
Thanks and Happy New Year!
Leonardo
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
