Subject: [nfsv4] [PATCH] Let the client inform the server as to what arms it supports in READ_PLUS

From: Thomas Haynes <[email-masked]>
Date: Mon, 21 Apr 2014 21:44:26 -0700

From: Tom Haynes <[email-masked]>

Signed-off-by: Tom Haynes <[email-masked]>
---
 Makefile                       |  4 +++-
 dotx.d/Makefile                |  3 ++-
 dotx.d/read_plus_args.x        |  7 ++++---
 dotx.d/spit_types.sh           | 16 ++++++++++++---
 nfsv42_middle_op_read_plus.xml | 45 ++++++++++++++++++++++++++----------------
 5 files changed, 50 insertions(+), 25 deletions(-)

diff --git a/Makefile b/Makefile
index 1c04c33..2374c95 100755
--- a/Makefile
+++ b/Makefile
@@ -1,4 +1,4 @@
-# Copyright (C) The IETF Trust (2011-2013)
+# Copyright (C) The IETF Trust (2011-2014)
 #
 # Manage the .xml for the NFSv4 minorversion 2 document.
 #
@@ -153,6 +153,7 @@ SPITGEN =	dotx.d/type_nfstime4.x \
 		dotx.d/const_aclflag4.x \
 		dotx.d/const_access_deny.x \
 		dotx.d/const_sizes.x \
+		dotx.d/const_data_types_supported4.x \
 		dotx.d/type_nfs_cb_opnum4.x \
 		dotx.d/type_nfs_cb_argop4.x \
 		dotx.d/type_CB_COMPOUND4args.x \
@@ -231,6 +232,7 @@ SPITGENXML =	autogen/type_nfstime4.xml \
 		autogen/const_aclflag4.xml \
 		autogen/const_access_deny.xml \
 		autogen/const_sizes.xml \
+		autogen/const_data_types_supported4.xml \
 		autogen/type_nfs_cb_opnum4.xml \
 		autogen/type_nfs_cb_argop4.xml \
 		autogen/type_CB_COMPOUND4args.xml \
diff --git a/dotx.d/Makefile b/dotx.d/Makefile
index dd1a2b5..4a8778a 100755
--- a/dotx.d/Makefile
+++ b/dotx.d/Makefile
@@ -1,4 +1,4 @@
-# Copyright (C) The IETF Trust (2011)
+# Copyright (C) The IETF Trust (2011-2014)
 
 FORE_OPS_AS_IS1 = \
 		access_args.x \
@@ -217,6 +217,7 @@ SPITGEN =	basic_types.x \
 		const_aclflag4.x \
 		const_access_deny.x \
 		const_sizes.x \
+		const_data_types_supported4.x \
 		type_nfs_cb_opnum4.x \
 		type_nfs_cb_argop4.x \
 		type_CB_COMPOUND4args.x \
diff --git a/dotx.d/read_plus_args.x b/dotx.d/read_plus_args.x
index b64f44b..f707e94 100644
--- a/dotx.d/read_plus_args.x
+++ b/dotx.d/read_plus_args.x
@@ -1,6 +1,7 @@
 struct READ_PLUS4args {
 	/* CURRENT_FH: file */
-	stateid4        rpa_stateid;
-	offset4         rpa_offset;
-	count4          rpa_count;
+	stateid4              rpa_stateid;
+	offset4               rpa_offset;
+	count4                rpa_count;
+	data_types_supported4 rpa_supported;
 };
diff --git a/dotx.d/spit_types.sh b/dotx.d/spit_types.sh
index 8e3b227..4506f73 100755
--- a/dotx.d/spit_types.sh
+++ b/dotx.d/spit_types.sh
@@ -47,6 +47,7 @@ typedef :opaque:verifier4[NFS4_VERIFIER_SIZE]:Verifier used for various operatio
 typedef :string:secret4<>:Secret value to share between servers.
 typedef :uint32_t:policy4:Label format specifier or policy identifier.
 typedef :uint64_t:change_sec_label4:Used to detect label changes.
+typedef :uint64_t:data_types_supported4:Used to declare what data types are supported to the server.
 EOF
 
 	fi
@@ -1418,9 +1419,9 @@ cat << EOF > $i
  * Use an enum such that we can extend new types.
  */
 enum data_content4 {
-	NFS4_CONTENT_DATA = 0,
-	NFS4_CONTENT_APP_DATA_HOLE = 1,
-	NFS4_CONTENT_HOLE = 2
+	NFS4_CONTENT_DATA          = 0x0,
+	NFS4_CONTENT_APP_DATA_HOLE = 0x1,
+	NFS4_CONTENT_HOLE          = 0x2
 };
 EOF
 
@@ -1469,6 +1470,15 @@ EOF
 
 	;;
 
+	const_data_types_supported4.x )
+
+cat << EOF > $i
+const DATA4_DATA		= 0x00000001;
+const DATA4_APP_DATA_HOLE	= 0x00000002;
+const DATA4_HOLE		= 0x00000004;
+EOF
+	;;
+
 	* )
 		echo $0: Error: $i not recognized target.
 
diff --git a/nfsv42_middle_op_read_plus.xml b/nfsv42_middle_op_read_plus.xml
index e04a219..abe7629 100644
--- a/nfsv42_middle_op_read_plus.xml
+++ b/nfsv42_middle_op_read_plus.xml
@@ -3,6 +3,7 @@
 
 <section anchor='op:read_plus' title='Operation 65: READ_PLUS'>
   <section toc='exclude' title="ARGUMENT">
+    <?rfc include='autogen/const_data_types_supported4.xml'?>
     <?rfc include='autogen/read_plus_args.xml'?>
   </section>
 
@@ -15,29 +16,39 @@
   <section toc='exclude' anchor='op:read_plus:desc'  title="DESCRIPTION">
     <t>
       The READ_PLUS operation is based upon the NFSv4.1 READ operation
-      (see Section 18.22 of <xref target="RFC5661" />) and similarly reads data from the
-      regular file identified by the current filehandle.
+      (see Section 18.22 of <xref target="RFC5661" />) and similarly
+      reads data from the regular file identified by the current
+      filehandle.
     </t>
 
     <t>
-      The client provides a rpa_offset of where the READ_PLUS is to start and a
-      rpa_count of how many bytes are to be read.  A rpa_offset of zero means to
-      read data starting at the beginning of the file.  If rpa_offset is
-      greater than or equal to the size of the file, the status NFS4_OK is
-      returned with di_length (the data length) set to zero and eof
-      set to TRUE.
+      The client provides a rpa_offset of where the READ_PLUS is
+      to start and a rpa_count of how many bytes are to be read.
+      A rpa_offset of zero means to read data starting at the
+      beginning of the file.  If rpa_offset is greater than or equal
+      to the size of the file, the status NFS4_OK is returned with
+      di_length (the data length) set to zero and eof set to TRUE.
     </t>
 
     <t>
-      The READ_PLUS result is comprised of an array of rpr_contents, each of
-      which describe a data_content4 type of data (<xref target="ss:adh:dc" />).
-      For NFSv4.2, the allowed
-      values are data, ADH, and hole. A server is required to support the
-      data type, but neither ADH nor hole. Both an ADH and a hole must be
-      returned in its entirety - clients must be prepared to get more
-      information than they requested. Both the start and the end of the
-      hole may exceed what was requested. The array contents MUST be
-      contiguous in the file.
+      Finally, the client utilizes rpa_supported to inform the
+      server as to what arms of the rpr_contents of the READ_PLUS
+      result that the client supports. If the client indicates
+      that it does not support a data_content4 that the server
+      wishes to return, then the server MUST return that portion
+      of the array as a NFS4_CONTENT_DATA, i.e., data.
+    </t>
+
+    <t>
+      The READ_PLUS result is comprised of an array of rpr_contents,
+      each of which describe a data_content4 type of data
+      (<xref target="ss:adh:dc" />).  For NFSv4.2, the allowed values
+      are data, ADH, and hole. A server is required to support the data
+      type, but neither ADH nor hole. Both an ADH and a hole must
+      be returned in its entirety - clients must be prepared to get
+      more information than they requested. Both the start and the
+      end of the hole may exceed what was requested. The array
+      contents MUST be contiguous in the file.
     </t>
 
     <t>
-- 
1.9.2

---

From: Thomas Haynes <[email-masked]>
Date: Tue, 22 Apr 2014 08:29:19 -0700

On Apr 21, 2014, at 11:34 PM, Christoph Hellwig <[email-masked]> wrote:

> Passing this in for every READ_PLUS seems wasteful.  A filesystem-wide
> attribute sounds more suitable to me.
> 

How?

It is the client that is informing the server as to what it will accept. The filesystem
lives on the server.

Options:

1) Encode the fact that the client supports HOLES in EXCHANGE_ID.

Con: Does not expand.

2) Create a new operation: DATA_TYPE_MASK that the client can use to 
register which types it will accept.

If not sent, then the use of READ_PLUS by the client creates a default of
all types?

---

From: Christoph Hellwig <[email-masked]>
Date: Tue, 22 Apr 2014 18:10:06 +0200

> > attribute sounds more suitable to me.

My idea would be a fs-wide rw/attribute of type bitmap4, where the client
would set a flag for each new return type supported.  By defauly only
the data arm is supported.

---

From: Tom Haynes <[email-masked]>
Date: Tue, 22 Apr 2014 13:36:11 -0700

> the data arm is supported.

And the server has to store this per client, much like my second proposal.

If the client is only going to support the data arm, then it should just use READ
(an argument for not deprecating it).

I.e., have the default be that the client accepts all arms.

Also, I’m not convinced the property is per-filesystem. The client should
have no knowledge of the filesystem being employed by the server. It
is therefore a property of the client itself.

---

From: David Noveck <[email-masked]>
Date: Wed, 23 Apr 2014 06:49:10 -0400

> I.e., have the default be that the client accepts all arms

We have three bits so there are eight possible choices for a default mask.

First of all, anything that doesn't accept DATA is not sensible, and, as
you
point out, only accepting DATA can be dealt with by using READ (deprecated
by the working group or not, but not by me).  That leaves three possible
choices:

   - DATA + ADB (without accepting holes): does not make sense
   - DATA + HOLE (translating any ADB's to DATA): likely to be the most
   common (if not universal) choice.
   - DATA + ADB + HOLE: Likely to have very few implementations.  In fact,
   I haven't heard of anyone with plans to use this in a client.  If anyone
   does have plans for this, I'd like to hear about them.

My opinion is that if we keep ADB's:

   - the default mask for READ_PLUS should be DATA+HOLE.
   - If there are potential clients that would accept DATA+ADB+HOLE, then
   there are only two sensible choices for the effective mask to control
   READ_PLUS types returned, which can be distinguished by a flag to be added
   to EXCHANGE_ID.
   - If there are no such clients, we can still allow something ADB-like
   for WRITE_PLUS, in which case ADB's  turn into something like "Application
   Data Patterns" with no implication that they are part of the structure of
   the file data

On Tue, Apr 22, 2014 at 4:36 PM, Tom Haynes
<[email-masked]>wrote:

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

---

From: Chuck Lever <[email-masked]>
Date: Wed, 23 Apr 2014 09:23:49 -0400

On Apr 23, 2014, at 6:49 AM, David Noveck <[email-masked]> wrote:

> First of all, anything that doesn't accept DATA is not sensible, and, as you 
> point out, only accepting DATA can be dealt with by using READ (deprecated by the working group or not, but not by me).  That leaves three possible choices:
> 	• DATA + ADB (without accepting holes): does not make sense
> 	• DATA + HOLE (translating any ADB's to DATA): likely to be the most common (if not universal) choice.
> 	• DATA + ADB + HOLE: Likely to have very few implementations.  In fact, I haven't heard of anyone with plans to use this in a client.  If anyone does have plans for this, I'd like to hear about them.
> 	• the default mask for READ_PLUS should be DATA+HOLE.
> 	• If there are potential clients that would accept DATA+ADB+HOLE, then there are only two sensible choices for the effective mask to control READ_PLUS types returned, which can be distinguished by a flag to be added to EXCHANGE_ID.
> 	• If there are no such clients, we can still allow something ADB-like for WRITE_PLUS, in which case ADB's  turn into something like "Application Data Patterns" with no implication that they are part of the structure of the file data

I understand the desire to accelerate WRITEs by reducing the amount of
data the client has to send to the server.  I also understand the
desire to preserve holes when copying files.

I am not aware of a requirement (use case) for an NFSv4.2 server to
return a pattern when the client reads part of a file written with an
ADB.

> On Tue, Apr 22, 2014 at 4:36 PM, Tom Haynes <[email-masked]> wrote:
> 

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

---

From: Christoph Hellwig <[email-masked]>
Date: Wed, 23 Apr 2014 16:48:53 +0200

> I.e., have the default be that the client accepts all arms.

That seems sensible to me, especially if we only have data+hole.  Now I
can't really think of anything else that should show up in a regular file,
so just not having a way to negotiate the supported set seems fine if we
manage to kill off ADHs.  If for some reason we get another optional type
we should go back and choose either your exchange id or my attribute
version once needed to opt into the new arm.

> is therefore a property of the client itself.

Right, that's defintively an argument for the exchange id variant.

---

From: Tom Haynes <[email-masked]>
Date: Wed, 23 Apr 2014 10:36:34 -0700

> version once needed to opt into the new arm.

Ack, I think I finally get your argument.

If we were to put this in the READ_PLUS args, we have to do it now.

But if we have a separate process, we can do it at any point in the future.

And I think we are at least agreed it would be a separate process...

So, to summarize, as we have removed ADHs -> ADBs,

READ will no longer be deprecated.

If the client uses READ, it is stating it does not support HOLES.

If the client uses READ_PLUS, it is stating it does support HOLES.

Further, if it uses READ_PLUS, gets a hole back and realizes it
cannot handle it, then it can reissue the I/O with a READ. Don’t know
why a client would do that, but it has the option.