Hi Jens,
> after some weeks of running a stable service with this patch, I'd like
> to propose merging this (we'd really like to switch back to an unpatched
> Cyrus release in future).
This would be good. Can you attach it as a patch file please? CC me
and I'll get it merged.
Thanks for your hard work figuring this out. :)
Cheers,
ellie
On Mon, Apr 11, 2016, at 05:52 PM, Jens Erat via Cyrus-devel wrote:
> Hi all,
>
> after some weeks of running a stable service with this patch, I'd like
> to propose merging this (we'd really like to switch back to an unpatched
> Cyrus release in future). This is not only an issue to us, but we know
> some other universities running similar Solaris setups and waiting to
> upgrade to Cyrus 2.5 because of our issues.
>
> This is only an issue with Cyrus IMAP 2.5 and probably 3.0; Cyrus 2.4
> also does not properly repeat select if it failed, but it's not an issue
> here as interrupts can occur anywhere.
>
> We didn't have those master loop hick ups any more. In fact, the while
> loop even seems superfluous; just running myselect again if `r==0` would
> be fine, but this might be different with other combinations of
> operating system kernels, C libraries and compilers, and the loop seems
> the safer thing to do. The limit of 100 retries seems a little bit
> arbitrary, but I can't really argument for any number, something around
> five should probably also be fine to cover all scenarios that are not
> completely broken, anyway.
>
> We dumped the interrupted and again counters each time the master loop
> ran (and several other metrics), and never had more than one EINTR or
> EAGAIN in a row (so the next `myselect` immediately returned the FDSET).
>
> Kind regards from Lake Constance, Germany,
> Jens
>
> Am 10.02.2016 um 13:39 schrieb Jens Erat via Cyrus-devel:
> > Hi Bron,
> >
> > digging deeper, we realized there are still issues with the loop you
> > proposed. `select` will only decrement tvptr on Linux (and maybe _some_
> > other systems, definitely not all of them), pselect will never do -- you
> > might end up waiting forever here if nothing happens.
> >
> > On the other hand, we partially got a grasp of understanding what was
> > going on. Because when `pselect` is available, signalling was disabled
> > but during `pselect`, thus with some weird combination of load on the
> > different services, each `pselect` returned with `EINTR` and the message
> > handling code was never reached
> >
> > We ended up putting the whole deadline calculation in a queue, and have
> > this code live at the moment:
> >
> > int interrupted = 0;
> > int again = 0;
> > do {
> > /* how long to wait? - do now so that any scheduled wakeup
> > * calls get accounted for*/
> > gettimeofday(&now, 0);
> > tvptr = NULL;
> > if (schedule && !in_shutdown) {
> > double delay = timesub(&now, &schedule->mark);
> > if (!interrupted && delay > 0.0) {
> > timeval_set_double(&tv, delay);
> > }
> > else {
> > tv.tv_sec = 0;
> > tv.tv_usec = 0;
> > }
> > tvptr = &tv;
> > }
> >
> > #if defined(HAVE_UCDSNMP) || defined(HAVE_NETSNMP)
> > if (tvptr == NULL) blockp = 1;
> > snmp_select_info(&maxfd, &rfds, tvptr, &blockp);
> > #endif
> > errno = 0;
> > r = myselect(maxfd, &rfds, NULL, NULL, tvptr);
> >
> > if (r == -1) switch(errno) {
> > /* Try again to get valid rfds, this time without blocking so we
> > * will definitely process messages without getting interrupted
> > * again. */
> > case EINTR:
> > interrupted++;
> > if (interrupted > 100) {
> > syslog(LOG_WARNING, "Repeatedly interrupted, too
> > many signals?");
> > /* Fake a timeout */
> > r = 0;
> > FD_ZERO(&rfds);
> > }
> > break;
> > /* Try again. */
> > case EAGAIN: again++; break;
> > /* uh oh */
> > default: fatalf(1, "select failed: %m");
> > }
> > } while (r == -1);
> >
> > We also dump interrupted and again together with some other variables at
> > the end of each loop. The server is running fine for two days now, and
> > the dumped information looks fine (we never experienced any delays >4
> > seconds since patching the server, we often found the master loop not
> > completely running for minutes). I will report again if we didn't suffer
> > from any interruptions for the next one or two weeks.
> >
> > Some remarks to the code:
> >
> > - We need to update the current timestamp `now` for decrementing the
> > passed time.
> > - `snmp_select_info` or's its FDs to rfds; thus it may be run repeatedly
> > to update `tvptr` and not run over any SNMP events.
> > - We are compiling without SNMP, thus this code is untested.
> > - If interrupted, we try again to receive FDs, but without waiting
> > (tvptr set to zero). Because of this, no large interruptions should
> > occur -- but we also won't skip over message handling when flooded with
> > signals.
> > - After 100 retries after being interrupted, we empty rfds and continue
> > to work, but print a warning. This is probably subject of discussion and
> > _much_ too large: we're currenlty dumping the number of interruptions,
> > and never had more than one at all before catching file descriptors (or
> > none).
> > - `int again` can and should be removed for live code, does not seem
> > relevant at all (but for our own debug purpose).
> >
> > Kind regards from Lake Constance, Germany,
> > Jens
> >
> >
> > Am 06.02.2016 um 13:00 schrieb Bron Gondwana:
> >> I could buy repeating the whole thing for EINTR but not for EAGAIN.
> >> Obviously everything is fine under non-heavy load no matter what.
> >>
> >> I'm pretty sure your patch is wrong, because it will process the fdset
> >> that the select was supposed to fill even if the select returned an error.
> >>
> >> You may have convinced me that my patch is wrong as well, and that we have
> >> to always check for the in_shutdown and sigquit magic in that tight loop
> >> in case it was a signal that needs to be responded to.
> >>
> >> Bron.
> >>
> >> On Sat, Feb 6, 2016, at 20:52, Jens Erat wrote:
> >>> Hi Bron,
> >>>
> >>> thank you for the fast reply.
> >>>
> >>>> One thing I would suggest is running something like nginx in front of
> >>> Cyrus, even if you only need it for authentication checks.
> >>>
> >>> We'll discuss this internally.
> >>>
> >>>> I believe this is the correct patch. If the select was interrupted
> >>> then it needs to be re-run, but there is no need to repeat all the other
> >>> work, so:
> >>>>
> >>>> - r = myselect(maxfd, &rfds, NULL, NULL, tvptr);
> >>>> - if (r == -1 && errno == EAGAIN) continue;
> >>>> - if (r == -1 && errno == EINTR) continue;
> >>>> + do {
> >>>> + r = myselect(maxfd, &rfds, NULL, NULL, tvptr);
> >>>> + } while (r == -1 && (errno == EAGAIN || errno == EINTR));
> >
>
> --
> Jens Erat
> Universität Konstanz
> Kommunikations-, Infomations-, Medienzentrum (KIM)
> Abteilung Basisdienste
> D-78457 Konstanz
> Mail: jens.e...@uni-konstanz.de
>
> Email had 1 attachment:
> + smime.p7s
> 7k (application/pkcs7-signature)
