2015-10-02 11:19:06

by Jeff Layton

Subject: [RFC PATCH] nfs4: clear delegation from open states when returning delegation voluntarily

When the client goes to return a delegation, it should always update any
nfs4_state currently set up to use that delegation stateid to instead
use the open stateid. It already does do this in some cases,
particularly in the state recovery code, but not currently when the
delegation is voluntarily returned (e.g. in advance of a RENAME).

This causes the client to try to continue using the delegation stateid
after the DELEGRETURN, e.g. in LAYOUTGET. This patch fixes this by
ensuring to set the nfs4_state back to using the open stateid in
nfs_delegation_claim_opens.

That said, this code is quite difficult to follow and it's not 100%
clear to me why the delegreturn handling and the state recovery code are
squashed together like this. So, consider this an RFC patch and please
let me know if there's a better way to fix this.

Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/delegation.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index be806ead7f4d..aba906c50e8e 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -147,6 +147,10 @@ again:
if (!err && read_seqcount_retry(&sp->so_reclaim_seqcount, seq))
err = -EAGAIN;
mutex_unlock(&sp->so_delegreturn_mutex);
+ write_seqlock(&state->seqlock);
+ nfs4_stateid_copy(&state->stateid, &state->open_stateid);
+ write_sequnlock(&state->seqlock);
+ clear_bit(NFS_DELEGATED_STATE, &state->flags);
put_nfs_open_context(ctx);
if (err != 0)
return err;
--
2.4.3

2015-10-02 17:14:45

by Jeff Layton

Subject: [PATCH v2] nfs4: reset states to use open_stateid when returning delegation voluntarily

When the client goes to return a delegation, it should always update any
nfs4_state currently set up to use that delegation stateid to instead
use the open stateid. It already does do this in some cases,
particularly in the state recovery code, but not currently when the
delegation is voluntarily returned (e.g. in advance of a RENAME). This
causes the client to try to continue using the delegation stateid after
the DELEGRETURN, e.g. in LAYOUTGET.

Set the nfs4_state back to using the open stateid in
nfs4_open_delegation_recall, just before clearing the
NFS_DELEGATED_STATE bit.

Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/nfs4proc.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index f93b9cdb4934..246753e1d19d 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1771,6 +1771,9 @@ int nfs4_open_delegation_recall(struct nfs_open_context *ctx,
if (IS_ERR(opendata))
return PTR_ERR(opendata);
nfs4_stateid_copy(&opendata->o_arg.u.delegation, stateid);
+ write_seqlock(&state->seqlock);
+ nfs4_stateid_copy(&state->stateid, &state->open_stateid);
+ write_sequnlock(&state->seqlock);
clear_bit(NFS_DELEGATED_STATE, &state->flags);
switch (type & (FMODE_READ|FMODE_WRITE)) {
case FMODE_READ|FMODE_WRITE:
--
2.4.3

2015-10-02 19:53:35

by Trond Myklebust

Subject: Re: [PATCH v2] nfs4: reset states to use open_stateid when returning delegation voluntarily

On Fri, Oct 2, 2015 at 1:14 PM, Jeff Layton <[email protected]> wrote:
> When the client goes to return a delegation, it should always update any
> nfs4_state currently set up to use that delegation stateid to instead
> use the open stateid. It already does do this in some cases,
> particularly in the state recovery code, but not currently when the
> delegation is voluntarily returned (e.g. in advance of a RENAME). This
> causes the client to try to continue using the delegation stateid after
> the DELEGRETURN, e.g. in LAYOUTGET.
>
> Set the nfs4_state back to using the open stateid in
> nfs4_open_delegation_recall, just before clearing the
> NFS_DELEGATED_STATE bit.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index f93b9cdb4934..246753e1d19d 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1771,6 +1771,9 @@ int nfs4_open_delegation_recall(struct nfs_open_context *ctx,
> if (IS_ERR(opendata))
> return PTR_ERR(opendata);
> nfs4_stateid_copy(&opendata->o_arg.u.delegation, stateid);
> + write_seqlock(&state->seqlock);
> + nfs4_stateid_copy(&state->stateid, &state->open_stateid);
> + write_sequnlock(&state->seqlock);
> clear_bit(NFS_DELEGATED_STATE, &state->flags);
> switch (type & (FMODE_READ|FMODE_WRITE)) {
> case FMODE_READ|FMODE_WRITE:
> --

Looks good. Thanks!