The one thing we'd need to be careful about here is if there are
existing
cached dentries without the attributes we want, then we run through
nfs_advise_use_readdirplus(), setting the flag, then the
nfs_force_readdirplus() fails to drop all the existing dentries..
That looks like a `ls <dir>`, then a stat of one of those
cached dentries, then an `ls -la` would look like a flood of GETATTR for
every entry instead of a series of READDIRPLUS.
Any chance you still have that test system? What happens if you do:
ls <big dir>
stat <big dir>/file
ls -la <big dir>
I'm worried that will leave you stuck in the GETATTR flood instead of
using
the more optimal READDIRPLUS.
If I'm going to send this patch, I should also test it, so I suppose I
might
just build and test this myself..
Ben
On 18 Jul 2019, at 3:26, Noam Lewis wrote:
The patch seems to work nicely. getdents() doesn't get stuck /
iteration does not restart when accessing a non-cached file.
Thanks!
Can we agree this is the more correct behavior?
What's the next step?
On Wed, Jul 17, 2019 at 3:08 PM Benjamin Coddington
<bcodding@xxxxxxxxxx> wrote:
Maybe because we always drop the directory's page cache whenever we
decide
to switch to READDIRPLUS, even if we're already doing it.. I've not
tested
or checked very thoroughly if this is ok, but I think maybe want
something
like this:
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 57b6a45576ad..acfe47668238 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -438,8 +438,8 @@ void nfs_force_use_readdirplus(struct inode *dir)
struct nfs_inode *nfsi = NFS_I(dir);
if (nfs_server_capable(dir, NFS_CAP_READDIRPLUS) &&
- !list_empty(&nfsi->open_files)) {
- set_bit(NFS_INO_ADVISE_RDPLUS, &nfsi->flags);
+ !list_empty(&nfsi->open_files) &&
+ !test_and_set_bit(NFS_INO_ADVISE_RDPLUS,
&nfsi->flags))
{
invalidate_mapping_pages(dir->i_mapping, 0, -1);
}
}
Want to give that a spin?
Ben
On 17 Jul 2019, at 2:44, Noam Lewis wrote:
I'm starting to think this is a bug. I can't see a good reason why
accessing (stat) a directory entry should cause the READDIRPLUS
cookie
to be reset.
It seems that the trigger for iteration reset is accessing a
directory
entry that doesn't have a valid entry in the cache. If it does has a
valid cache entry it doesn't trigger the cookie reset. Note, it
doesn't matter if the entry (or traversed dir) has actually changed:
the reset occurs even if both did not change on the server side.
Setting actimeo to a large enough value allows the dir iteration to
complete without any resets, but this is just a workaround that
isn't
acceptable if the file system is being modified or if there isn't
enough memory. It's also heuristic and can lead to unexpected
hiccups
if something in the environment changes.
So my questions still stand: is this expected behavior? What's the
reason?
P.S. I'm using NFSv3
On Mon, Jul 15, 2019, 08:56 Noam Lewis <noam.lewis@xxxxxxxxxxxxxx>
wrote:
I've encountered a problem while iterating large directories via an
NFS mount.
Scenario:
1. Linux NFS client iterates a directory with many (millions) of
files, e.g. via getdents() until all entries are done. In my case,
READDIRPLUS is being used under the hood. Trivial reproduction is
to
run: ls -la
2. At the same time, run the stat tool on a file inside that
directory.
The directory on the server is not being modified anywhere (on this
client or any other client).
Result: the next or ongoing getdents will get stuck for a long time
(tens of seconds to minutes). It appears to be re-iterating some of
the work it already did, by going back to a previous NFS
READDIRPLUS
cookie.
Things I've tried as workarounds:
- Mounting with nordirplus - the iteration doesn't seem to reset or
at
least getdents doesn't get stuck, but now I have tons of LOOKUPs,
as
expected.
- Setting actimeo=(large number) doesn't affect the behavior
Questions:
1. Why does the stat command cause this?
2. How can I avoid the reset, i.e. ensure forward progress of the
dir
iteration?
