Skip to content
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

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

clhedrick
Copy link

@clhedrick clhedrick commented Dec 3, 2024

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@clhedrick
Copy link
Author

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.

@clhedrick clhedrick force-pushed the zfs-2.2.7-staging branch 2 times, most recently from 6b49d0c to 9eb5b62 Compare December 3, 2024 18:32
@clhedrick
Copy link
Author

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.

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Dec 3, 2024
@amotin
Copy link
Member

amotin commented Dec 3, 2024

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>
@tonyhutter tonyhutter merged commit 310c08b into openzfs:zfs-2.2.7-staging Dec 4, 2024
19 checks passed
@clhedrick
Copy link
Author

FYI, we're now running with this patch on a staff file server.

anodos325 pushed a commit to truenas/zfs that referenced this pull request Dec 6, 2024
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>
anodos325 pushed a commit to truenas/zfs that referenced this pull request Dec 6, 2024
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>
arter97 pushed a commit to arter97/zfs that referenced this pull request Dec 9, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants