Looks like in the case you find a previous delegation you decided it was
OK to just return -EAGAIN unconditionally. Makes sense to me, and
simplifies the code, good.
The one remaining thing I notice is that we don't need to move same_clid
any more. I'll fix that up myself.
Applying--thanks!
--b.
On Thu, Oct 15, 2015 at 12:07:28PM -0400, Andrew Elble wrote:
> We've observed the nfsd server in a state where there are
> multiple delegations on the same nfs4_file for the same client.
> The nfs client does attempt to DELEGRETURN these when they are presented to
> it - but apparently under some (unknown) circumstances the client does not
> manage to return all of them. This leads to the eventual
> attempt to CB_RECALL more than one delegation with the same nfs
> filehandle to the same client. The first recall will succeed, but the
> next recall will fail with NFS4ERR_BADHANDLE. This leads to the server
> having delegations on cl_revoked that the client has no way to FREE
> or DELEGRETURN, with resulting inability to recover. The state manager
> on the server will continually assert SEQ4_STATUS_RECALLABLE_STATE_REVOKED,
> and the state manager on the client will be looping unable to satisfy
> the server.
>
> List discussion also reports a race between OPEN and DELEGRETURN that
> will be avoided by only sending the delegation once to the
> client. This is also logically in accordance with RFC5561 9.1.1 and 10.2.
>
> So, let's:
>
> 1.) Not hand out duplicate delegations.
> 2.) Only send them to the client once.
>
> RFC 5561:
>
> 9.1.1:
> "Delegations and layouts, on the other hand, are not associated with a
> specific owner but are associated with the client as a whole
> (identified by a client ID)."
>
> 10.2:
> "...the stateid for a delegation is associated with a client ID and may be
> used on behalf of all the open-owners for the given client. A
> delegation is made to the client as a whole and not to any specific
> process or thread of control within it."
>
> Reported-by: Eric Meddaugh <etmsys@xxxxxxx>
> Cc: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> Cc: Olga Kornievskaia <aglo@xxxxxxxxx>
> Signed-off-by: Andrew Elble <aweits@xxxxxxx>
> ---
> fs/nfsd/nfs4state.c | 106 ++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 90 insertions(+), 16 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 0f1d5691b795..da21df673ed9 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -765,16 +765,74 @@ void nfs4_unhash_stid(struct nfs4_stid *s)
> s->sc_type = 0;
> }
>
> -static void
> +static int
> +same_clid(clientid_t *cl1, clientid_t *cl2)
> +{
> + return (cl1->cl_boot == cl2->cl_boot) && (cl1->cl_id == cl2->cl_id);
> +}
> +
> +/**
> + * nfs4_get_existing_delegation - Discover if this delegation already exists
> + * @clp: a pointer to the nfs4_client we're granting a delegation to
> + * @fp: a pointer to the nfs4_file we're granting a delegation on
> + *
> + * Return:
> + * On success: NULL if an existing delegation was not found.
> + *
> + * On error: -EAGAIN if one was previously granted to this nfs4_client
> + * for this nfs4_file.
> + *
> + */
> +
> +static int
> +nfs4_get_existing_delegation(struct nfs4_client *clp, struct nfs4_file *fp)
> +{
> + struct nfs4_delegation *searchdp = NULL;
> + struct nfs4_client *searchclp = NULL;
> +
> + lockdep_assert_held(&state_lock);
> + lockdep_assert_held(&fp->fi_lock);
> +
> + list_for_each_entry(searchdp, &fp->fi_delegations, dl_perfile) {
> + searchclp = searchdp->dl_stid.sc_client;
> + if (clp == searchclp) {
> + return -EAGAIN;
> + }
> + }
> + return 0;
> +}
> +
> +/**
> + * hash_delegation_locked - Add a delegation to the appropriate lists
> + * @dp: a pointer to the nfs4_delegation we are adding.
> + * @fp: a pointer to the nfs4_file we're granting a delegation on
> + *
> + * Return:
> + * On success: NULL if the delegation was successfully hashed.
> + *
> + * On error: -EAGAIN if one was previously granted to this
> + * nfs4_client for this nfs4_file. Delegation is not hashed.
> + *
> + */
> +
> +static int
> hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
> {
> + int status;
> + struct nfs4_client *clp = dp->dl_stid.sc_client;
> +
> lockdep_assert_held(&state_lock);
> lockdep_assert_held(&fp->fi_lock);
>
> + status = nfs4_get_existing_delegation(clp, fp);
> + if (status)
> + return status;
> + ++fp->fi_delegees;
> atomic_inc(&dp->dl_stid.sc_count);
> dp->dl_stid.sc_type = NFS4_DELEG_STID;
> list_add(&dp->dl_perfile, &fp->fi_delegations);
> - list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
> + list_add(&dp->dl_perclnt, &clp->cl_delegations);
> + return 0;
> }
>
> static bool
> @@ -1833,12 +1891,6 @@ same_verf(nfs4_verifier *v1, nfs4_verifier *v2)
> return 0 == memcmp(v1->data, v2->data, sizeof(v1->data));
> }
>
> -static int
> -same_clid(clientid_t *cl1, clientid_t *cl2)
> -{
> - return (cl1->cl_boot == cl2->cl_boot) && (cl1->cl_id == cl2->cl_id);
> -}
> -
> static bool groups_equal(struct group_info *g1, struct group_info *g2)
> {
> int i;
> @@ -3945,6 +3997,18 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
> return fl;
> }
>
> +/**
> + * nfs4_setlease - Obtain a delegation by requesting lease from vfs layer
> + * @dp: a pointer to the nfs4_delegation we're adding.
> + *
> + * Return:
> + * On success: Return code will be 0 on success.
> + *
> + * On error: -EAGAIN if there was an existing delegation.
> + * nonzero if there is an error in other cases.
> + *
> + */
> +
> static int nfs4_setlease(struct nfs4_delegation *dp)
> {
> struct nfs4_file *fp = dp->dl_stid.sc_file;
> @@ -3976,16 +4040,19 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
> goto out_unlock;
> /* Race breaker */
> if (fp->fi_deleg_file) {
> - status = 0;
> - ++fp->fi_delegees;
> - hash_delegation_locked(dp, fp);
> + status = hash_delegation_locked(dp, fp);
> goto out_unlock;
> }
> fp->fi_deleg_file = filp;
> - fp->fi_delegees = 1;
> - hash_delegation_locked(dp, fp);
> + fp->fi_delegees = 0;
> + status = hash_delegation_locked(dp, fp);
> spin_unlock(&fp->fi_lock);
> spin_unlock(&state_lock);
> + if (status) {
> + /* Should never happen, this is a new fi_deleg_file */
> + WARN_ON_ONCE(1);
> + goto out_fput;
> + }
> return 0;
> out_unlock:
> spin_unlock(&fp->fi_lock);
> @@ -4005,6 +4072,15 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
> if (fp->fi_had_conflict)
> return ERR_PTR(-EAGAIN);
>
> + spin_lock(&state_lock);
> + spin_lock(&fp->fi_lock);
> + status = nfs4_get_existing_delegation(clp, fp);
> + spin_unlock(&fp->fi_lock);
> + spin_unlock(&state_lock);
> +
> + if (status)
> + return ERR_PTR(status);
> +
> dp = alloc_init_deleg(clp, fh, odstate);
> if (!dp)
> return ERR_PTR(-ENOMEM);
> @@ -4023,9 +4099,7 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
> status = -EAGAIN;
> goto out_unlock;
> }
> - ++fp->fi_delegees;
> - hash_delegation_locked(dp, fp);
> - status = 0;
> + status = hash_delegation_locked(dp, fp);
> out_unlock:
> spin_unlock(&fp->fi_lock);
> spin_unlock(&state_lock);
> --
> 2.4.6
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
