Subject: [nfsv4] Proposed erratum for RFC5661: allow LAYOUTCOMMIT to fail with NFS4ERR_BAD_STATEID

From: Benny Halevy <[email-masked]>
Date: Wed, 30 Oct 2013 05:14:32 +0200

Since there were no objections to doing that
http://www.ietf.org/mail-archive/web/nfsv4/current/msg12383.html

I propose the following change:

In section 18.42.3, change:

   The loca_stateid is a layout stateid value as returned by previously
   successful layout operations (see Section 12.5.3).

to:

   The loca_stateid is a valid layout stateid value as returned by previously
   successful layout operations (see Section 12.5.3).  If the the layout stateid
   is no longer valid after e.g., successful LAYOUTRETURN, CLOSE for a layout
   marked with logr_return_on_close set, or CB_LAYOUTRECALL returning the
   NFS4ERR_NOMATCHING_LAYOUT, the error NFS4ERR_BAD_STATEID is returned.
   If the layout was revoked by the server the error NFS4ERR_ADMIN_REVOKED is returned.

Respectively, in section 15.2:

   | LAYOUTCOMMIT         | NFS4ERR_ACCESS, NFS4ERR_ADMIN_REVOKED,     |
   |                      | NFS4ERR_ATTRNOTSUPP, NFS4ERR_BADIOMODE,    |
   |                      | NFS4ERR_BADLAYOUT, NFS4ERR_BADXDR, NFS4ERR_BAD_STATEID,         |
   |                      | NFS4ERR_DEADSESSION, NFS4ERR_DELAY,        |
   |                      | NFS4ERR_DELEG_REVOKED, NFS4ERR_EXPIRED,    |
   |                      | NFS4ERR_FBIG, NFS4ERR_FHEXPIRED,           |
   |                      | NFS4ERR_GRACE, NFS4ERR_INVAL, NFS4ERR_IO,  |
   |                      | NFS4ERR_ISDIR NFS4ERR_MOVED,               |
   |                      | NFS4ERR_NOFILEHANDLE, NFS4ERR_NOTSUPP,     |
   |                      | NFS4ERR_NO_GRACE,                          |
   |                      | NFS4ERR_OP_NOT_IN_SESSION,                 |
   |                      | NFS4ERR_RECLAIM_BAD,                       |
   |                      | NFS4ERR_RECLAIM_CONFLICT,                  |
   |                      | NFS4ERR_REP_TOO_BIG,                       |
   |                      | NFS4ERR_REP_TOO_BIG_TO_CACHE,              |
   |                      | NFS4ERR_REQ_TOO_BIG,                       |
   |                      | NFS4ERR_RETRY_UNCACHED_REP,                |
   |                      | NFS4ERR_SERVERFAULT, NFS4ERR_STALE,        |
   |                      | NFS4ERR_SYMLINK, NFS4ERR_TOO_MANY_OPS,     |
   |                      | NFS4ERR_UNKNOWN_LAYOUTTYPE,                |
   |                      | NFS4ERR_WRONG_CRED                         |

section 15.4:

   | NFS4ERR_BAD_STATEID               | CB_LAYOUTRECALL, CB_NOTIFY,   |
   |                                   | CB_NOTIFY_LOCK, CB_RECALL,    |
   |                                   | CLOSE, DELEGRETURN,           |
   |                                   | FREE_STATEID, LAYOUTGET, LAYOUTCOMMIT     |
   |                                   | LAYOUTRETURN, LOCK, LOCKU,    |
   |                                   | OPEN, OPEN_DOWNGRADE, READ,   |
   |                                   | SETATTR, WRITE                |

---

From: "Noveck, David" <[email-masked]>
Date: Wed, 30 Oct 2013 06:14:07 -0400

>  If the layout was revoked by the server the error NFS4ERR_ADMIN_REVOKED is returned.

How about: 

     If the layout was previously revoked by the server, one of  the errors NFS4ERR_EXPIRED  or
     NFS4ERR_ADMIN_REVOKED is returned, as appropriate.

BTW, I don't see why GRACE and NO_GRACE are in this list. 

-----Original Message-----
From: Benny Halevy [mailto:[email-masked]] 
Sent: Tuesday, October 29, 2013 11:15 PM
To: nfsv4 mailing list; Haynes, Tom; Noveck, David
Cc: Benny Halevy
Subject: Proposed erratum for RFC5661: allow LAYOUTCOMMIT to fail with NFS4ERR_BAD_STATEID

Since there were no objections to doing that http://www.ietf.org/mail-archive/web/nfsv4/current/msg12383.html

to:

section 15.4:

---

From: Benny Halevy <[email-masked]>
Date: Wed, 30 Oct 2013 13:13:17 +0200

>      NFS4ERR_ADMIN_REVOKED is returned, as appropriate.

Looks good. thanks.

> BTW, I don't see why GRACE and NO_GRACE are in this list.

loca_reclaim

---

From: "Myklebust, Trond" <[email-masked]>
Date: Wed, 30 Oct 2013 12:57:53 +0000

>   |                                   | OPEN, OPEN_DOWNGRADE, READ,   |

Hi Benny,

Can you please remind us why NFS4ERR_BADLAYOUT does not suffice to cover this case?

Cheers
  Trond

---

From: Benny Halevy <[email-masked]>
Date: Wed, 30 Oct 2013 15:18:39 +0200

> Can you please remind us why NFS4ERR_BADLAYOUT does not suffice to cover this case?

NFS4ERR_BADLAYOUT in this case is supposed to be returned
"If the layout identified in the arguments does not exist"

and: 15.1.10.2.  NFS4ERR_BADLAYOUT (Error Code 10050)

   The layout specified is invalid in some way.
[it's valid in the case at hand]

   For LAYOUTCOMMIT, this
   indicates that the specified layout is not held by the client

[why special case LAYOUTCOMMIT? we could use this error for any other case when
the layout stateid is used after being returned]

or is
   not of mode LAYOUTIOMODE4_RW.
[not the discussed case]

In the case I'm talking about the layout does exist, but the layout
stateid provided is stale.
And that said, I'd also fix the text in 15.1.10.2 to reflect

The proposed error code makes it easier for the server to implement
layout stateid
validation and is more descriptive to the client.

Benny

---

From: "Myklebust, Trond" <[email-masked]>
Date: Wed, 30 Oct 2013 14:56:05 +0000

> the layout stateid is used after being returned]

It is also used for LAYOUTGET. I suspect that the text above is stale too... :-)

>   not of mode LAYOUTIOMODE4_RW.

That is already covered by NFS4ERR_BAD_IOMODE.

> And that said, I'd also fix the text in 15.1.10.2 to reflect

I'm not sure that I understand. Do you mean that the stateid is OK, but the seqid is stale? If so, then what you really need is NFS4ERR_OLD_STATEID.

> validation and is more descriptive to the client.

I don't think the client really cares. Both NFS4ERR_BADLAYOUT and NFS4ERR_BAD_STATEID need to be recovered through the same mechanisms.

AFAICS, it makes sense to have one or the other error, but not both. We've already made a choice in the spec and we have existing implementations.

Cheers
  Trond

---

From: Benny Halevy <[email-masked]>
Date: Wed, 30 Oct 2013 18:00:32 +0200

> That is already covered by NFS4ERR_BAD_IOMODE.

Hmm, NFS4ERR_BAD_IOMODE (in section 18.42.3) is a typo.
It should be NFS4ERR_BADIOMODE.
[Tom, is that documented anywhere?]

And strictly speaking LAYOUTCOMMIT does not provide a layout iomode,
only a layout stateid and the error code seem to indicate that the
iomode provided by the operation is inappropriate.

We can collapse the cases by augmenting NFS4ERR_BADIOMODE.

> I'm not sure that I understand. Do you mean that the stateid is OK, but the seqid is stale? If so, then what you really need is NFS4ERR_OLD_STATEID.

The opaque part does no longer identify an existing layout state.
Therefore I prefer BAD_STATEID as per:

15.1.5.2. NFS4ERR_BAD_STATEID (Error Code 10026)

   A stateid does not properly designate any valid state.  See Sections
   8.2.4 and 8.2.3 for a discussion of how stateids are validated.

vs. OLD_STATEID:

15.1.5.5. NFS4ERR_OLD_STATEID (Error Code 10024)

   A stateid with a non-zero seqid value does match the current seqid
   for the state designated by the user.

Since the latter designate a valid opaque part with an invalid seqid.

> I don't think the client really cares. Both NFS4ERR_BADLAYOUT and NFS4ERR_BAD_STATEID need to be recovered through the same mechanisms.

And so is recovery from ADMIN_REVOKED and EXPIRED so we can have just one error code and get away with it ;)
The point is to reflect the nature of the error and BADLAYOUT should say that something's is wrong with the layout (and we say nothing about the loca_layoutupdate by the way), not with the layout stateid.

> AFAICS, it makes sense to have one or the other error, but not both. We've already made a choice in the spec and we have existing implementations.

It's true that existing implementations may not tolerate NFS4ERR_BAD_STATEID for LAYOUTCOMMIT.

Benny

---

From: "Myklebust, Trond" <[email-masked]>
Date: Wed, 30 Oct 2013 16:35:27 +0000

> The opaque part does no longer identify an existing layout state.

That maps directly into NFS4ERR_BADLAYOUT as I read the spec: "the specified layout is not held by the client"...

<snip>

> Since the latter designate a valid opaque part with an invalid seqid.

Exactly. That probably _should_ be a valid return for LAYOUTCOMMIT (it already is for the other LAYOUT* operations).

> And so is recovery from ADMIN_REVOKED and EXPIRED so we can have just one error code and get away with it ;)

There has been an (IMO unnecessary) duplication of error codes here.

That said, NFS4ERR_EXPIRED is somewhat different: it should be interpreted to mean the same as the SEQ4_STATUS_EXPIRED_ALL_STATE_REVOKED sequence flag. i.e. it applies to more than just the stateid that was passed to this operation.

> It's true that existing implementations may not tolerate NFS4ERR_BAD_STATEID for LAYOUTCOMMIT.

Right. If we'd gone with BAD_STATEID from the start, then I'd have been fine with that. It's the change of on-wire semantics that bothers me.

Trond