New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
init: fixes file descriptor accounting #30065
base: master
Are you sure you want to change the base?
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
src/init.cpp
Outdated
nMaxConnections = std::max(std::min<int>(nMaxConnections, fd_max - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE), 0); | ||
if (nFD < MIN_CORE_FILEDESCRIPTORS) | ||
return InitError(_("Not enough file descriptors available.")); | ||
nMaxConnections = std::min(nFD - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE, nMaxConnections); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice how the latter nMaxConnections
trim is redundant once we limit nFD = std::min(FD_SETSIZE, nFD);
. Moving things around, plus accounting for nBind
(which is currently not being accounted for in master), we get:
nMaxConnections = std::max(std::min<int>(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE), 0);
nMaxConnections = std::min(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE,);
Concept ACK |
I think it is worth mentioning that the current approach is defining the minimum amount of file descriptions required without accounting for a single connection*, this means that if we are at the bare minimum, we will run but won't be able to connect to any nodes. This is consistent with the current logic in master, but I think it's not the way it should be. I'm happy to add another commit addressing this, but I've rathered start approaching this sticking to the same assumptions as master. * This actually accounts for manual connections, which may never happen, but not to any of our outbound. If we happen to not create any manuals we'd have 8 slots ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
Concept ACK, I stumbled on this recently in #29415 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/init.cpp
Outdated
#else | ||
int fd_max = FD_SETSIZE; | ||
// Reserve enough FDs to account for the bare minimum, plus any manual connections, plus the bound interfaces | ||
int nMinRequiredFds = MIN_CORE_FDS + MAX_ADDNODE_CONNECTIONS + nBind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to doc/developer-notes.md
variables should use snake_case and no need to denote the type in the variable name (n
for integer): min_required_fds
.
But more importantly, the name nMinRequiredFds
is misleading because we accept a smaller number - a bit below we check if (nFD < MIN_CORE_FDS) return error...
, so it is not "minimum required". I think this variable here should be: min_required_fds = MIN_CORE_FDS + nBind
, that check should be if (nFD < min_required_fds) return error
and then:
- nFD = RaiseFileDescriptorLimit(nMaxConnections + nMinRequiredFds);
+ nFD = RaiseFileDescriptorLimit(min_required_fds + MAX_ADDNODE_CONNECTIONS + nMaxConnections);
and
- nMaxConnections = std::max(std::min<int>(nMaxConnections, nFD - nMinRequiredFds), 0);
+ nMaxConnections = std::max(std::min<int>(nMaxConnections, available_fds - min_required_fds - MAX_ADDNODE_CONNECTIONS), 0);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the if check should be updated, but I don't necessarily agree with splitting MAX_ADDNODE_CONNECTIONS
from min_required_fds
.
There is the outstanding question of whether the minimum required fds to run Core should account for some of the connections. Currently, we are not accounting for any (given the check is performed against MIN_CORE_FDS
), but I think we should be accounting for at least, outbound + manuals
src/init.cpp
Outdated
int nMinRequiredFds = MIN_CORE_FDS + MAX_ADDNODE_CONNECTIONS + nBind; | ||
|
||
// Try raising the FD limit to what we need (nFD may be smaller than the requested amount if this fails) | ||
nFD = RaiseFileDescriptorLimit(nMaxConnections + nMinRequiredFds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snake_case for variables. I think available
or available_fds
is a good name:
nFD = RaiseFileDescriptorLimit(nMaxConnections + nMinRequiredFds); | |
available_fds = RaiseFileDescriptorLimit(nMaxConnections + nMinRequiredFds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept nFD
because it is not locally defined, but defined in the namespace, so at least one other method is using it. If we don't mind a bigger diff I'm happy to change it
We are computing our file descriptors limits based on whether we use poll or select. However, we are taking that into account only partially (subtracting from fd_max in one case, but from nFD later on). Moreover, nBind is also only accounted for partially. Simplify and fix this logic
Thanks for the suggestions @vasild, I've partially covered them and commented on some of the pending ones |
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
The current logic for file descriptor accounting is pretty convoluted and hard to follow. This is partially caused by the lack of documentation plus non-intuitive variable naming (which was more intuitive when fewer things were accounted for, but hasn't aged well). This has led to this accounting being error-prone and hard to maintain (as shown in he previous commit). Redefine some of the constants, plus document what are we accounting for so this can be extended more easily Remove FreeBSD workaround to bitcoin#2695
The current logic for file descriptor accounting is pretty convoluted and hard to follow. This is partially caused by the lack of documentation plus non-intuitive variable naming (which was more intuitive when fewer things were accounted for, but
hasn't aged well). This has led to this accounting being error-prone and hard to maintain (as shown in the first commit of this PR).
Redefine some of the constants, plus document what are we accounting for so this can be extended more easily
Fixes #18911