Fix up the regression introduced by commit 311324ad1713, and then
improve the READDIRPLUS hinting.
Trond Myklebust (3):
NFS: Fix a performance regression in readdir
NFS: Be more targeted about readdirplus use when doing
lookup/revalidation
NFS: Allow getattr to also report readdirplus cache hits
fs/nfs/dir.c | 46 +++++++++++++++++++++-------------------------
fs/nfs/inode.c | 21 +++++++++++++++++----
fs/nfs/internal.h | 1 +
3 files changed, 39 insertions(+), 29 deletions(-)
--
2.9.3
There is little point in setting NFS_INO_ADVISE_RDPLUS in nfs_lookup and
nfs_lookup_revalidate() unless a process is actually doing readdir on the
parent directory.
Furthermore, there is little point in using readdirplus if we're trying
to revalidate a negative dentry.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 9220bc7b9cd7..22835150579a 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -455,14 +455,18 @@ bool nfs_use_readdirplus(struct inode *dir, struct dir_context *ctx)
}
/*
- * This function is called by the lookup code to request the use of
- * readdirplus to accelerate any future lookups in the same
+ * This function is called by the lookup and getattr code to request the
+ * use of readdirplus to accelerate any future lookups in the same
* directory.
*/
static
void nfs_advise_use_readdirplus(struct inode *dir)
{
- set_bit(NFS_INO_ADVISE_RDPLUS, &NFS_I(dir)->flags);
+ 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);
}
/*
@@ -475,8 +479,11 @@ void nfs_advise_use_readdirplus(struct inode *dir)
*/
void nfs_force_use_readdirplus(struct inode *dir)
{
- if (!list_empty(&NFS_I(dir)->open_files)) {
- nfs_advise_use_readdirplus(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);
invalidate_mapping_pages(dir->i_mapping, 0, -1);
}
}
@@ -1150,7 +1157,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
return -ECHILD;
goto out_bad;
}
- goto out_valid_noent;
+ goto out_valid;
}
if (is_bad_inode(inode)) {
@@ -1173,6 +1180,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
return -ECHILD;
goto out_zap_parent;
}
+ nfs_advise_use_readdirplus(dir);
goto out_valid;
}
@@ -1208,12 +1216,12 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
nfs_free_fhandle(fhandle);
nfs4_label_free(label);
+ /* set a readdirplus hint that we had a cache miss */
+ nfs_force_use_readdirplus(dir);
+
out_set_verifier:
nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
out_valid:
- /* Success: notify readdir to use READDIRPLUS */
- nfs_advise_use_readdirplus(dir);
- out_valid_noent:
if (flags & LOOKUP_RCU) {
if (parent != ACCESS_ONCE(dentry->d_parent))
return -ECHILD;
@@ -1413,8 +1421,8 @@ struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, unsigned in
if (IS_ERR(res))
goto out_label;
- /* Success: notify readdir to use READDIRPLUS */
- nfs_advise_use_readdirplus(dir);
+ /* Notify readdir to use READDIRPLUS */
+ nfs_force_use_readdirplus(dir);
no_entry:
res = d_splice_alias(inode, dentry);
--
2.9.3
Ben Coddington reports that commit 311324ad1713, by adding the function
nfs_dir_mapping_need_revalidate() that checks page cache validity on
each call to nfs_readdir() causes a performance regression when
the directory is being modified.
If the directory is changing while we're iterating through the directory,
POSIX does not require us to invalidate the page cache unless the user
calls rewinddir(). However, we still do want to ensure that we use
readdirplus in order to avoid a load of stat() calls when the user
is doing an 'ls -l' workload.
The fix should be to invalidate the page cache immediately when we're
setting the NFS_INO_ADVISE_RDPLUS bit.
Reported-by: Benjamin Coddington <[email protected]>
Fixes: 311324ad1713 ("NFS: Be more aggressive in using readdirplus...")
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index f1ecfcc632eb..9220bc7b9cd7 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -477,7 +477,7 @@ void nfs_force_use_readdirplus(struct inode *dir)
{
if (!list_empty(&NFS_I(dir)->open_files)) {
nfs_advise_use_readdirplus(dir);
- nfs_zap_mapping(dir, dir->i_mapping);
+ invalidate_mapping_pages(dir->i_mapping, 0, -1);
}
}
@@ -886,17 +886,6 @@ int uncached_readdir(nfs_readdir_descriptor_t *desc)
goto out;
}
-static bool nfs_dir_mapping_need_revalidate(struct inode *dir)
-{
- struct nfs_inode *nfsi = NFS_I(dir);
-
- if (nfs_attribute_cache_expired(dir))
- return true;
- if (nfsi->cache_validity & NFS_INO_INVALID_DATA)
- return true;
- return false;
-}
-
/* The file offset position represents the dirent entry number. A
last cookie cache takes care of the common case of reading the
whole directory.
@@ -928,7 +917,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
desc->decode = NFS_PROTO(inode)->decode_dirent;
desc->plus = nfs_use_readdirplus(inode, ctx) ? 1 : 0;
- if (ctx->pos == 0 || nfs_dir_mapping_need_revalidate(inode))
+ if (ctx->pos == 0 || nfs_attribute_cache_expired(inode))
res = nfs_revalidate_mapping(inode, file->f_mapping);
if (res < 0)
goto out;
--
2.9.3
If the use called stat() on an 'ls -l' workload, and the attribute
cache was successfully revalidate by READDIRPLUS, then we want to
report that back so that the readdir code continues to use
readdirplus.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 1 -
fs/nfs/inode.c | 21 +++++++++++++++++----
fs/nfs/internal.h | 1 +
3 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 22835150579a..f5702457c052 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -459,7 +459,6 @@ bool nfs_use_readdirplus(struct inode *dir, struct dir_context *ctx)
* use of readdirplus to accelerate any future lookups in the same
* directory.
*/
-static
void nfs_advise_use_readdirplus(struct inode *dir)
{
struct nfs_inode *nfsi = NFS_I(dir);
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index d79e0bac836e..75f5a9cb2e66 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -634,15 +634,28 @@ void nfs_setattr_update_inode(struct inode *inode, struct iattr *attr,
}
EXPORT_SYMBOL_GPL(nfs_setattr_update_inode);
-static void nfs_request_parent_use_readdirplus(struct dentry *dentry)
+static void nfs_readdirplus_parent_cache_miss(struct dentry *dentry)
{
struct dentry *parent;
+ if (!nfs_server_capable(d_inode(dentry), NFS_CAP_READDIRPLUS))
+ return;
parent = dget_parent(dentry);
nfs_force_use_readdirplus(d_inode(parent));
dput(parent);
}
+static void nfs_readdirplus_parent_cache_hit(struct dentry *dentry)
+{
+ struct dentry *parent;
+
+ if (!nfs_server_capable(d_inode(dentry), NFS_CAP_READDIRPLUS))
+ return;
+ parent = dget_parent(dentry);
+ nfs_advise_use_readdirplus(d_inode(parent));
+ dput(parent);
+}
+
static bool nfs_need_revalidate_inode(struct inode *inode)
{
if (NFS_I(inode)->cache_validity &
@@ -683,10 +696,10 @@ int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)
if (need_atime || nfs_need_revalidate_inode(inode)) {
struct nfs_server *server = NFS_SERVER(inode);
- if (server->caps & NFS_CAP_READDIRPLUS)
- nfs_request_parent_use_readdirplus(dentry);
+ nfs_readdirplus_parent_cache_miss(dentry);
err = __nfs_revalidate_inode(server, inode);
- }
+ } else
+ nfs_readdirplus_parent_cache_hit(dentry);
if (!err) {
generic_fillattr(inode, stat);
stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 4622d5f18e35..6b79c2ca9b9a 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -344,6 +344,7 @@ extern struct nfs_client *nfs_init_client(struct nfs_client *clp,
const struct nfs_client_initdata *);
/* dir.c */
+extern void nfs_advise_use_readdirplus(struct inode *dir);
extern void nfs_force_use_readdirplus(struct inode *dir);
extern unsigned long nfs_access_cache_count(struct shrinker *shrink,
struct shrink_control *sc);
--
2.9.3
On 2 Dec 2016, at 9:21, Trond Myklebust wrote:
> Fix up the regression introduced by commit 311324ad1713, and then
> improve the READDIRPLUS hinting.
>
> Trond Myklebust (3):
> NFS: Fix a performance regression in readdir
> NFS: Be more targeted about readdirplus use when doing
> lookup/revalidation
> NFS: Allow getattr to also report readdirplus cache hits
>
> fs/nfs/dir.c | 46 +++++++++++++++++++++-------------------------
> fs/nfs/inode.c | 21 +++++++++++++++++----
> fs/nfs/internal.h | 1 +
> 3 files changed, 39 insertions(+), 29 deletions(-)
My testing shows that these three patches restore and/or retain these
three
NFS readdir optimizations:
1. Detect ls -l, so that we continue to use READDIRPLUS after the first
batch
of entries are returned by nfs_readdir(). This avoids a GETATTR
storm.
311324ad1713 NFS: Be more aggressive in using readdirplus for ‘ls
-l’
situations
2. Don’t use READDIRPLUS unnecessarily.
d69ee9b85541 NFS: Adapt readdirplus to application usage patterns
http://www.spinics.net/lists/linux-nfs/msg19996.html
3. Don’t invalidate the page cache every time the directory is
modified if we
are in the middle of a listing. At least wait until the attributes
time out.
07b5ce8ef2d8 NFS: Make nfs_readdir revalidate less often
Feel free to add
Reviewed-and-Tested-by: Benjamin Coddington <[email protected]>
.. and thanks very much!
Ben