Subject: [nfsv4] [PATCH] NFS4ERR_UNION_NOTSUPP is not a valid error for READ_PLUS

From: Christoph Hellwig <[email-masked]>
Date: Tue, 19 Nov 2013 12:59:54 +0100

The client doesn't tell the server what data_content4 to read, but the
server determines it based on the offset.  Thus returning
NFS4ERR_UNION_NOTSUPP for READ_PLUS does not make sense.

diff --git a/nfsv42_middle_errors.xml b/nfsv42_middle_errors.xml
index e4766f2..f279f46 100644
--- a/nfsv42_middle_errors.xml
+++ b/nfsv42_middle_errors.xml
@@ -242,7 +242,7 @@
           One of the arguments to the operation is a discriminated union and
           while the server supports the given operation, it does not support
           the selected arm of the discriminated union. For an example, see
-          READ_PLUS (<xref target="op:read_plus" />).
+          WRITE_PLUS (<xref target="op:write_plus" />).
         </t>
       </section>
     </section>
@@ -2399,7 +2399,6 @@
         NFS4ERR_STALE,
         NFS4ERR_SYMLINK,
         NFS4ERR_TOO_MANY_OPS,
-        NFS4ERR_UNION_NOTSUPP,
         NFS4ERR_WRONG_TYPE
       </c>
 
diff --git a/nfsv42_middle_new_errors.xml b/nfsv42_middle_new_errors.xml
index 68a0bf4..7dca049 100644
--- a/nfsv42_middle_new_errors.xml
+++ b/nfsv42_middle_new_errors.xml
@@ -39,7 +39,7 @@
           One of the arguments to the operation is a discriminated union and
           while the server supports the given operation, it does not support
           the selected arm of the discriminated union. For an example, see
-          READ_PLUS (<xref target="op:read_plus" />).
+          WRITE_PLUS (<xref target="op:write_plus" />).
         </t>
       </section>
     </section>
@@ -269,7 +269,6 @@
         NFS4ERR_STALE,
         NFS4ERR_SYMLINK,
         NFS4ERR_TOO_MANY_OPS,
-        NFS4ERR_UNION_NOTSUPP,
         NFS4ERR_WRONG_TYPE
       </c>
 
diff --git a/nfsv42_middle_op_read_plus.xml b/nfsv42_middle_op_read_plus.xml
index aa4a165..e04a219 100644
--- a/nfsv42_middle_op_read_plus.xml
+++ b/nfsv42_middle_op_read_plus.xml
@@ -41,14 +41,6 @@
     </t>
 
     <t>
-      READ_PLUS has to support all of the errors which are returned by READ
-      plus NFS4ERR_UNION_NOTSUPP. If the client asks for a hole and the server
-      does not support that arm of the discriminated union, but does support
-      one or more additional arms, it can signal to the client that it supports
-      the operation, but not the arm with NFS4ERR_UNION_NOTSUPP.
-    </t>
-
-    <t>
       If the data to be returned is comprised entirely of zeros, then
       the server may elect to return that data as a hole. The server
       differentiates this to the client by setting di_allocated to TRUE

---

From: "Noveck, David" <[email-masked]>
Date: Tue, 19 Nov 2013 14:36:39 -0500

Agree with this. Note that in -20 the fourth paragraph of 14.10.3 explains (incorrectly) why READ_PLUS  has to support NFS4ERR_UNION_NOTSUPP and so should be deleted.

Note that the bigger problem with READ_PLUS as currently specified is that there is no way to deal with the fact that the server can return a data_content4 that it supports but that the client does not. Given that such features as ADH's are to be optional, clients need to be able to avoid supporting such content types.

How about:

    typedef bitmap4 data_cmask4;  // mask defining a set of content types. 

    struct READ_PLUS4args {
           /* CURRENT_FH: file */
          stateid4        rpa_stateid;
          data_cmask4 rpa_ctset.
          offset4         rpa_offset;
          count4          rpa_count;
   };

rpa_ctset defines a set of content types that the client is prepared to accept
and deal with.    Each of the elements in the rpr_contents field of the reply
must be a data_content4  that is either NFS4_CONTENT_DATA4 or represents 
a bit in rpa_ctset which is set.  If there is a data_content4 element in the file which does
not meet this constraint, it appears as data (for example, a NFS4_CONTENT_HOLE
appears as a string of zeros).  If rpa_ctset contains bits set for content types not
supported by the server, those bits are ignored and no error results.  Generally,
bit positions beyond the size of the opaque array are treated as not set.  However,
a zero-length rpa_ctset I s treated as allowing the return of any server-supotrted
content type.

-----Original Message-----
From: nfsv4 [mailto:[email-masked]] On Behalf Of Christoph Hellwig
Sent: Tuesday, November 19, 2013 7:00 AM
To: Tom Haynes
Cc: [email-masked]
Subject: [nfsv4] [PATCH] NFS4ERR_UNION_NOTSUPP is not a valid error for READ_PLUS

The client doesn't tell the server what data_content4 to read, but the server determines it based on the offset.  Thus returning NFS4ERR_UNION_NOTSUPP for READ_PLUS does not make sense.

diff --git a/nfsv42_middle_errors.xml b/nfsv42_middle_errors.xml index e4766f2..f279f46 100644
 
diff --git a/nfsv42_middle_new_errors.xml b/nfsv42_middle_new_errors.xml index 68a0bf4..7dca049 100644
 
diff --git a/nfsv42_middle_op_read_plus.xml b/nfsv42_middle_op_read_plus.xml index aa4a165..e04a219 100644
--- a/nfsv42_middle_op_read_plus.xml
+++ b/nfsv42_middle_op_read_plus.xml
@@ -41,14 +41,6 @@
     </t>
 
     <t>
-      READ_PLUS has to support all of the errors which are returned by READ
-      plus NFS4ERR_UNION_NOTSUPP. If the client asks for a hole and the server
-      does not support that arm of the discriminated union, but does support
-      one or more additional arms, it can signal to the client that it supports
-      the operation, but not the arm with NFS4ERR_UNION_NOTSUPP.
-    </t>
-
-    <t>
       If the data to be returned is comprised entirely of zeros, then
       the server may elect to return that data as a hole. The server
       differentiates this to the client by setting di_allocated to TRUE _______________________________________________
nfsv4 mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/nfsv4

---

From: Christoph Hellwig <[email-masked]>
Date: Tue, 19 Nov 2013 20:51:54 +0100

> Agree with this. Note that in -20 the fourth paragraph of 14.10.3 explains (incorrectly) why READ_PLUS  has to support NFS4ERR_UNION_NOTSUPP and so should be deleted.

That's in fact the part I stumbled upon.  It is removed in the patch that
I attached to my previous mail.

> Note that the bigger problem with READ_PLUS as currently specified is that there is no way to deal with the fact that the server can return a data_content4 that it supports but that the client does…

Agreed, I brought this issue up a while ago already.

>     typedef bitmap4 data_cmask4;  // mask defining a set of content types. 

I'd much prefer to get rid of the ADH type, as I proposed earlier.  Holes
are simple enough that we can expect every client that supports READ_PLUS
to perform the zeroing.

If we cannot agree on doing that I think your bitmap approach is a good
idea, but instead of bloating each READ_PLUS call with it I'd prefer
to make it a filesystem-wide attribute, which defaults to just supporting
NFS4_CONTENT_DATA, but allows the client to negotiate additional data
types at mount time using a SETATTR call.

---

From: "Noveck, David" <[email-masked]>
Date: Tue, 19 Nov 2013 15:51:22 -0500

> I'd much prefer to get rid of the ADH type, as I proposed earlier.

Personally I'd agree, but before we strike a feature from NFSv4.2, we need to  let the working group speak on the matter.  I haven't heard anyone say "Please don't get rid of ADH's in 4.2" or "Put them off to 4.3, at the earliest". I suspect that nobody has been interested enough to comment on the matter, one way  or the other.

Maybe we need to schedule some NFSv4.2 conference calls.

> I'd prefer to make it a filesystem-wide attribute, which defaults to just

> supporting NFS4_CONTENT_DATA, but allows the client to negotiate

> additional data types at mount time using a SETATTR call.

OK, if you write the text explaining this new type of attribute with per-fs/per-client scope and the working group is OK with it.

Another option is to do this at EXCHANGE_ID time, in which case we are looking at EXCHANGE_ID_PLUS and marking EXCHANGE_ID  obsolescent.  Note that this would mean that I would have to face the issue of what happens when migration causes lease merger and the merged leases have different sets of allowed content types.  Sigh!

Does the working group prefer?

*         rpa_ctset

*         A SETATTR approach

*         EXCHANGE_ID_PLUS

*         Getting rid of ADH's, at least for NFSv4.2.

*         Something else?  Please specify.

-----Original Message-----
From: Christoph Hellwig [mailto:[email-masked]]
Sent: Tuesday, November 19, 2013 2:52 PM
To: Noveck, David
Cc: Christoph Hellwig; Tom Haynes; [email-masked]
Subject: Re: [nfsv4] [PATCH] NFS4ERR_UNION_NOTSUPP is not a valid error for READ_PLUS

> Agree with this. Note that in -20 the fourth paragraph of 14.10.3 explains (incorrectly) why READ_PLUS  has to support NFS4ERR_UNION_NOTSUPP and so should be deleted.

That's in fact the part I stumbled upon.  It is removed in the patch that I attached to my previous mail.

>     typedef bitmap4 data_cmask4;  // mask defining a set of content types.

I'd much prefer to get rid of the ADH type, as I proposed earlier.  Holes are simple enough that we can expect every client that supports READ_PLUS to perform the zeroing.

If we cannot agree on doing that I think your bitmap approach is a good idea, but instead of bloating each READ_PLUS call with it I'd prefer to make it a filesystem-wide attribute, which defaults to just supporting NFS4_CONTENT_DATA, but allows the client to negotiate additional data types at mount time using a SETATTR call.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 19 Nov 2013 16:13:50 -0500

> Another option is to do this at EXCHANGE_ID time, in which case we are looking at EXCHANGE_ID_PLUS and marking EXCHANGE_ID  obsolescent.  Note that this would mean that I would have to face the iss…

Ugh.  But couldn't this be another bit in eia_flags?

If it's an attribute is there a reason not to make it global?  (Well,
per-client?)

> *         Getting rid of ADH's, at least for NFSv4.2.

I think the latter's the default until someone speaks up for it.  No
point trying to maintain protocol features that noone's still interested
in.  (And we're unlikely to do it right without someone that knows what
they want out of it anyway.)

--b.

> _______________________________________________
> nfsv4 mailing list
> [email-masked]

---

From: Marc Eshel <[email-masked]>
Date: Wed, 20 Nov 2013 09:00:03 -0800

I would prefer to remove ADH from NFSv4.2 but if we keep it than it should 
be a separate operation from READ/WRITE_PLUS and it can be an optional 
operation, I don't think we need to change EXCHANGE_ID.

I also think that we need more explanation on SEEK operation, just saying 
that it is like READ_PLUS is not enough since it is not exactly the same.

Marc. 

From:   "Noveck, David" <[email-masked]>
To:     Christoph Hellwig <[email-masked]>, 
Cc:     Tom Haynes <[email-masked]>, "[email-masked]" 
<[email-masked]>
Date:   11/19/2013 12:51 PM
Subject:        Re: [nfsv4] [PATCH] NFS4ERR_UNION_NOTSUPP is not a valid 
error   for     READ_PLUS
Sent by:        "nfsv4" <[email-masked]>

> I'd much prefer to get rid of the ADH type, as I proposed earlier. 
 
Personally I'd agree, but before we strike a feature from NFSv4.2, we need 
to  let the working group speak on the matter.  I haven't heard anyone say 
"Please don't get rid of ADH's in 4.2" or "Put them off to 4.3, at the 
earliest". I suspect that nobody has been interested enough to comment on 
the matter, one way  or the other.

> additional data types at mount time using a SETATTR call.
 
OK, if you write the text explaining this new type of attribute with 
per-fs/per-client scope and the working group is OK with it. 
 
Another option is to do this at EXCHANGE_ID time, in which case we are 
looking at EXCHANGE_ID_PLUS and marking EXCHANGE_ID  obsolescent.  Note 
that this would mean that I would have to face the issue of what happens 
when migration causes lease merger and the merged leases have different 
sets of allowed content types.  Sigh!
 
Does the working group prefer?
·         rpa_ctset
·         A SETATTR approach
·         EXCHANGE_ID_PLUS
·         Getting rid of ADH?s, at least for NFSv4.2.
·         Something else?  Please specify.
 
-----Original Message-----
From: Christoph Hellwig [mailto:[email-masked]] 
Sent: Tuesday, November 19, 2013 2:52 PM
To: Noveck, David
Cc: Christoph Hellwig; Tom Haynes; [email-masked]
Subject: Re: [nfsv4] [PATCH] NFS4ERR_UNION_NOTSUPP is not a valid error 
for READ_PLUS
 
On Tue, Nov 19, 2013 at 02:36:39PM -0500, Noveck, David wrote:
> Agree with this. Note that in -20 the fourth paragraph of 14.10.3 
explains (incorrectly) why READ_PLUS  has to support NFS4ERR_UNION_NOTSUPP 
and so should be deleted.

> Note that the bigger problem with READ_PLUS as currently specified is 
that there is no way to deal with the fact that the server can return a 
data_content4 that it supports but that the client does not. Given that 
such features as ADH's are to be optional, clients need to be able to 
avoid supporting such content types.

 
 
If we cannot agree on doing that I think your bitmap approach is a good 
idea, but instead of bloating each READ_PLUS call with it I'd prefer to 
make it a filesystem-wide attribute, which defaults to just supporting 
NFS4_CONTENT_DATA, but allows the client to negotiate additional data 
types at mount time using a SETATTR call.