| Message ID | 1391724779.6399.2.camel@leira.trondhjem.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
On Feb 6, 2014, at 23:30, NeilBrown <neilb@suse.de> wrote: > 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. Right, but I really do not know how to fix that case. I read your patch as saying: "If we're revalidating a dentry and we find that the directory has not changed but that the dentry’s inode needs revalidation, then invalidate that directory's attribute, acl and readdir cached data. Then drop the dentry”. I have a hard time seeing how that can be beneficial. If we were to modify it, to only invalidate the readdir cached data, then that might improve matters, but it is still hard for me to see how a single inode needing revalidation can justify a full re-read of the parent directory in the general case. You may end up rereading thousands of readdir entries from the server just because someone did a path lookup.
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);