Re: readdir vs. getattr

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3713 bytes --]

On Thu, 06 Feb 2014 17:12:59 -0500 Trond Myklebust
<trond.myklebust@primarydata.com> wrote:

> On Thu, 2014-02-06 at 13:51 +1100, NeilBrown wrote:
> > On Thu, 6 Feb 2014 13:45:39 +1100 NeilBrown <neilb@suse.de> wrote:
> > 
> > 
> > > The change to nfs_update_inode fixes an issue that had me stumped for a
> > > while.  It was still sending lots of GETATTR requests even after it had
> > > switched to READDIRPLUS instead of using cached info.  So that might be a
> > > genuine bug that should be fixed independently of this patch.
> > 
> > I managed to post the wrong version of the patch, which didn't have this
> > change.  Sorry.
> > 
> > Here is the real one.
> > 
> > NeilBrown
> 
> Hi Neil,
> 
> Is there any reason why this patch couldn't just be replaced with the
> following?

Well.... the following doesn't make any change in my test case. :-(

In my test case there is a large directory on the server which doesn't change.
The client sends lots of requests to see if it has changed, but it hasn't.
(Tested with the labeled-NFS-regression fix applied)

Your patch only changes behaviour if nfs_force_lookup_revalidate is called,
and that is only called when NFS_ATTR_FATTR_CHANGE is set, or when an NFSv4
client modifies the directory.  Neither of those are happening.

Thanks,
NeilBrown


> (Please note the separate fix for the labeled NFS regression that you
> pointed out.)
> 
> Cheers
>   Trond
> 
> 8<-------------------------------------------------------------------
> >From fda6d0fb7988fd7d541ef6d1023bca92b0e10333 Mon Sep 17 00:00:00 2001
> From: Trond Myklebust <trond.myklebust@primarydata.com>
> Date: Thu, 6 Feb 2014 16:45:21 -0500
> Subject: [PATCH] NFS: Use readdirplus in order to efficiently revalidate whole
>  directories
> 
> When we call nfs_force_lookup_revalidate() in order to force a full
> lookup of all child dentries of a directory, it makes sense to use
> readdirplus to perform that revalidation as efficiently as possible.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  fs/nfs/dir.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index be38b573495a..2abcae330ad0 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -437,6 +437,24 @@ void nfs_advise_use_readdirplus(struct inode *dir)
>  	set_bit(NFS_INO_ADVISE_RDPLUS, &NFS_I(dir)->flags);
>  }
>  
> +/*
> + * Force use of readdirplus to ensure efficient revalidation of
> + * the directory child dentries when we know that we will need
> + * to revalidate the whole directory.
> + *
> + * Caller must hold dir->i_lock.
> + */
> +static
> +void nfs_force_use_readdirplus(struct inode *dir)
> +{
> +	if (nfs_server_capable(dir, NFS_CAP_READDIRPLUS)) {
> +		struct nfs_inode *nfsi = NFS_I(dir);
> +
> +		set_bit(NFS_INO_ADVISE_RDPLUS, &nfsi->flags);
> +		nfsi->cache_validity |= NFS_INO_INVALID_DATA;
> +	}
> +}
> +
>  static
>  void nfs_prime_dcache(struct dentry *parent, struct nfs_entry *entry)
>  {
> @@ -942,12 +960,14 @@ static int nfs_fsync_dir(struct file *filp, loff_t start, loff_t end,
>   * This forces the revalidation code in nfs_lookup_revalidate() to do a
>   * full lookup on all child dentries of 'dir' whenever a change occurs
>   * on the server that might have invalidated our dcache.
> + * Call nfs_force_use_readdirplus for efficiency.
>   *
>   * The caller should be holding dir->i_lock
>   */
>  void nfs_force_lookup_revalidate(struct inode *dir)
>  {
>  	NFS_I(dir)->cache_change_attribute++;
> +	nfs_force_use_readdirplus(dir);
>  }
>  EXPORT_SYMBOL_GPL(nfs_force_lookup_revalidate);
>  


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]