-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
add get_name implementation for exports #16833
add get_name implementation for exports #16833
Conversation
daffed5
to
d4758df
Compare
I wanted to make sure the code was doing the right thinig. I have little idea how I'd tell whether directories are getting reconnected, but I used bpftrace to check expfs.c: reconnect_one. exportfs_get_name can't be traced. But it should call zpl_get_name, so I traced it, including a kernel backtrace. it gets called from reconnect_one, and returns 0, which I assume is no error. It then calls lookup_one_len_unlocked. I traced it, and it does get called from the right place with valid directory names, which would have to have come from the zpl_get_name. When I look at the output of reconnect_one it is properly returning the parent directories. So as far as I can tell things are working. |
6b49d0c
to
9eb5b62
Compare
I tried the test given in #16254, which reproduces the problem. This patch is a bit better than the one I was using, though this still looks like a performance issue. I don't think zfs can do it any better given the nfsd design and the relatively slow speed on searching a directory for a specific value. The previous patch did move us from having hours of unacceptable load to brief spikes. This should do a bit better. In addition to the previous test with a directory having 200,000 subdirectories, I tried one with 500,000 subdirectories. The time to look up a file when it's cached on the client and not the server does seem to be roughly proportional to the size of the directory, as I had conjectured. |
BTW, it would be nice to have first line of commit message shorter and self-sufficient, so that it would look better in a short list. |
This fixes a serious performance problem with NFS handling of large directories, as the new get_name code is much more efficient than the default zfs_readdir. This is actually part of 20232ec in 2.3. But I've taken only the minimum code to implement get_name, and not the rest of the long name changes. Signed-off-by: Charles Hedrick <hedrick@rutgers.edu>
9eb5b62
to
f0681f0
Compare
FYI, we're now running with this patch on a staff file server. |
This fixes a serious performance problem with NFS handling of large directories, as the new get_name code is much more efficient than the default zfs_readdir. This is actually part of 20232ec in 2.3. But I've taken only the minimum code to implement get_name, and not the rest of the long name changes. Signed-off-by: Charles Hedrick <hedrick@rutgers.edu> Co-authored-by: Charles L. Hedrick <hedrick@ncommunis.cs.rutgers.edu> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Tony Hutter <hutter2@llnl.gov>
This fixes a serious performance problem with NFS handling of large directories, as the new get_name code is much more efficient than the default zfs_readdir. This is actually part of 20232ec in 2.3. But I've taken only the minimum code to implement get_name, and not the rest of the long name changes. Signed-off-by: Charles Hedrick <hedrick@rutgers.edu> Co-authored-by: Charles L. Hedrick <hedrick@ncommunis.cs.rutgers.edu> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Tony Hutter <hutter2@llnl.gov>
This fixes a serious performance problem with NFS handling of large directories, as the new get_name code is much more efficient than the default zfs_readdir. This is actually part of 20232ec in 2.3. But I've taken only the minimum code to implement get_name, and not the rest of the long name changes. Signed-off-by: Charles Hedrick <hedrick@rutgers.edu> Co-authored-by: Charles L. Hedrick <hedrick@ncommunis.cs.rutgers.edu> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Motivation and Context
This should resolve a serious NFS performance problem, as described in #16318
Description
Retrofit implementation of the get_name interface for nfs export, based on patch 20232ec from 2.3. That patch is much larger, as it implements long names. This PR does not implement long names, but takes the minimum needed to implement get_name.
How Has This Been Tested?
Tested on a test server. Note that the code affects only NFS accesses where the client has data cached and the server does not. The simplest approach is to create a directory hierarchy from the client and then clear cache on the server. At that point simply doing ls -lR on the hiearchy will execute the new code. I verified that the code is actually being called using bpftrace on zpl_get_name.
It's very unlikely that CI tests will exercize this code (either here or in 2.3 RC).
Types of changes
Checklist:
Signed-off-by
.