question about code in delegation.c

question about code in delegation.c

[Olga Kornievskaia]

Hi Trond,

I have a question about a patch you committed 57bfa891 and
specifically about the comment that's in the code saying "deal with
the broken servers that hand out two delegations for the same file".
Why is this considered to be broken?

The situation I'm experiencing has the following flow:
1. client opens a file and gets a write delegation.
2. this file is then closed, it's subsequently locally opened for
write (no open on the wire). delegation stateid is also used for write
operations seen on the wire. all is good.
3. then client (on the wire) sends an open for read. first i'm not
sure why this is not local. but let's say the client is allowed to do
so.
4. the server knows this client has a write delegation for this file
so it replies to the open with the write delegation.
5. then code in "nfs_node_set_delegation" sees that it's the same
delegation and ends up returning "it". however from the server's point
of you, it considers the client returning the one delegation it gave
out.
6. the client proceeds to use the delegation stateid which causes the
server to send BAD_SESSIONID which leads the client to initiate state
recovery and mark it's locks lost and return EIO.

(a) it seems like the open for read shouldn't have gone on the wire to
begin with, but
(b) if there are cases when we do want to send an open even if we hold
a delegation, then shouldn't we just ignore if we receive the same
delegation that already hold?

Thank you.

Re: question about code in delegation.c

[Trond Myklebust]

Hi Olga

On Fri, Dec 19, 2014 at 10:11 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
> Hi Trond,
>
> I have a question about a patch you committed 57bfa891 and
> specifically about the comment that's in the code saying "deal with
> the broken servers that hand out two delegations for the same file".
> Why is this considered to be broken?
>
> The situation I'm experiencing has the following flow:
> 1. client opens a file and gets a write delegation.
> 2. this file is then closed, it's subsequently locally opened for
> write (no open on the wire). delegation stateid is also used for write
> operations seen on the wire. all is good.
> 3. then client (on the wire) sends an open for read. first i'm not
> sure why this is not local. but let's say the client is allowed to do
> so.

Does the client know that this is the same file? i.e. is this a
situation where it is using the same directory filehandle + filename
in the open?

> 4. the server knows this client has a write delegation for this file
> so it replies to the open with the write delegation.
> 5. then code in "nfs_node_set_delegation" sees that it's the same
> delegation and ends up returning "it". however from the server's point
> of you, it considers the client returning the one delegation it gave
> out.

It won't return a delegation with a matching stateid and _type_ to the
one it already holds.

That said, the code should probably make a distinction here:

1) If the delegation stateid matches, we shouldn't delegreturn.
Instead, just check the delegation type and try to figure out if this
is an upgrade from a read delegation to a write delegation.
or
2) If the delegation stateid doesn't match, figure out whether or not
this is a delegation upgrade, and either discard the old stateid if it
is (good server) or discard the new stateid if it isn't (broken
server). Issue delegreturn for the stateid we're discarding.

> 6. the client proceeds to use the delegation stateid which causes the
> server to send BAD_SESSIONID which leads the client to initiate state
> recovery and mark it's locks lost and return EIO.

BAD_SESSIONID? That's just wrong...

> (a) it seems like the open for read shouldn't have gone on the wire to
> begin with, but
> (b) if there are cases when we do want to send an open even if we hold
> a delegation, then shouldn't we just ignore if we receive the same
> delegation that already hold?
>
> Thank you.



-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: question about code in delegation.c

[Olga Kornievskaia]

On Fri, Dec 19, 2014 at 10:37 AM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> Hi Olga
>
> On Fri, Dec 19, 2014 at 10:11 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>> Hi Trond,
>>
>> I have a question about a patch you committed 57bfa891 and
>> specifically about the comment that's in the code saying "deal with
>> the broken servers that hand out two delegations for the same file".
>> Why is this considered to be broken?
>>
>> The situation I'm experiencing has the following flow:
>> 1. client opens a file and gets a write delegation.
>> 2. this file is then closed, it's subsequently locally opened for
>> write (no open on the wire). delegation stateid is also used for write
>> operations seen on the wire. all is good.
>> 3. then client (on the wire) sends an open for read. first i'm not
>> sure why this is not local. but let's say the client is allowed to do
>> so.
>
> Does the client know that this is the same file? i.e. is this a
> situation where it is using the same directory filehandle + filename
> in the open?
>
>> 4. the server knows this client has a write delegation for this file
>> so it replies to the open with the write delegation.
>> 5. then code in "nfs_node_set_delegation" sees that it's the same
>> delegation and ends up returning "it". however from the server's point
>> of you, it considers the client returning the one delegation it gave
>> out.
>
> It won't return a delegation with a matching stateid and _type_ to the
> one it already holds.
>
> That said, the code should probably make a distinction here:
>
> 1) If the delegation stateid matches, we shouldn't delegreturn.
> Instead, just check the delegation type and try to figure out if this
> is an upgrade from a read delegation to a write delegation.
> or
> 2) If the delegation stateid doesn't match, figure out whether or not
> this is a delegation upgrade, and either discard the old stateid if it
> is (good server) or discard the new stateid if it isn't (broken
> server). Issue delegreturn for the stateid we're discarding.
>
>> 6. the client proceeds to use the delegation stateid which causes the
>> server to send BAD_SESSIONID which leads the client to initiate state
>> recovery and mark it's locks lost and return EIO.
>
> BAD_SESSIONID? That's just wrong...

Sorry that was suppose to be BAD_STATEID. I see what you mean about
the same type of delegation should not be returned. I was staring at
that code for too long and miss read it. That would have been an easy
explanation for the erroneous DELEG_RETURN I'm see on submitted
traces. It's hard to debug when I can't reproduce the behavior.. :-/

Thank you for the explanation. I'll keep digging where the real client
brokenness lies.

>> (a) it seems like the open for read shouldn't have gone on the wire to
>> begin with, but
>> (b) if there are cases when we do want to send an open even if we hold
>> a delegation, then shouldn't we just ignore if we receive the same
>> delegation that already hold?
>>
>> Thank you.
>
>
>
> --
> Trond Myklebust
>
> Linux NFS client maintainer, PrimaryData
>
> trond.myklebust@primarydata.com

[PATCH 1/2] NFSv4: Deal with atomic upgrades of an existing delegation

[Trond Myklebust]

Ensure that we deal correctly with the case where the server sends us a
newer instance of the same delegation. If the stateids match, but the
sequence numbers differ, then treat the new delegation as if it were
an atomic upgrade.

Signed-off-by: Trond Myklebust <Trond.Myklebust@primarydata.com>
---
 fs/nfs/delegation.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 7f3f60641344..90413cdbf254 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -301,6 +301,18 @@ nfs_inode_detach_delegation(struct inode *inode)
 	return nfs_detach_delegation(nfsi, delegation, server);
 }
 
+static void
+nfs_update_inplace_delegation(struct nfs_delegation *delegation,
+		struct nfs_delegation *update)
+{
+	if (nfs4_stateid_is_newer(&update->stateid, &delegation->stateid))
+		return;
+	delegation->stateid.seqid = update->stateid.seqid;
+	smp_wmb();
+	delegation->type == update->type;
+	nfsi->delegation_state = update->type;
+}
+
 /**
  * nfs_inode_set_delegation - set up a delegation on an inode
  * @inode: inode to which delegation applies
@@ -334,9 +346,11 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 	old_delegation = rcu_dereference_protected(nfsi->delegation,
 					lockdep_is_held(&clp->cl_lock));
 	if (old_delegation != NULL) {
-		if (nfs4_stateid_match(&delegation->stateid,
-					&old_delegation->stateid) &&
-				delegation->type == old_delegation->type) {
+		/* Is this an update of the existing delegation? */
+		if (nfs4_stateid_match_other(&old_delegation->stateid,
+					&delegation->stateid)) {
+			nfs_update_inplace_delegation(old_delegation,
+					delegation);
 			goto out;
 		}
 		/*
-- 
2.1.0

[PATCH 2/2] NFSv4: Remove incorrect check in can_open_delegated()

[Trond Myklebust]

Remove an incorrect check for NFS_DELEGATION_NEED_RECLAIM in
can_open_delegated(). We are allowed to cache opens even in
a situation where we're doing reboot recovery.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs4proc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 8514b59a8c30..002c7dfedb08 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1119,8 +1119,6 @@ static int can_open_delegated(struct nfs_delegation *delegation, fmode_t fmode)
 		return 0;
 	if ((delegation->type & fmode) != fmode)
 		return 0;
-	if (test_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags))
-		return 0;
 	if (test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
 		return 0;
 	nfs_mark_delegation_referenced(delegation);
-- 
2.1.0

Re: [PATCH 1/2] NFSv4: Deal with atomic upgrades of an existing delegation

[Trond Myklebust]

On Fri, Dec 19, 2014 at 1:10 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> Ensure that we deal correctly with the case where the server sends us a
> newer instance of the same delegation. If the stateids match, but the
> sequence numbers differ, then treat the new delegation as if it were
> an atomic upgrade.
>
> Signed-off-by: Trond Myklebust <Trond.Myklebust@primarydata.com>
> ---
>  fs/nfs/delegation.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 7f3f60641344..90413cdbf254 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -301,6 +301,18 @@ nfs_inode_detach_delegation(struct inode *inode)
>         return nfs_detach_delegation(nfsi, delegation, server);
>  }
>
> +static void
> +nfs_update_inplace_delegation(struct nfs_delegation *delegation,
> +               struct nfs_delegation *update)
> +{
> +       if (nfs4_stateid_is_newer(&update->stateid, &delegation->stateid))
> +               return;
> +       delegation->stateid.seqid = update->stateid.seqid;
> +       smp_wmb();
> +       delegation->type == update->type;
> +       nfsi->delegation_state = update->type;

Oops... That wasn't intended. I'll resend an update that actually compiles...

> +}
> +
>  /**
>   * nfs_inode_set_delegation - set up a delegation on an inode
>   * @inode: inode to which delegation applies
> @@ -334,9 +346,11 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
>         old_delegation = rcu_dereference_protected(nfsi->delegation,
>                                         lockdep_is_held(&clp->cl_lock));
>         if (old_delegation != NULL) {
> -               if (nfs4_stateid_match(&delegation->stateid,
> -                                       &old_delegation->stateid) &&
> -                               delegation->type == old_delegation->type) {
> +               /* Is this an update of the existing delegation? */
> +               if (nfs4_stateid_match_other(&old_delegation->stateid,
> +                                       &delegation->stateid)) {
> +                       nfs_update_inplace_delegation(old_delegation,
> +                                       delegation);
>                         goto out;
>                 }
>                 /*
> --
> 2.1.0
>



-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

[PATCH v2 1/2] NFSv4: Deal with atomic upgrades of an existing delegation

[Trond Myklebust]

Ensure that we deal correctly with the case where the server sends us a
newer instance of the same delegation. If the stateids match, but the
sequence numbers differ, then treat the new delegation as if it were
an atomic upgrade.

Signed-off-by: Trond Myklebust <Trond.Myklebust@primarydata.com>
---
 fs/nfs/delegation.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 7f3f60641344..02b5b2a6d557 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -301,6 +301,19 @@ nfs_inode_detach_delegation(struct inode *inode)
 	return nfs_detach_delegation(nfsi, delegation, server);
 }
 
+static void
+nfs_update_inplace_delegation(struct nfs_inode *nfsi,
+		struct nfs_delegation *delegation,
+		struct nfs_delegation *update)
+{
+	if (nfs4_stateid_is_newer(&update->stateid, &delegation->stateid))
+		return;
+	delegation->stateid.seqid = update->stateid.seqid;
+	smp_wmb();
+	delegation->type = update->type;
+	nfsi->delegation_state = update->type;
+}
+
 /**
  * nfs_inode_set_delegation - set up a delegation on an inode
  * @inode: inode to which delegation applies
@@ -334,9 +347,11 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 	old_delegation = rcu_dereference_protected(nfsi->delegation,
 					lockdep_is_held(&clp->cl_lock));
 	if (old_delegation != NULL) {
-		if (nfs4_stateid_match(&delegation->stateid,
-					&old_delegation->stateid) &&
-				delegation->type == old_delegation->type) {
+		/* Is this an update of the existing delegation? */
+		if (nfs4_stateid_match_other(&old_delegation->stateid,
+					&delegation->stateid)) {
+			nfs_update_inplace_delegation(nfsi, old_delegation,
+					delegation);
 			goto out;
 		}
 		/*
-- 
2.1.0

[PATCH v2 2/2] NFSv4: Remove incorrect check in can_open_delegated()

[Trond Myklebust]

Remove an incorrect check for NFS_DELEGATION_NEED_RECLAIM in
can_open_delegated(). We are allowed to cache opens even in
a situation where we're doing reboot recovery.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs4proc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 8514b59a8c30..002c7dfedb08 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1119,8 +1119,6 @@ static int can_open_delegated(struct nfs_delegation *delegation, fmode_t fmode)
 		return 0;
 	if ((delegation->type & fmode) != fmode)
 		return 0;
-	if (test_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags))
-		return 0;
 	if (test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
 		return 0;
 	nfs_mark_delegation_referenced(delegation);
-- 
2.1.0

Re: [PATCH v2 1/2] NFSv4: Deal with atomic upgrades of an existing delegation

[Olga Kornievskaia]

Aha, yes yes yes, that would make sense how it is still returned if
seq ids are different. I'll have this tested asap.

On Fri, Dec 19, 2014 at 1:52 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> Ensure that we deal correctly with the case where the server sends us a
> newer instance of the same delegation. If the stateids match, but the
> sequence numbers differ, then treat the new delegation as if it were
> an atomic upgrade.
>
> Signed-off-by: Trond Myklebust <Trond.Myklebust@primarydata.com>
> ---
>  fs/nfs/delegation.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 7f3f60641344..02b5b2a6d557 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -301,6 +301,19 @@ nfs_inode_detach_delegation(struct inode *inode)
>         return nfs_detach_delegation(nfsi, delegation, server);
>  }
>
> +static void
> +nfs_update_inplace_delegation(struct nfs_inode *nfsi,
> +               struct nfs_delegation *delegation,
> +               struct nfs_delegation *update)
> +{
> +       if (nfs4_stateid_is_newer(&update->stateid, &delegation->stateid))
> +               return;
> +       delegation->stateid.seqid = update->stateid.seqid;
> +       smp_wmb();
> +       delegation->type = update->type;
> +       nfsi->delegation_state = update->type;
> +}
> +
>  /**
>   * nfs_inode_set_delegation - set up a delegation on an inode
>   * @inode: inode to which delegation applies
> @@ -334,9 +347,11 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
>         old_delegation = rcu_dereference_protected(nfsi->delegation,
>                                         lockdep_is_held(&clp->cl_lock));
>         if (old_delegation != NULL) {
> -               if (nfs4_stateid_match(&delegation->stateid,
> -                                       &old_delegation->stateid) &&
> -                               delegation->type == old_delegation->type) {
> +               /* Is this an update of the existing delegation? */
> +               if (nfs4_stateid_match_other(&old_delegation->stateid,
> +                                       &delegation->stateid)) {
> +                       nfs_update_inplace_delegation(nfsi, old_delegation,
> +                                       delegation);
>                         goto out;
>                 }
>                 /*
> --
> 2.1.0
>

Re: [PATCH v2 1/2] NFSv4: Deal with atomic upgrades of an existing delegation

[Trond Myklebust]

On Fri, Dec 19, 2014 at 1:52 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> Ensure that we deal correctly with the case where the server sends us a
> newer instance of the same delegation. If the stateids match, but the
> sequence numbers differ, then treat the new delegation as if it were
> an atomic upgrade.
>
> Signed-off-by: Trond Myklebust <Trond.Myklebust@primarydata.com>
> ---
>  fs/nfs/delegation.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 7f3f60641344..02b5b2a6d557 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -301,6 +301,19 @@ nfs_inode_detach_delegation(struct inode *inode)
>         return nfs_detach_delegation(nfsi, delegation, server);
>  }
>
> +static void
> +nfs_update_inplace_delegation(struct nfs_inode *nfsi,
> +               struct nfs_delegation *delegation,
> +               struct nfs_delegation *update)
> +{
> +       if (nfs4_stateid_is_newer(&update->stateid, &delegation->stateid))

...and the above comparison should be reversed.

> +               return;
> +       delegation->stateid.seqid = update->stateid.seqid;
> +       smp_wmb();
> +       delegation->type = update->type;
> +       nfsi->delegation_state = update->type;
> +}
> +
>  /**
>   * nfs_inode_set_delegation - set up a delegation on an inode
>   * @inode: inode to which delegation applies
> @@ -334,9 +347,11 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
>         old_delegation = rcu_dereference_protected(nfsi->delegation,
>                                         lockdep_is_held(&clp->cl_lock));
>         if (old_delegation != NULL) {
> -               if (nfs4_stateid_match(&delegation->stateid,
> -                                       &old_delegation->stateid) &&
> -                               delegation->type == old_delegation->type) {
> +               /* Is this an update of the existing delegation? */
> +               if (nfs4_stateid_match_other(&old_delegation->stateid,
> +                                       &delegation->stateid)) {
> +                       nfs_update_inplace_delegation(nfsi, old_delegation,
> +                                       delegation);
>                         goto out;
>                 }
>                 /*
> --
> 2.1.0
>



-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: [PATCH v2 1/2] NFSv4: Deal with atomic upgrades of an existing delegation

[Olga Kornievskaia]

On Fri, Dec 19, 2014 at 3:31 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> On Fri, Dec 19, 2014 at 1:52 PM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>> Ensure that we deal correctly with the case where the server sends us a
>> newer instance of the same delegation. If the stateids match, but the
>> sequence numbers differ, then treat the new delegation as if it were
>> an atomic upgrade.
>>
>> Signed-off-by: Trond Myklebust <Trond.Myklebust@primarydata.com>
>> ---
>>  fs/nfs/delegation.c | 21 ++++++++++++++++++---
>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
>> index 7f3f60641344..02b5b2a6d557 100644
>> --- a/fs/nfs/delegation.c
>> +++ b/fs/nfs/delegation.c
>> @@ -301,6 +301,19 @@ nfs_inode_detach_delegation(struct inode *inode)
>>         return nfs_detach_delegation(nfsi, delegation, server);
>>  }
>>
>> +static void
>> +nfs_update_inplace_delegation(struct nfs_inode *nfsi,
>> +               struct nfs_delegation *delegation,
>> +               struct nfs_delegation *update)
>> +{
>> +       if (nfs4_stateid_is_newer(&update->stateid, &delegation->stateid))
>
> ...and the above comparison should be reversed.

do you mean: if(!nfs4_stateid_is_newer())

but if we received a delegation stateid with sequence number lower
than what we have, shouldn't that be some kind of an error?

>
>> +               return;
>> +       delegation->stateid.seqid = update->stateid.seqid;
>> +       smp_wmb();
>> +       delegation->type = update->type;
>> +       nfsi->delegation_state = update->type;
>> +}
>> +
>>  /**
>>   * nfs_inode_set_delegation - set up a delegation on an inode
>>   * @inode: inode to which delegation applies
>> @@ -334,9 +347,11 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
>>         old_delegation = rcu_dereference_protected(nfsi->delegation,
>>                                         lockdep_is_held(&clp->cl_lock));
>>         if (old_delegation != NULL) {
>> -               if (nfs4_stateid_match(&delegation->stateid,
>> -                                       &old_delegation->stateid) &&
>> -                               delegation->type == old_delegation->type) {
>> +               /* Is this an update of the existing delegation? */
>> +               if (nfs4_stateid_match_other(&old_delegation->stateid,
>> +                                       &delegation->stateid)) {
>> +                       nfs_update_inplace_delegation(nfsi, old_delegation,
>> +                                       delegation);
>>                         goto out;
>>                 }
>>                 /*
>> --
>> 2.1.0
>>
>
>
>
> --
> Trond Myklebust
>
> Linux NFS client maintainer, PrimaryData
>
> trond.myklebust@primarydata.com

[PATCH v3 1/2] NFSv4: Deal with atomic upgrades of an existing delegation

[Trond Myklebust]

Ensure that we deal correctly with the case where the server sends us a
newer instance of the same delegation. If the stateids match, but the
sequence numbers differ, then treat the new delegation as if it were
an atomic upgrade.

Signed-off-by: Trond Myklebust <Trond.Myklebust@primarydata.com>
---
 fs/nfs/delegation.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 7f3f60641344..16b754ee0d09 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -301,6 +301,17 @@ nfs_inode_detach_delegation(struct inode *inode)
 	return nfs_detach_delegation(nfsi, delegation, server);
 }
 
+static void
+nfs_update_inplace_delegation(struct nfs_delegation *delegation,
+		const struct nfs_delegation *update)
+{
+	if (nfs4_stateid_is_newer(&update->stateid, &delegation->stateid)) {
+		delegation->stateid.seqid = update->stateid.seqid;
+		smp_wmb();
+		delegation->type = update->type;
+	}
+}
+
 /**
  * nfs_inode_set_delegation - set up a delegation on an inode
  * @inode: inode to which delegation applies
@@ -334,9 +345,12 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 	old_delegation = rcu_dereference_protected(nfsi->delegation,
 					lockdep_is_held(&clp->cl_lock));
 	if (old_delegation != NULL) {
-		if (nfs4_stateid_match(&delegation->stateid,
-					&old_delegation->stateid) &&
-				delegation->type == old_delegation->type) {
+		/* Is this an update of the existing delegation? */
+		if (nfs4_stateid_match_other(&old_delegation->stateid,
+					&delegation->stateid)) {
+			nfs_update_inplace_delegation(old_delegation,
+					delegation);
+			nfsi->delegation_state = old_delegation->type;
 			goto out;
 		}
 		/*
-- 
2.1.0

[PATCH v3 2/2] NFSv4: Remove incorrect check in can_open_delegated()

[Trond Myklebust]

Remove an incorrect check for NFS_DELEGATION_NEED_RECLAIM in
can_open_delegated(). We are allowed to cache opens even in
a situation where we're doing reboot recovery.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs4proc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 8514b59a8c30..002c7dfedb08 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1119,8 +1119,6 @@ static int can_open_delegated(struct nfs_delegation *delegation, fmode_t fmode)
 		return 0;
 	if ((delegation->type & fmode) != fmode)
 		return 0;
-	if (test_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags))
-		return 0;
 	if (test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
 		return 0;
 	nfs_mark_delegation_referenced(delegation);
-- 
2.1.0

Re: [PATCH v2 1/2] NFSv4: Deal with atomic upgrades of an existing delegation

[Trond Myklebust]

On Fri, Dec 19, 2014 at 3:39 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> On Fri, Dec 19, 2014 at 3:31 PM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>> On Fri, Dec 19, 2014 at 1:52 PM, Trond Myklebust
>> <trond.myklebust@primarydata.com> wrote:
>>> Ensure that we deal correctly with the case where the server sends us a
>>> newer instance of the same delegation. If the stateids match, but the
>>> sequence numbers differ, then treat the new delegation as if it were
>>> an atomic upgrade.
>>>
>>> Signed-off-by: Trond Myklebust <Trond.Myklebust@primarydata.com>
>>> ---
>>>  fs/nfs/delegation.c | 21 ++++++++++++++++++---
>>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
>>> index 7f3f60641344..02b5b2a6d557 100644
>>> --- a/fs/nfs/delegation.c
>>> +++ b/fs/nfs/delegation.c
>>> @@ -301,6 +301,19 @@ nfs_inode_detach_delegation(struct inode *inode)
>>>         return nfs_detach_delegation(nfsi, delegation, server);
>>>  }
>>>
>>> +static void
>>> +nfs_update_inplace_delegation(struct nfs_inode *nfsi,
>>> +               struct nfs_delegation *delegation,
>>> +               struct nfs_delegation *update)
>>> +{
>>> +       if (nfs4_stateid_is_newer(&update->stateid, &delegation->stateid))
>>
>> ...and the above comparison should be reversed.
>
> do you mean: if(!nfs4_stateid_is_newer())
>
> but if we received a delegation stateid with sequence number lower
> than what we have, shouldn't that be some kind of an error?

See the v3 patch. Yes, it would be a bug if the server sent us
something with a lower number, so we ignore that and don't update the
delegation. Ditto if it sends us something with the same sequence
number.

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: [PATCH v3 2/2] NFSv4: Remove incorrect check in can_open_delegated()

[Olga Kornievskaia]

This is the patch that breaks recovery of opens upon server reboot.

I have a test that open a file and gets a write delegation and tried
to do a write. At this point, I reboot my server so that write fails
with bad_session and then stale_clientid. Upon completing the
exchange_id, create_session, and putrootfh, the client no longer sends
the open to be recovered and instead resends the failed write. It
would use all 0s stateid (this is 4.1) for the write and for the close
that follows it.

Reverting the patch fixes the problem.

On Fri, Dec 19, 2014 at 3:44 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> Remove an incorrect check for NFS_DELEGATION_NEED_RECLAIM in
> can_open_delegated(). We are allowed to cache opens even in
> a situation where we're doing reboot recovery.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  fs/nfs/nfs4proc.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 8514b59a8c30..002c7dfedb08 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1119,8 +1119,6 @@ static int can_open_delegated(struct nfs_delegation *delegation, fmode_t fmode)
>                 return 0;
>         if ((delegation->type & fmode) != fmode)
>                 return 0;
> -       if (test_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags))
> -               return 0;
>         if (test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
>                 return 0;
>         nfs_mark_delegation_referenced(delegation);
> --
> 2.1.0
>

Re: [PATCH v3 2/2] NFSv4: Remove incorrect check in can_open_delegated()

[Trond Myklebust]

On Tue, Aug 18, 2015 at 1:38 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>
> This is the patch that breaks recovery of opens upon server reboot.
>
> I have a test that open a file and gets a write delegation and tried
> to do a write. At this point, I reboot my server so that write fails
> with bad_session and then stale_clientid. Upon completing the
> exchange_id, create_session, and putrootfh, the client no longer sends
> the open to be recovered and instead resends the failed write. It
> would use all 0s stateid (this is 4.1) for the write and for the close
> that follows it.

Why is the client not attempting to recover the open? That's a bug,
whether or not this patch is correct.

> On Fri, Dec 19, 2014 at 3:44 PM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
> > Remove an incorrect check for NFS_DELEGATION_NEED_RECLAIM in
> > can_open_delegated(). We are allowed to cache opens even in
> > a situation where we're doing reboot recovery.
> >
> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> > ---
> >  fs/nfs/nfs4proc.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 8514b59a8c30..002c7dfedb08 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -1119,8 +1119,6 @@ static int can_open_delegated(struct nfs_delegation *delegation, fmode_t fmode)
> >                 return 0;
> >         if ((delegation->type & fmode) != fmode)
> >                 return 0;
> > -       if (test_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags))
> > -               return 0;
> >         if (test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
> >                 return 0;
> >         nfs_mark_delegation_referenced(delegation);
> > --
> > 2.1.0
> >

Re: [PATCH v3 2/2] NFSv4: Remove incorrect check in can_open_delegated()

[Olga Kornievskaia]

On Tue, Aug 18, 2015 at 5:07 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> On Tue, Aug 18, 2015 at 1:38 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>
>> This is the patch that breaks recovery of opens upon server reboot.
>>
>> I have a test that open a file and gets a write delegation and tried
>> to do a write. At this point, I reboot my server so that write fails
>> with bad_session and then stale_clientid. Upon completing the
>> exchange_id, create_session, and putrootfh, the client no longer sends
>> the open to be recovered and instead resends the failed write. It
>> would use all 0s stateid (this is 4.1) for the write and for the close
>> that follows it.
>
> Why is the client not attempting to recover the open? That's a bug,
> whether or not this patch is correct.

I added printks in the path of nfs4_do_reclaim(). Client does
"attempt" to recover the open. OPEN never goes on the wire because
can_open_delegated() returned true and no action of adding an rpc task
gets done.

>
>> On Fri, Dec 19, 2014 at 3:44 PM, Trond Myklebust
>> <trond.myklebust@primarydata.com> wrote:
>> > Remove an incorrect check for NFS_DELEGATION_NEED_RECLAIM in
>> > can_open_delegated(). We are allowed to cache opens even in
>> > a situation where we're doing reboot recovery.
>> >
>> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>> > ---
>> >  fs/nfs/nfs4proc.c | 2 --
>> >  1 file changed, 2 deletions(-)
>> >
>> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> > index 8514b59a8c30..002c7dfedb08 100644
>> > --- a/fs/nfs/nfs4proc.c
>> > +++ b/fs/nfs/nfs4proc.c
>> > @@ -1119,8 +1119,6 @@ static int can_open_delegated(struct nfs_delegation *delegation, fmode_t fmode)
>> >                 return 0;
>> >         if ((delegation->type & fmode) != fmode)
>> >                 return 0;
>> > -       if (test_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags))
>> > -               return 0;
>> >         if (test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
>> >                 return 0;
>> >         nfs_mark_delegation_referenced(delegation);
>> > --
>> > 2.1.0
>> >

Re: [PATCH v3 2/2] NFSv4: Remove incorrect check in can_open_delegated()

[Trond Myklebust]

On Tue, Aug 18, 2015 at 2:13 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> On Tue, Aug 18, 2015 at 5:07 PM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>> On Tue, Aug 18, 2015 at 1:38 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>
>>> This is the patch that breaks recovery of opens upon server reboot.
>>>
>>> I have a test that open a file and gets a write delegation and tried
>>> to do a write. At this point, I reboot my server so that write fails
>>> with bad_session and then stale_clientid. Upon completing the
>>> exchange_id, create_session, and putrootfh, the client no longer sends
>>> the open to be recovered and instead resends the failed write. It
>>> would use all 0s stateid (this is 4.1) for the write and for the close
>>> that follows it.
>>
>> Why is the client not attempting to recover the open? That's a bug,
>> whether or not this patch is correct.
>
> I added printks in the path of nfs4_do_reclaim(). Client does
> "attempt" to recover the open. OPEN never goes on the wire because
> can_open_delegated() returned true and no action of adding an rpc task
> gets done.

Ahh... OK. I'll fix that.

Thanks Olga!

>>
>>> On Fri, Dec 19, 2014 at 3:44 PM, Trond Myklebust
>>> <trond.myklebust@primarydata.com> wrote:
>>> > Remove an incorrect check for NFS_DELEGATION_NEED_RECLAIM in
>>> > can_open_delegated(). We are allowed to cache opens even in
>>> > a situation where we're doing reboot recovery.
>>> >
>>> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>>> > ---
>>> >  fs/nfs/nfs4proc.c | 2 --
>>> >  1 file changed, 2 deletions(-)
>>> >
>>> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> > index 8514b59a8c30..002c7dfedb08 100644
>>> > --- a/fs/nfs/nfs4proc.c
>>> > +++ b/fs/nfs/nfs4proc.c
>>> > @@ -1119,8 +1119,6 @@ static int can_open_delegated(struct nfs_delegation *delegation, fmode_t fmode)
>>> >                 return 0;
>>> >         if ((delegation->type & fmode) != fmode)
>>> >                 return 0;
>>> > -       if (test_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags))
>>> > -               return 0;
>>> >         if (test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
>>> >                 return 0;
>>> >         nfs_mark_delegation_referenced(delegation);
>>> > --
>>> > 2.1.0
>>> >