Subject: [nfsv4] Delaying RELEASE_LOCKOWNER and server-side seqid state

From: Saul Tamari <[email-masked]>
Date: Mon, 23 Sep 2013 11:36:26 -0700

Hi,

Currently RFC3530 does not allow NFS4ERR_DELAY to be replied for
RELEASE_LOCKOWNER.
This seems to cause an issue if the server temporarily can't complete
the operation.

I've noticed that if the server replies with NFS4ERR_DELAY to
RELEASE_LOCKOWNER then current Linux clients will ignore this error
and proceed, sending a lock request with new_lock_owner, lock_seqid=0
and reuse the lock_owner that was recently (not) released.
This causes a sequence id matching failure because the server did not
release the old lock-owner and its associated seqid.

Would it be possible to change the bis to allow adding NFS4ERR_DELAY
to the current list of allowed errors for RELEASE_LOCKOWNER?

RFC3530 lists the following allowed replies for RELEASE_LOCKOWNER:
      NFS4ERR_ADMIN_REVOKED
      NFS4ERR_BADXDR
      NFS4ERR_EXPIRED
      NFS4ERR_LEASE_MOVED
      NFS4ERR_LOCKS_HELD
      NFS4ERR_RESOURCE
      NFS4ERR_SERVERFAULT
      NFS4ERR_STALE_CLIENTID

Thanks,
Saul

---

From: "J. Bruce Fields" <[email-masked]>
Date: Mon, 23 Sep 2013 15:20:20 -0400

> to the current list of allowed errors for RELEASE_LOCKOWNER?

If you fix this problem then you'll also find other ways the Linux
client's (at least) handling of DELAY falls short of what you want:

	http://www.ietf.org/mail-archive/web/nfsv4/current/msg11720.html

I don't think DELAY was really meant for this.

Maybe this could be better documented.  It seems several servers have
tried to use DELAY in places other than delegation-breaks and run into
this kind of trouble.

--b.

> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4

---

From: "Noveck, David" <[email-masked]>
Date: Mon, 23 Sep 2013 19:22:00 -0400

> Would it be possible to change the bis to allow adding NFS4ERR_DELAY to the current list of allowed errors for RELEASE_LOCKOWNER?

I think this is a reasonable change, although  I sense that some people might have trouble with it.

I'm not clear the issue will be resolved in time for rfc3530bis.

> RFC3530 lists the following allowed replies for RELEASE_LOCKOWNER:

In the case of RESOURCE or SERVERFAULT, I don't think that a client would have any basis
to assume that the operation was done successfully, as seems to be the case with the Linux
client and DELAY.  In fact, I can see a client deciding that, since DELAY is disallowed, any
server which returns it is de facto broken and treating this as a SERVERFAULT equivalent.

Given the nature of RELEASE_LOCKOWNER and its use to prune state, I can see a client just
continuing, leaving one owner unreleased., when a RELEASE_LOCKOWNER fails

As far as ADMIN_REVOKED and EXPIRED, I'm wondering whether these are appropriate.  If there
are locks existing, as far as the client is concerned  then LOCKS_HELD seems to me the more appropriate
error.

-----Original Message-----
From: [email-masked] [mailto:[email-masked]] On Behalf Of Saul Tamari
Sent: Monday, September 23, 2013 2:36 PM
To: nfsv4
Subject: [nfsv4] Delaying RELEASE_LOCKOWNER and server-side seqid state

Hi,

Currently RFC3530 does not allow NFS4ERR_DELAY to be replied for RELEASE_LOCKOWNER.
This seems to cause an issue if the server temporarily can't complete the operation.

I've noticed that if the server replies with NFS4ERR_DELAY to RELEASE_LOCKOWNER then current Linux clients will ignore this error and proceed, sending a lock request with new_lock_owner, lock_seqid=0 and reuse the lock_owner that was recently (not) released.
This causes a sequence id matching failure because the server did not release the old lock-owner and its associated seqid.

Would it be possible to change the bis to allow adding NFS4ERR_DELAY to the current list of allowed errors for RELEASE_LOCKOWNER?

Thanks,
Saul

---

From: Rick Macklem <[email-masked]>
Date: Mon, 23 Sep 2013 20:28:33 -0400 (EDT)

David Noveck wrote:
> > Would it be possible to change the bis to allow adding
> > NFS4ERR_DELAY to the current list of allowed errors for
> > RELEASE_LOCKOWNER?
> 
> are locks existing, as far as the client is concerned  then
> LOCKS_HELD seems to me the more appropriate
> error.
> 
If the clientid of the lock_owner argument has an expired lease,
I'd think NFS4ERR_EXPIRED would be appropriate. I can also see
the clientid being administratively revoked in a server.

Although I don't have a strong opinion w.r.t. whether or not
NFS4ERR_DELAY should be allowed, the FreeBSD client will accept
the error and try the RPC again after a delay.

rick

> and proceed, sending a lock request with new_lock_owner,
> lock_seqid=0 and reuse the lock_owner that was recently (not)
> released.

---

From: Saul Tamari <[email-masked]>
Date: Tue, 24 Sep 2013 06:35:47 -0700

> On Mon, Sep 23, 2013 at 11:36:26AM -0700, Saul Tamari wrote:
[snip]
> tried to use DELAY in places other than delegation-breaks and run into

I have a good experience with delaying a seqid-mutating operation
(e.g. OPEN or LOCK) by returning a NFS4ERR_DELAY on an earlier
operation (like PUTFH).
It doesn't require changes to server state and it works with current
client implementations.

The issue with RELEASE_LOCKOWNER is that it changes server state (by
removing the seq-id associated with the lock-owner) and usually is
sent in a single op COMPOUND so there isn't an earlier OP to reply
with NFS4ERR_DELAY.

Thanks,
Saul

---

From: Piyush Shivam <[email-masked]>
Date: Wed, 25 Sep 2013 10:59:36 -0500

> I don't think DELAY was really meant for this.
We have been wrestling with a number of issues to make NFS migration 
implementable within the context of 3530bis and to some extent 5661. The 
details are available at: 
http://tools.ietf.org/html/draft-ietf-nfsv4-migration-issues-04.

In the above draft, we recommend the use of NFS4ERR_DELAY on certain ops 
to prevent the modification of the locking state on the source server, 
while the transfer of such locking state is in progress. Please see: 
http://tools.ietf.org/html/draft-ietf-nfsv4-rfc3530-migration-update-02#section-6.2 
for more details on how the migration draft is recommending the use of 
NFS4ERR_DELAY on certain ops.

While such use of NFS4ERR_DELAY may not be in line with its original 
intentions, it appears to be a fundamental recommendation to make 
migration implementable within 3530bis. As per that recommendation, if 
NFS4ERR_DELAY were a legal error for RELEASE_LOCKOWNER, then the 
processing of this op just falls in place when the transfer of locking 
state is in progress from source to destination. However, since 
NFS4ERR_DELAY is not a legal error, it is unclear as to how to handle 
the op at the source server, while the migration is still going on. 
After having worked through all ops and scenarios, RELEASE_LOCKOWNER is 
the only op that appears to be problematic.

So, we would like to better understand the objections, if any, to adding 
NFS4ERR_DELAY to a set of legal errors for RELEASE_LOCKOWNER.

Thanks,
Piyush

---

From: "J. Bruce Fields" <[email-masked]>
Date: Wed, 25 Sep 2013 16:12:29 -0400

On Wed, Sep 25, 2013 at 10:59:36AM -0500, Piyush Shivam wrote:
> implementable within the context of 3530bis and to some extent 5661.
> The details are available at:
> In the above draft, we recommend the use of NFS4ERR_DELAY on certain
> ops to prevent the modification of the locking state on the source
> server, while the transfer of such locking state is in progress.
> Please see: http://tools.ietf.org/html/draft-ietf-nfsv4-rfc3530-migration-update-02#section-6.2
> for more details on how the migration draft is recommending the use
> of NFS4ERR_DELAY on certain ops.

It recommends special handling for RELEASE_LOCKOWNER to account for the
fact that it doesn't allow NFS4ERR_DELAY:

	"RELEASE_LOCKOWNER can be handled by propagating the fact of the
	lockowner deletion (e.g. by using an RPC) to the destination
	server...."

> migration implementable within 3530bis. As per that recommendation,
> if NFS4ERR_DELAY were a legal error for RELEASE_LOCKOWNER, then the
> processing of this op just falls in place when the transfer of
> locking state is in progress from source to destination. However,
> since NFS4ERR_DELAY is not a legal error, it is unclear as to how to
> handle the op at the source server, while the migration is still
> going on. After having worked through all ops and scenarios,
> RELEASE_LOCKOWNER is the only op that appears to be problematic.
> 
> So, we would like to better understand the objections, if any, to
> adding NFS4ERR_DELAY to a set of legal errors for RELEASE_LOCKOWNER.

I appreciate that it would be simpler to return DELAY instead of
handling RELEASE_LOCKOWNER in this special way.  But it appears that
existing deployed clients won't handle that.  It would seem unhelpful
for the spec not to mention that fact.

--b.

---

From: Piyush Shivam <[email-masked]>
Date: Wed, 25 Sep 2013 17:23:10 -0500

> 	lockowner deletion (e.g. by using an RPC) to the destination

Right. I should have known, since I reviewed that text. :)

> for the spec not to mention that fact.

Fair point. There is a risk of affecting existing deployed clients with 
addition newer error codes. A couple of points related to that.

1. I just did a comparison of ops that have NFS4ERR_DELAY as a legal 
error in 3530 versus ops that have NFS4ERR_DELAY as a legal error in 
3530bis. We added NFS4ERR_DELAY as a legal error in 3530bis for 15 
additional ops:  CB_GETATTR, CB_RECALL, COMMIT, DELEGPURGE, DELEGRETURN, 
LOCKU, LOOKUP, LOOKUPP, OPEN_DOWNGRADE, PUTFH, PUTPUBFH, PUTROOTFH, 
SECINFO, SETCLIENTID, SETCLIENTID_CONFIRM.

I would make a case that adding RELEASE_LOCKOWNER to the list above 
would not affect interoperability any more than adding NFS4ERR_DELAY for 
the above ops. Perhaps we need to add a general text in the 3530bis 
indicating the issue of newer error codes w.r.t. already deployed clients?

I found one email thread covering the rationale for adding NFS4ERR_DELAY 
for some of above ops: 
http://www.ietf.org/mail-archive/web/nfsv4/current/msg07848.html .

2.The primary case where the clients would encounter NFS4ERR_DELAY will 
be with a migration-aware client and server. The client changes are 
required to support migration anyway, and there are no clients deployed 
currently that support migration. That does not eliminate the 
possibility of a server returning NFS4ERR_DELAY to a non-migration 
capable client, but IMO, it does reduce the window of exposure considerably.

Thanks,
Piyush

---

From: "Haynes, Tom" <[email-masked]>
Date: Thu, 26 Sep 2013 04:25:10 +0000

On Sep 25, 2013, at 4:12 PM, J. Bruce Fields <[email-masked]> wrote:

> existing deployed clients won't handle that.  

What expectation is there that there is complete interoperability between
3530 compliant and 3530bis compliant implementations?

Despite our best efforts at reaching out to implementers to get feedback
on some of the changes, we could be breaking their code.

> It would seem unhelpful

Is your argument that you are okay with the change as long as we call
out a warning?

---

From: "Noveck, David" <[email-masked]>
Date: Thu, 26 Sep 2013 08:40:54 -0400

Would it satisfy your objection to say that servers that do not implement migration MUST NOT return
DELAY to RELEASE_LOCKOWNER (and that servers that do MAY do so)?  So, existing clients that do not 
support migration, would not be bothered by the issue.

If there is general agreement that this is OK, I can include that change in the next update of
draft-ietf-nfsv4-rfc3530-migration-update.

-----Original Message-----
From: [email-masked] [mailto:[email-masked]] On Behalf Of J. Bruce Fields
Sent: Wednesday, September 25, 2013 4:12 PM
To: Piyush Shivam
Cc: nfsv4
Subject: Re: [nfsv4] Delaying RELEASE_LOCKOWNER and server-side seqid state

On Wed, Sep 25, 2013 at 10:59:36AM -0500, Piyush Shivam wrote:
> >>lock_seqid=0 and reuse the lock_owner that was recently (not) released.
> >>This causes a sequence id matching failure because the server did 
> >>not release the old lock-owner and its associated seqid.
> >>
> Please see: 
> http://tools.ietf.org/html/draft-ietf-nfsv4-rfc3530-migration-update-0
> 2#section-6.2 for more details on how the migration draft is 
> recommending the use of NFS4ERR_DELAY on certain ops.

It recommends special handling for RELEASE_LOCKOWNER to account for the fact that it doesn't allow NFS4ERR_DELAY:

> After having worked through all ops and scenarios, RELEASE_LOCKOWNER 
> is the only op that appears to be problematic.
> 

I appreciate that it would be simpler to return DELAY instead of handling RELEASE_LOCKOWNER in this special way.  But it appears that existing deployed clients won't handle that.  It would seem unhelpful for the spec not to mention that fact.

--b.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Thu, 26 Sep 2013 10:22:08 -0400

On Wed, Sep 25, 2013 at 05:23:10PM -0500, Piyush Shivam wrote:
> Fair point. There is a risk of affecting existing deployed clients
> with addition newer error codes. A couple of points related to that.
> 
> additional ops:  CB_GETATTR, CB_RECALL, COMMIT, DELEGPURGE,
> DELEGRETURN, LOCKU, LOOKUP, LOOKUPP, OPEN_DOWNGRADE, PUTFH,
> PUTPUBFH, PUTROOTFH, SECINFO, SETCLIENTID, SETCLIENTID_CONFIRM.
> 
> would not affect interoperability any more than adding NFS4ERR_DELAY
> for the above ops. Perhaps we need to add a general text in the
> 3530bis indicating the issue of newer error codes w.r.t. already
> deployed clients?

I don't know.  My (possibly faulty) memory is that the 3530 error lists
had some important ommissions, and vague warnings about using the new
lists may just make implementer's lives more difficult.

So if we want to do that then somebody probably needs to diff the 3530
and 3530bis lists and work through them on a case-by-case basis.  (I'm
not volunteering.)

Absent that we're probably best off assuming we the changes are right
until someone demonstrates the contrary.

> I found one email thread covering the rationale for adding
> NFS4ERR_DELAY for some of above ops:
> 2.The primary case where the clients would encounter NFS4ERR_DELAY
> will be with a migration-aware client and server. The client changes
> are required to support migration anyway, and there are no clients
> deployed currently that support migration. That does not eliminate
> the possibility of a server returning NFS4ERR_DELAY to a
> non-migration capable client, but IMO, it does reduce the window of
> exposure considerably.

Sounds OK.  Looks like Dave Noveck has more or less the same suggestion.

Disclaimer: all I have is Saul's original report, I haven't looked into
it any closer.  Maybe the behavior he describes is a recent bug that
could just be fixed--I defer to Saul and Trond on that question.

Assuming this is really the behavior of every Linux client out there
then I think we're stuck with this nit, and the suggestion from you and
Dave sounds reasonable.

--b.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Thu, 26 Sep 2013 10:22:58 -0400

> 3530 compliant and 3530bis compliant implementations?

Sounds like pretty much requirement #1 to me.

> on some of the changes, we could be breaking their code.

Yep.  Mistakes happen.

--b.

---

From: "Myklebust, Trond" <[email-masked]>
Date: Thu, 26 Sep 2013 14:28:32 +0000

> From: [email-masked] [mailto:[email-masked]] On Behalf
> Of Haynes, Tom
> Sent: Wednesday, September 25, 2013 11:25 PM
> To: J. Bruce Fields
> Cc: nfsv4
> Despite our best efforts at reaching out to implementers to get feedback on
> some of the changes, we could be breaking their code.

The point of RFC3530bis should be to sync the spec to the existing implementations. It should certainly not be creating new discrepancies with those implementations.

> Is your argument that you are okay with the change as long as we call out a
> warning?

I am not OK with such a change.

Trond

---

From: Saul Tamari <[email-masked]>
Date: Thu, 26 Sep 2013 13:53:53 -0700

On Thu, Sep 26, 2013 at 7:22 AM, J. Bruce Fields <[email-masked]> wrote:
[snip]
> then I think we're stuck with this nit, and the suggestion from you and

My experience with current Linux clients (I tried Linux 2.6.32 and
3.5) is that when such a client sends a RELEASE_LOCKOWNER op and
receives an NFS4ERR_DELAY, the client ignores the error and continues
with its work. I didn't experience any client issue with this
scenario.

Saul

---

From: "Myklebust, Trond" <[email-masked]>
Date: Thu, 26 Sep 2013 20:59:39 +0000

> Of Saul Tamari
> Sent: Thursday, September 26, 2013 3:54 PM
> To: J. Bruce Fields
> Cc: nfsv4
> > Disclaimer: all I have is Saul's original report, I haven't looked
> > into it any closer.  Maybe the behavior he describes is a recent bug
> > that could just be fixed--I defer to Saul and Trond on that question.
> >
> > then I think we're stuck with this nit, and the suggestion from you
> > and Dave sounds reasonable.
> 
> 3.5) is that when such a client sends a RELEASE_LOCKOWNER op and receives
> an NFS4ERR_DELAY, the client ignores the error and continues with its work. I
> didn't experience any client issue with this scenario.

I thought you said that you  _did_ experience errors when the client later reuses that lock owner and gets a bad sequence id? That doesn't sound acceptable.

Trond

---

From: Saul Tamari <[email-masked]>
Date: Thu, 26 Sep 2013 14:09:44 -0700

The errors I saw where not for the RELEASE_LOCKOWNER but for later ops
that reuse the same lock-owner which RELEASE_LOCKOWNER tried to
release.
When at a later point the client reuses the lock-owner which was (not)
released by the release-lockowner op then the application using this
lock (google-chrome in my test) gets stuck with infinite LOCK retries
receiving NFS4ERR_BAD_SEQID replies.

---

From: "Myklebust, Trond" <[email-masked]>
Date: Thu, 26 Sep 2013 21:25:58 +0000

> From: Saul Tamari [mailto:[email-masked]]
> Sent: Thursday, September 26, 2013 4:10 PM
> To: Myklebust, Trond
> Cc: J. Bruce Fields; nfsv4
> released by the release-lockowner op then the application using this lock
> (google-chrome in my test) gets stuck with infinite LOCK retries receiving
> NFS4ERR_BAD_SEQID replies.

...and that is exactly what makes the NFS4ERR_DELAY reply to RELEASE_LOCKOWNER unacceptable.

Trond

---

From: Rick Macklem <[email-masked]>
Date: Thu, 26 Sep 2013 18:07:05 -0400 (EDT)

> support migration, would not be bothered by the issue.
I'm being nitpicky, but the above words don't actually say that a server
can only return NFS4ERR_DELAY when doing a migration, which is what I
think you intended. (To me, the above seems to allow a server that implements
migration to return NFS4ERR_DELAY for RELEASE_LOCKOWNER whenever it feels like
doing so and not just when doing a migration?)

rick

> > >>NFS4ERR_DELAY
> It recommends special handling for RELEASE_LOCKOWNER to account for
> the fact that it doesn't allow NFS4ERR_DELAY:
> 
> > if
> > locking
> existing deployed clients won't handle that.  It would seem
> unhelpful for the spec not to mention that fact.
> 
> --b.

---

From: "Noveck, David" <[email-masked]>
Date: Thu, 26 Sep 2013 18:47:25 -0400

Yes, you're being nitpicky, but that's what you're supposed to be when reviewing this stuff.

I'm not sure how I can say it can only return DELAY, when doing the migration.  When the client
receives DELAY, he hasn't found out about the migration.  In fact, a server can be gathering up state
to send to the destination, and only later find out, that the destination has decided to abort the 
transfer.   In that case the server will have returned DELAY with no actual migration occurring.

How about:

        In order to make server state fixed for a filesystem being migrated, a server MAY return
        NFS4ERR_DELAY in response to RELEASE_LOCKOWNER.   It MUST NOT do so unless it is
        likely that it will later return NFS4ERR_MOVED for the filesystem in question.  Thus, clients
        that unprepared to receive NFS4ERR_MOVED after creating state associated with a given
        filesystem (i.e. those unprepared for filesystem migration) are assured that they will never
        encounter NFS4ERR_DELAY in response to RELEASE_LOCKOWNER.  

-----Original Message-----
From: Rick Macklem [mailto:[email-masked]] 
Sent: Thursday, September 26, 2013 6:07 PM
To: Noveck, David
Cc: nfsv4; J. Bruce Fields; Piyush Shivam
Subject: Re: [nfsv4] Delaying RELEASE_LOCKOWNER and server-side seqid state

David Noveck wrote:
> Would it satisfy your objection to say that servers that do not 
> implement migration MUST NOT return DELAY to RELEASE_LOCKOWNER (and 
> that servers that do MAY do so)?  So, existing clients that do not 
I'm being nitpicky, but the above words don't actually say that a server can only return NFS4ERR_DELAY when doing a migration, which is what I think you intended. (To me, the above seems to allow a server that implements migration to return NFS4ERR_DELAY for RELEASE_LOCKOWNER whenever it feels like doing so and not just when doing a migration?)

rick

> Of J. Bruce Fields
> > >>RELEASE_LOCKOWNER then current Linux clients will ignore this 
> > >>error and proceed, sending a lock request with new_lock_owner,
> > going on.