> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@xxxxxxx> wrote:
>
> nfsd_lookup() takes an exclusive lock on the parent inode, but no
> callers want the lock and it may not be needed at all if the
> result is in the dcache.
>
> Change nfsd_lookup_dentry() to not take the lock, and call
> lookup_one_len_locked() which takes lock only if needed.
>
> nfsd4_open() currently expects the lock to still be held, but that isn't
> necessary as nfsd_validate_delegated_dentry() provides required
> guarantees without the lock.
>
> NOTE: NFSv4 requires directory changeinfo for OPEN even when a create
> wasn't requested and no change happened. Now that nfsd_lookup()
> doesn't use fh_lock(), we need to explicitly fill the attributes
> when no create happens. A new fh_fill_both_attrs() is provided
> for that task.
>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> fs/nfsd/nfs4proc.c | 20 ++++++++++++--------
> fs/nfsd/nfs4state.c | 3 ---
> fs/nfsd/nfsfh.c | 19 +++++++++++++++++++
> fs/nfsd/nfsfh.h | 2 +-
> fs/nfsd/vfs.c | 34 ++++++++++++++--------------------
> 5 files changed, 46 insertions(+), 32 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 1aa6ae4ec2f5..48e4efb39a9c 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -302,6 +302,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (d_really_is_positive(child)) {
> status = nfs_ok;
>
> + /* NFSv4 protocol requires change attributes even though
> + * no change happened.
> + */
> + fh_fill_both_attrs(fhp);
> +
> switch (open->op_createmode) {
> case NFS4_CREATE_UNCHECKED:
> if (!d_is_reg(child))
> @@ -417,15 +422,15 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
> if (nfsd4_create_is_exclusive(open->op_createmode) && status == 0)
> open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
> FATTR4_WORD1_TIME_MODIFY);
> - } else
> - /*
> - * Note this may exit with the parent still locked.
> - * We will hold the lock until nfsd4_open's final
> - * lookup, to prevent renames or unlinks until we've had
> - * a chance to an acquire a delegation if appropriate.
> - */
> + } else {
> status = nfsd_lookup(rqstp, current_fh,
> open->op_fname, open->op_fnamelen, *resfh);
> + if (!status)
> + /* NFSv4 protocol requires change attributes even though
> + * no change happened.
> + */
> + fh_fill_both_attrs(current_fh);
> + }
> if (status)
> goto out;
> status = nfsd_check_obj_isreg(*resfh);
> @@ -1043,7 +1048,6 @@ nfsd4_secinfo(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> &exp, &dentry);
> if (err)
> return err;
> - fh_unlock(&cstate->current_fh);
> if (d_really_is_negative(dentry)) {
> exp_put(exp);
> err = nfserr_noent;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c2ca37d0a616..45df1e85ff32 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5304,9 +5304,6 @@ nfsd4_verify_deleg_dentry(struct nfsd4_open *open, struct nfs4_file *fp,
> struct dentry *child;
> __be32 err;
>
> - /* parent may already be locked, and it may get unlocked by
> - * this call, but that is safe.
> - */
> err = nfsd_lookup_dentry(open->op_rqstp, parent,
> open->op_fname, open->op_fnamelen,
> &exp, &child);
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 5e2ed4b2a925..cd2946a88d72 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -672,6 +672,25 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
> nfsd4_change_attribute(&fhp->fh_post_attr, inode);
> }
>
> +/**
> + * fh_fill_both_attrs - Fill pre-op and post-op attributes
> + * @fhp: file handle to be updated
> + *
> + * This is used when the directory wasn't changed, but wcc attributes
> + * are needed anyway.
> + */
> +void fh_fill_both_attrs(struct svc_fh *fhp)
> +{
> + fh_fill_post_attrs(fhp);
> + if (!fhp->fh_post_saved)
> + return;
> + fhp->fh_pre_change = fhp->fh_post_change;
> + fhp->fh_pre_mtime = fhp->fh_post_attr.mtime;
> + fhp->fh_pre_ctime = fhp->fh_post_attr.ctime;
> + fhp->fh_pre_size = fhp->fh_post_attr.size;
> + fhp->fh_pre_saved = true;
> +}
> +
> /*
> * Release a file handle.
> */
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index fb9d358a267e..28a4f9a94e2c 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -322,7 +322,7 @@ static inline u64 nfsd4_change_attribute(struct kstat *stat,
>
> extern void fh_fill_pre_attrs(struct svc_fh *fhp);
> extern void fh_fill_post_attrs(struct svc_fh *fhp);
> -
> +extern void fh_fill_both_attrs(struct svc_fh *fhp);
>
> /*
> * Lock a file handle/inode
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 06b1408db08b..8ebad4a99552 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -199,27 +199,13 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
> goto out_nfserr;
> }
> } else {
> - /*
> - * In the nfsd4_open() case, this may be held across
> - * subsequent open and delegation acquisition which may
> - * need to take the child's i_mutex:
> - */
> - fh_lock_nested(fhp, I_MUTEX_PARENT);
> - dentry = lookup_one_len(name, dparent, len);
> + dentry = lookup_one_len_unlocked(name, dparent, len);
> host_err = PTR_ERR(dentry);
> if (IS_ERR(dentry))
> goto out_nfserr;
> if (nfsd_mountpoint(dentry, exp)) {
> - /*
> - * We don't need the i_mutex after all. It's
> - * still possible we could open this (regular
> - * files can be mountpoints too), but the
> - * i_mutex is just there to prevent renames of
> - * something that we might be about to delegate,
> - * and a mountpoint won't be renamed:
> - */
> - fh_unlock(fhp);
> - if ((host_err = nfsd_cross_mnt(rqstp, &dentry, &exp))) {
> + host_err = nfsd_cross_mnt(rqstp, &dentry, &exp);
> + if (host_err) {
> dput(dentry);
> goto out_nfserr;
> }
> @@ -234,7 +220,15 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
> return nfserrno(host_err);
> }
>
> -/*
> +/**
> + * nfsd_lookup - look up a single path component for nfsd
> + *
> + * @rqstp: the request context
> + * @ftp: the file handle of the directory
/home/cel/src/linux/klimt/fs/nfsd/vfs.c:246: warning: Function parameter or member 'fhp' not described in 'nfsd_lookup'
/home/cel/src/linux/klimt/fs/nfsd/vfs.c:246: warning: Excess function parameter 'ftp' description in 'nfsd_lookup'
> + * @name: the component name, or %NULL to look up parent
> + * @len: length of name to examine
> + * @resfh: pointer to pre-initialised filehandle to hold result.
> + *
> * Look up one component of a pathname.
> * N.B. After this call _both_ fhp and resfh need an fh_put
> *
> @@ -244,11 +238,11 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
> * returned. Otherwise the covered directory is returned.
> * NOTE: this mountpoint crossing is not supported properly by all
> * clients and is explicitly disallowed for NFSv3
> - * NeilBrown <neilb@xxxxxxxxxxxxxxx>
> + *
> */
> __be32
> nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
> - unsigned int len, struct svc_fh *resfh)
> + unsigned int len, struct svc_fh *resfh)
> {
> struct svc_export *exp;
> struct dentry *dentry;
>
>
--
Chuck Lever
