On Wed, 2016-11-30 at 14:09 -0500, Benjamin Coddington wrote:
> .. this one breaks things again:
>
> On 19 Nov 2016, at 11:54, Trond Myklebust wrote:
>
> > 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 <trond.myklebust@xxxxxxxxxxxxxxx>
> > ---
> > fs/nfs/dir.c | 28 +++++++++++++++++-----------
> > 1 file changed, 17 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index 53e02b8bd9bd..5befd382be7d 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -455,14 +455,23 @@ 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.
> > + * Do this by checking if there is an active file descriptor
> > + * and calling nfs_advise_use_readdirplus, then forcing a
> > + * cache flush.
> > */
> > 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);
> > + invalidate_mapping_pages(dir->i_mapping, 0, -1);
> > + }
> > }
>
> So every time advise_use_readdirplus it drops the mapping.. but what
> about
> subsequent calls into nfs_readdir() to get the next batch of
> entries?
> For
> the ls -l case, we want to keep setting NFS_INO_ADVISE_RDPLUS, but we
> shouldn't start over filling the mapping from the beginning again.
How do I ensure that the readdir isn't being served from cache, if I
don't invalidate the mapping? The intention of the patch is to ensure
that we only call this on a dcache or inode attribute cache miss.
>
> >
> > /*
> > @@ -475,10 +484,7 @@ 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);
> > - invalidate_mapping_pages(dir->i_mapping, 0, -1);
> > - }
> > + nfs_advise_use_readdirplus(dir);
> > }
> >
> > static
> > @@ -1150,7 +1156,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)) {
> > @@ -1192,6 +1198,9 @@ static int nfs_lookup_revalidate(struct
> > dentry
> > *dentry, unsigned int flags)
> > if (IS_ERR(label))
> > goto out_error;
> >
> > + /* We need to force a revalidation: set a readdirplus hint
> > */
> > + nfs_advise_use_readdirplus(dir);
> > +
> > trace_nfs_lookup_revalidate_enter(dir, dentry, flags);
> > error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name,
> > fhandle, fattr,
> > label);
> > trace_nfs_lookup_revalidate_exit(dir, dentry, flags,
> > error);
> > @@ -1211,9 +1220,6 @@ static int nfs_lookup_revalidate(struct
> > dentry
> > *dentry, unsigned int flags)
> > 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;
>
>
> Now when listing with `ls -l`: the second call into nfs_readdir()
> to
> get
> the next batch of entries will not have NFS_INO_ADVISE_RDPLUS.
>
> I think this removes nfs_advise_use_readdirplus(dir) from the common
> "goto
> out_valid" path through nfs_lookup_revalidate() (the block with the
> 'iff'
> typo).
>
Actually, 'iff' is intentionally used there as the common shorthand for
'if and only if' (https://www.merriam-webster.com/dictionary/iff).
As I said above, the point is to only trigger READDIRPLUS when we know
the dcache or the inode cache needs revalidation. Otherwise we want to
use the less expensive READDIR. I'm open for suggestions as to how we
can improve that heuristic.
Cheers
Trond��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
