Skip to content

Commit

Permalink
nfsd: eliminate sending duplicate and repeated delegations
Browse files Browse the repository at this point in the history
commit 34ed987 upstream.

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@rit.edu>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: Olga Kornievskaia <aglo@umich.edu>
Signed-off-by: Andrew Elble <aweits@rit.edu>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
Andrew Elble authored and Greg Kroah-Hartman committed Dec 15, 2015
1 parent 35b2e29 commit 3c10560
Showing 1 changed file with 84 additions and 10 deletions.
94 changes: 84 additions & 10 deletions fs/nfsd/nfs4state.c
Original file line number Diff line number Diff line change
Expand Up @@ -765,16 +765,68 @@ void nfs4_unhash_stid(struct nfs4_stid *s)
s->sc_type = 0;
}

static void
/**
* 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
Expand Down Expand Up @@ -3941,6 +3993,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;
Expand Down Expand Up @@ -3972,16 +4036,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);
Expand All @@ -4001,6 +4068,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);
Expand All @@ -4019,9 +4095,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);
Expand Down

0 comments on commit 3c10560

Please sign in to comment.