Bug 251368
| Summary: | rpc.mountd crashes with particular netgroup setup | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Jeff Layton <jlayton> |
| Component: | nfs-utils | Assignee: | Jeff Layton <jlayton> |
| Status: | CLOSED UPSTREAM | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | coughlan, staubach, steved, tao |
| Target Milestone: | --- | ||
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2007-09-28 15:54:49 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
| Attachments: | |||
|
Description
Jeff Layton
2007-08-08 16:08:51 UTC
Cloning for fedora to track upstream work... Created attachment 160915 [details]
patch 1 -- make m_hostname a dynamically allocated string
This patch just makes it so that m_hostname is a dynamically allocated string,
rather than a fixed length array of NFSCLNT_IDMAX.
Created attachment 160916 [details]
patch 2 -- make e_hostname a dynamically allocated string
This makes the exportent.e_hostname a dynamically allocated string. This needs
to be done since we copy m_hostname into it in some places.
Created attachment 160917 [details]
patch3 -- make a client_resolve function and have client_compose take a hostent
This patch may not be strictly needed, but it should get rid of one set of
hostname resolution from the auth_authenticate codepath. Rather than resolving
the name in auth_authenticate and then doing it again in client_compose, we
just use reuse the first one in client_compose.
These patches should get rid of the userspace limitations. It also adds a bit of optimization. The current code does a lot of string copying that can be avoided, and also should help eliminate some extra hostname resolution. We should be able to have arbitrarily long m_hostname strings with this. I believe, however, we have a limitation on how long this string can be when it's written to the kernel cache. So the big issue is what to do now: 1) hash these strings and use the hashes as cache keys, but that means pulling in a hashing routine (maybe libcrypt.a?). This is also doesn't deal with another problem which is that when using a lot of netgroups, you have to resolve them any that are listed in any export. 2) use Neil B's idea and use the ipaddr as a cache key. This means some duplication in the export cache, but maybe that's acceptable to cut down on the number of netgroup lookups. Still investigating... Reproducer is apparently here. Though by inspection, I can see what the issue is I think... https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=160598 Created attachment 161172 [details]
patch 4 -- add "use_ipaddr" command line option
This patch adds a use_ipaddr command-line option and makes it so that ipaddr
maps to ipaddr in auth.unix.ip cache when it's enabled. When we go to look up
in the nfsd.export cache, we have an IP address and a path. mountd can then
just compare the ip address/hostname to entries that match the path. In
addition to working around the string length limitations of the kernel caches,
this should prevent the (rather severe) performance penalty incurred when
dealing with an export table that contains a large number of netgroups.
As a side note, NFSCLNT_IDMAX is set to 1024, but that seems to be a number that was grabbed out of thin air. The actual string length limitation seems to be ~945 or so (between 940 and 949, determined emprirically). I suspect that the total string length of a cache line maxes out at 1024). I'm not thrilled with using a command-line flag for this, but I don't see a great way to automatically switch between the two modes. I need to do some more testing and then I'll post it to nfs list as an RFC. Perhaps someone there will have a good way to switch between the two modes. My testing hasn't shown any obvious problems. Sent patches to upstream list as an RFC to see if the general approach is valid. Awaiting feedback... Created attachment 161874 [details]
patch4 -- add new "mode" for mountd to change how ipaddrs are mapped
Neil Brown commented that the patches looked good to him. My testing has been
successful with them so far, so I've told him to go ahead and commit the first
3 patches as is.
I've respun the 4th patch though to make it so that the "use_ipaddr" mode gets
enabled automatically when the list of netgroups is more than 1/2 of
NFSCLNT_IDMAX. This is the explanation I sent with the upstream patch:
----------[snip]-----------
If the total length of all m_hostnames in the MCL_NETGROUP list is more
than half NFSCLNT_IDMAX, then switch to the "use_ipaddr" mode.
I considered doing what Neil suggested and just using all of the
m_hostnames from all of the clientlists. Consider however a
diskless client setup where someone has a ton of exports all
exported to different IP addresses. We probably don't want to enable
this there, but it would probably be enabled if we based this on
all of the m_hostnames.
I figure someone will want to enable this new mode for 2 reasons:
1) a client is likely to overflow NFSCLNT_IDMAX (which is a decent
approximation of the max length of a line in the kernel cache)
...or...
2) we have a lot of netgroups in /etc/exports and resolving them all
for a single mount is prohibitively expensive
I think the only reasonable way to end up in situation #1 is if you
are dealing with a lot of netgroups. A client isn't likely to match
a bunch of different MCL_WILDCARD entries (though maybe it can), nor is
it likely to match a bunch of different MCL_SUBNET entries. So it
really only makes sense to pay attention to the MCL_NETGROUP list
for this.
Why NFSCLNT_IDMAX/2? No particular reason. I figured that would mean
that we had a lot of netgroups, but still had enough space in the
string to allow for non-netgroup matches.
-------[snip]----------
Waiting for upstream comments at the moment. Ideally we'd like to make this not
need a new command-line option at all. I think that would entail getting rid of
the "old" behavior altogether, but Neilb seems to want to keep it since it does
reduce the size of the kernel caches. So we may be stuck with a new
command-line option unless he can be convinced.
New patchset posted upstream. Awaiting comments/commits... Patchset is now upstream in nfs-utils git tree. There is still some ongoing discussion about whether we should also include a commandline option to turn the new behavior on and off, but I'm going to close this as UPSTREAM for now. |