Bug 5312: Startup aborts if OPEN_MAX exceeds RLIMIT_NOFILE #1551
Conversation
|
Can one of the admins verify this patch? |
|
More details are explained in https://bugs.squid-cache.org/show_bug.cgi?id=5312 |
squid-anubis
added
the
M-failed-description
|
OK to test |
yadij
changed the title
|
The force push just fixed the MAX_OPEN -> OPEN_MAX typo in the commit title. |
@alanc, just FYI: We do not use PR branch commit titles and messages when merging changes into master. Anubis, the bot automating such merging, uses GitHub PR title and PR description for the squashed master commit. We need to add an automated greeting message to explain that and other caveats to new contributors... |
There was a problem hiding this comment.
Thank you for this bug fix!
I cannot object to this fix because it makes some things better without making other things worse, but I think we should finish this update by addressing several closely related problems, as detailed in my change requests. I can implement those change requests if you prefer -- just let me know! Both look trivial to me, but I may be missing some hidden caveats.
rousskov
added
the
S-waiting-for-author
Also partially addresses https://bugs.squid-cache.org/show_bug.cgi?id=5314 by replacing the devpoll_state array (that was sized to SQUID_MAXFD) with use of the epoll_state member of the fde structure, to match ModEpoll, but doesn't solve the problem of devpoll_fd not being opened in that path.
rousskov
added
S-waiting-for-reviewer
Also partially addresses https://bugs.squid-cache.org/show_bug.cgi?id=5314 by replacing the devpoll_state array (that was sized to SQUID_MAXFD) with use of the epoll_state member of the fde structure, to match ModEpoll, but doesn't solve the problem of devpoll_fd not being opened in that path.
There was a problem hiding this comment.
FWIW, I am quite surprised by the recent PR updates. I expected this PR to do what its current description says -- use Squid_MaxFD for everything.
I hope this PR does not need to make all these complex decisions to fully address Bug 5312 (i.e. it is sufficient to just make all the three arrays Squid_MaxFD in size), but I may be missing something.
I left a few specific questions to facilitate progress. At the end of the day, if there are good reasons to make those complex decisions in this PR, we will need to document, via C++ comments, the rationale behind those new constants (i.e., why "256" and why "1024").
rousskov
added
S-waiting-for-author
The DP_POLL ioctl on Solaris will fail with EINVAL if the number of fds specified is higher than the current RLIMIT_NOFILE setting. Fortunately, squid already puts the RLIMIT_NOFILE value in the Squid_MaxFD variable, so we just use that instead. Signed-off-by: Alan Coopersmith <[email protected]>
rousskov
changed the title
There was a problem hiding this comment.
Thank you for addressing my concerns. I have adjusted PR title/description to supply (assumed) logged symptoms and fix formatting.
I am approving this PR and requesting @yadij re-review (I believe his earlier concern has been addressed as well).
rousskov
added
S-waiting-for-reviewer
squid-anubis
removed
the
M-failed-description
yadij
added
M-cleared-for-merge
FATAL: Event loop exited with failure The DP_POLL ioctl on Solaris fails with EINVAL when the number of supplied descriptors (OPEN_MAX) is higher than the current RLIMIT_NOFILE setting. When it comes to the maximum number of descriptors, we should use Squid_MaxFD (which already reflects RLIMIT_NOFILE setting).
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
added
M-merged
The DP_POLL ioctl on Solaris fails with EINVAL when the number of
supplied descriptors (OPEN_MAX) is higher than the current RLIMIT_NOFILE
setting. When it comes to the maximum number of descriptors, we should
use Squid_MaxFD (which already reflects RLIMIT_NOFILE setting).