commit 79f687a3de9e ("NFS: Fix a performance regression in readdir")
removed nfs_dir_mapping_need_revalidate() in an attempt to fix a
performance regression, but had the effect that with NFSv3 the
pagecache would never expire while a process was reading a directory.
An earlier patch addressed this problem by allowing the pagecache
to expire but the current process to continue using the last cookie
returned by the server when searching the pagecache, rather than
always starting at 0. Thus, bring back nfs_dir_mapping_need_revalidate()
so that nfsi->cache_validity & NFS_INO_INVALID_DATA will be seen
and nfs_revalidate_mapping() will be called with NFSv3 as well,
making it behave the same as NFSv4.
Signed-off-by: Dave Wysochanski <[email protected]>
---
fs/nfs/dir.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index cbd74cbdbb9f..aeb086fb3d0a 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -872,6 +872,17 @@ int uncached_readdir(nfs_readdir_descriptor_t *desc)
return status;
}
+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.
@@ -903,7 +914,7 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
* to either find the entry with the appropriate number or
* revalidate the cookie.
*/
- if (ctx->pos == 0 || nfs_attribute_cache_expired(inode))
+ if (ctx->pos == 0 || nfs_dir_mapping_need_revalidate(inode))
res = nfs_revalidate_mapping(inode, file->f_mapping);
if (res < 0)
goto out;
--
1.8.3.1
Hi Dave,
----- Original Message -----
> From: "David Wysochanski" <[email protected]>
> To: "trondmy" <[email protected]>, "Anna Schumaker" <[email protected]>
> Cc: "linux-nfs" <[email protected]>
> Sent: Monday, 2 November, 2020 14:50:11
> Subject: [PATCH 11/11] NFS: Bring back nfs_dir_mapping_need_revalidate() in nfs_readdir()
> commit 79f687a3de9e ("NFS: Fix a performance regression in readdir")
> removed nfs_dir_mapping_need_revalidate() in an attempt to fix a
> performance regression, but had the effect that with NFSv3 the
> pagecache would never expire while a process was reading a directory.
> An earlier patch addressed this problem by allowing the pagecache
> to expire but the current process to continue using the last cookie
> returned by the server when searching the pagecache, rather than
> always starting at 0. Thus, bring back nfs_dir_mapping_need_revalidate()
> so that nfsi->cache_validity & NFS_INO_INVALID_DATA will be seen
> and nfs_revalidate_mapping() will be called with NFSv3 as well,
> making it behave the same as NFSv4.
>
> Signed-off-by: Dave Wysochanski <[email protected]>
> ---
> fs/nfs/dir.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index cbd74cbdbb9f..aeb086fb3d0a 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -872,6 +872,17 @@ int uncached_readdir(nfs_readdir_descriptor_t *desc)
> return status;
> }
>
> +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;
> +}
Is this the same as:
static bool nfs_dir_mapping_need_revalidate(struct inode *dir)
{
struct nfs_inode *nfsi = NFS_I(dir);
return nfs_attribute_cache_expired(dir) || nfsi->cache_validity & NFS_INO_INVALID_DATA);
}
Tigran.
> +
> /* The file offset position represents the dirent entry number. A
> last cookie cache takes care of the common case of reading the
> whole directory.
> @@ -903,7 +914,7 @@ static int nfs_readdir(struct file *file, struct dir_context
> *ctx)
> * to either find the entry with the appropriate number or
> * revalidate the cookie.
> */
> - if (ctx->pos == 0 || nfs_attribute_cache_expired(inode))
> + if (ctx->pos == 0 || nfs_dir_mapping_need_revalidate(inode))
> res = nfs_revalidate_mapping(inode, file->f_mapping);
> if (res < 0)
> goto out;
> --
> 1.8.3.1
On Mon, Nov 2, 2020 at 10:48 AM Mkrtchyan, Tigran
<[email protected]> wrote:
>
> Hi Dave,
>
> ----- Original Message -----
> > From: "David Wysochanski" <[email protected]>
> > To: "trondmy" <[email protected]>, "Anna Schumaker" <[email protected]>
> > Cc: "linux-nfs" <[email protected]>
> > Sent: Monday, 2 November, 2020 14:50:11
> > Subject: [PATCH 11/11] NFS: Bring back nfs_dir_mapping_need_revalidate() in nfs_readdir()
>
> > commit 79f687a3de9e ("NFS: Fix a performance regression in readdir")
> > removed nfs_dir_mapping_need_revalidate() in an attempt to fix a
> > performance regression, but had the effect that with NFSv3 the
> > pagecache would never expire while a process was reading a directory.
> > An earlier patch addressed this problem by allowing the pagecache
> > to expire but the current process to continue using the last cookie
> > returned by the server when searching the pagecache, rather than
> > always starting at 0. Thus, bring back nfs_dir_mapping_need_revalidate()
> > so that nfsi->cache_validity & NFS_INO_INVALID_DATA will be seen
> > and nfs_revalidate_mapping() will be called with NFSv3 as well,
> > making it behave the same as NFSv4.
> >
> > Signed-off-by: Dave Wysochanski <[email protected]>
> > ---
> > fs/nfs/dir.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index cbd74cbdbb9f..aeb086fb3d0a 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -872,6 +872,17 @@ int uncached_readdir(nfs_readdir_descriptor_t *desc)
> > return status;
> > }
> >
> > +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;
> > +}
>
> Is this the same as:
>
> static bool nfs_dir_mapping_need_revalidate(struct inode *dir)
> {
> struct nfs_inode *nfsi = NFS_I(dir);
>
> return nfs_attribute_cache_expired(dir) || nfsi->cache_validity & NFS_INO_INVALID_DATA);
> }
>
Yes that's a nice simplification!
> Tigran.
>
> > +
> > /* The file offset position represents the dirent entry number. A
> > last cookie cache takes care of the common case of reading the
> > whole directory.
> > @@ -903,7 +914,7 @@ static int nfs_readdir(struct file *file, struct dir_context
> > *ctx)
> > * to either find the entry with the appropriate number or
> > * revalidate the cookie.
> > */
> > - if (ctx->pos == 0 || nfs_attribute_cache_expired(inode))
> > + if (ctx->pos == 0 || nfs_dir_mapping_need_revalidate(inode))
> > res = nfs_revalidate_mapping(inode, file->f_mapping);
> > if (res < 0)
> > goto out;
> > --
> > 1.8.3.1
>