Subject: Re: [nfsv4] [pnfs] 19 questions

From: Mike Eisler <[email-masked]>
Date: Wed, 6 Feb 2008 01:40:09 -0800 (PST)

--- Benny Halevy <[email-masked]> wrote:

> IETF listeners, please see spec related issues below.
> 
> Benny
> 
> On Feb. 05, 2008, 2:11 +0200, Dean Hildebrand <[email-masked]>
> wrote:
> > Some questions about draft 19 (not 19 questions):
> > 
> > 1) On the server I need the superblock so I can call the
> getdeviceinfo 
> > export operation.  The problem is that getdeviceinfo no longer
> includes 
> > the current_fh.  Is there a way on the server to get the superblock
> 
> > without the filehandle? (maybe using something in the session?)  If
> not, 
> > then we will have to encode something in the device id that allows
> us to 
> > map to the superblock (is the fsid enough?)  Any ideas?
> 
> Yes, this is the price of having a global device ID namespace.
> To ensure uniqueness between different file systems we should
> encode the deviceid4 type which is 16 bytes long as 
> 
> struct pnfs_deviceid {
> 	u64	ex_fsid;
> 	__be64	devid;
> };
> 
> Although the exported file system should only encode its private part
> we can let it fill the whole structure as it will also need to do
> that
> when encoding the deviceid in layoutget so I suggest defining static
> inline helper functions for xdr encoding and decoding the deviceid.

Is there an issue with the specification here?

> 
> > 
> > 2) Can getdeviceinfo return a device for the wrong layout type?  It
> 
> > seems that getdeviceinfo returns the layout type, which seems a
> little 
> > silly to me.  If it should return the layout type, and it doesn't
> match 
> > the requesting layout type, what should the client do?
> 
> Yeah, the returned layout type is part of device_addr4 which is self
> sufficient
> with that respect.  getdeviceinfo could have just returned the opaque
> part of the
> device_addr4 structure.  Cc'ing the nfsv4 ietf mailing list...

layouttype is also returned in LAYOUTGET and probably other 
operations. It has been this way for about a year now.

> 
> > 
> > 3) In draft-19, should bitmap4 not have <>?
> > bitmap4<> typedef uint32_t bitmap4<>;
> >       ^^^^
> 
> I don't this it matters too much, However it does look like a bug
> in the generating script...  Fixed by the following patch:

I chose to not tempt fate with -r and left '>' symbols in.
They are in other autogenerated .xml files.

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

---

From: Benny Halevy <[email-masked]>
Date: Wed, 06 Feb 2008 15:16:56 +0200

> Is there an issue with the specification here?

Not that I'm aware of....

> operations. It has been this way for about a year now.

Maybe we just need to state the obvious that when successful, the metadata
server MUST return the same layouttype as was requested, respectively and that
the client SHOULD discard the result if the result's layouttype is not equal
to the request's.

> They are in other autogenerated .xml files.

So at the minimum, the following is required for getting rid of the angle brackets
in the data type name:

diff -Npu a/dotx.d/spit_types.sh b/dotx.d/spit_types.sh
--- a/dotx.d/spit_types.sh
+++ b/dotx.d/spit_types.sh
@@ -54,7 +54,7 @@ EOF
 		sed 's/qdfuixref/<xref/g' | awk -F: '
 
 			{
-				i = index($3, "<");
+				i = index($3, "&lt;");
 				if (i == 0) {
 					i = index($3, "[");
 				};
8< =======================================================

Since the closing angle brackets in the xml source are potential
*ml "land mines" I think it's quite important to clean them up.
How about the following, more portable, solution:

diff -Npu a/Makefile b/Makefile
--- a/Makefile
+++ b/Makefile
@@ -14,7 +14,7 @@ autogen/%.xml : %.x
 	@mkdir -p autogen
 	@rm -f $@.tmp $@
 	@( cd dotx.d ; m4 `basename $<` > ../$@.tmp )
-	@cat $@.tmp | sed 's/^\%//' | sed 's/</\&lt;/g'| \
+	@cat $@.tmp | sed 's/^\%//' | sed 's/</\&lt;/g'| sed 's/>/\&gt;/g'| \
 	awk ' \
 		BEGIN	{ print "<figure>"; print" <artwork>"; } \
 			{ print $0 ; } \
diff -Npu a/dotx-id.d/Makefile b/dotx-id.d/Makefile
--- a/dotx-id.d/Makefile
+++ b/dotx-id.d/Makefile
@@ -84,7 +84,7 @@ AUTOGEN =	\
 dotx_autogen.xml: Makefile ../dotx.d/nfsv41.x
 	@rm -f $@.tmp $@
 	@cat ../dotx.d/nfsv41.x | sed 's?^?   ///?' | sed 's/</\&lt;/g'| \
-	awk ' \
+	sed 's/>/\&gt;/g'| awk ' \
 		BEGIN	{ print "<figure>"; print" <artwork>"; } \
 			{ print $0 ; } \
 		END	{ print " </artwork>"; print"</figure>" ; } ' \
diff -Npu a/dotx.d/spit_types.sh b/dotx.d/spit_types.sh
--- a/dotx.d/spit_types.sh
+++ b/dotx.d/spit_types.sh
@@ -16,31 +16,31 @@ typedef :int:int32_t
 typedef :unsigned int:uint32_t
 typedef :hyper:int64_t
 typedef :unsigned hyper:uint64_t
-typedef :opaque:attrlist4<>:Used for file/directory attributes
-typedef :uint32_t:bitmap4<>:Used in attribute array encoding.
+typedef :opaque:attrlist4&lt;&gt;:Used for file/directory attributes
+typedef :uint32_t:bitmap4&lt;&gt;:Used in attribute array encoding.
 typedef :uint64_t:changeid4:Used in definition of change_info
 typedef :uint64_t:clientid4:Shorthand reference to client identification
 typedef :uint32_t:count4:Various count parameters (READ, WRITE, COMMIT)
 typedef :uint64_t:length4:Describes LOCK lengths
 typedef :uint32_t:mode4:Mode attribute data type
 typedef :uint64_t:nfs_cookie4:Opaque cookie value for READDIR
-typedef :opaque:nfs_fh4<NFS4_FHSIZE>:Filehandle definition
+typedef :opaque:nfs_fh4&lt;NFS4_FHSIZE&gt;:Filehandle definition
 :enum:nfs_ftype4:Various defined file types
 :enum:nfsstat4:Return value for operations
 typedef :uint64_t:offset4:Various offset designations (READ, WRITE, LOCK, COMMIT)
 typedef :uint32_t:qop4:Quality of protection designation in SECINFO
-typedef :opaque:sec_oid4<>:Security Object Identifier The sec_oid4 data type is not really opaque. Instead it contains an ASN.1 OBJECT IDENTIFIER as used by GSS-API in the mech_type argument to GSS_Init_sec_context. See <xref target="RFC2743" /> for details.
+typedef :opaque:sec_oid4&lt;&gt;:Security Object Identifier The sec_oid4 data type is not really opaque. Instead it contains an ASN.1 OBJECT IDENTIFIER as used by GSS-API in the mech_type argument to GSS_Init_sec_context. See <xref target="RFC2743" /> for details.
 typedef :uint32_t:sequenceid4:sequence number used for various session operations (EXCHANGE_ID, CREATE_SESSION, SEQUENCE, CB_SEQUENCE).
 typedef :uint32_t:seqid4:Sequence identifier used for file locking
 typedef :opaque:sessionid4[NFS4_SESSIONID_SIZE]:Session identifier
 typedef :uint32_t:slotid4:sequencing artifact for various session operations (SEQUENCE, CB_SEQUENCE).
-typedef :opaque:utf8string<>:UTF-8 encoding for strings
+typedef :opaque:utf8string&lt;&gt;:UTF-8 encoding for strings
 typedef :utf8string:utf8str_cis:Case-insensitive UTF-8 string
 typedef :utf8string:utf8str_cs:Case-sensitive UTF-8 string
 typedef :utf8string:utf8str_mixed:UTF-8 strings with a case sensitive prefix and a case insensitive suffix.
 typedef :utf8str_cs:component4:Represents path name components
 typedef :utf8str_cs:linktext4:Symbolic link contents
-typedef :component4:pathname4<>:Represents path name for fs_locations
+typedef :component4:pathname4&lt;&gt;:Represents path name for fs_locations
 typedef :opaque:verifier4[NFS4_VERIFIER_SIZE]:Verifier used for various operations (COMMIT, CREATE, EXCHANGE_ID, OPEN, READDIR, WRITE) NFS4_VERIFIER_SIZE is defined as 8.
 EOF
 
@@ -50,9 +50,7 @@ EOF
 	then
 		mkdir -p autogen
 
-		cat $tmp | sed 's/<xref/qdfuixref/g' | sed 's/</\&lt;/g' |
-		sed 's/qdfuixref/<xref/g' | awk -F: '
-
+		cat $tmp | awk -F: '
 			{
 				i = index($3, "&lt;");
 				if (i == 0) {
@@ -73,7 +71,7 @@ EOF
 
 		' > $i
 	else
-		cat $tmp | awk -F: '
+		cat $tmp | sed 's/&lt;/</g' | sed 's/&gt;/>/g' | awk -F: '
 			NR >= 5 {
 				if ($1 != "") {
 					printf "%s%s\t%s;\n", $1, $2, $3;
diff -Npu a/nfsv41_middle_fileattributes.xml b/nfsv41_middle_fileattributes.xml
--- a/nfsv41_middle_fileattributes.xml
+++ b/nfsv41_middle_fileattributes.xml
@@ -340,7 +340,7 @@
       <ttcol align='left' >Acc</ttcol>
       <ttcol align='left' >Defined in:</ttcol>
 
-      <c>acl</c><c>12</c><c>nfsace4&lt;></c><c>R/W</c>
+      <c>acl</c><c>12</c><c>nfsace4&lt;&gt;</c><c>R/W</c>
       <c>
 	<xref target="attrdef_acl" />
       </c>
@@ -420,7 +420,7 @@
 	<xref target="attrdef_fs_charset_cap" />
       </c>
 
-      <c>fs_layout_type</c><c>62</c><c>layouttype4&lt;></c><c>RD</c>
+      <c>fs_layout_type</c><c>62</c><c>layouttype4&lt;&gt;</c><c>RD</c>
       <c>
 	<xref target="attrdef_fs_layout_type" />
       </c>
@@ -465,7 +465,7 @@
 	<xref target="attrdef_layout_hint" />
       </c>
 
-      <c>layout_type</c><c>64</c><c>layouttype4&lt;></c><c>RD</c>
+      <c>layout_type</c><c>64</c><c>layouttype4&lt;&gt;</c><c>RD</c>
       <c>
 	<xref target="attrdef_layout_type" />
       </c>
@@ -500,7 +500,7 @@
 	<xref target="attrdef_mdsthreshold" />
       </c>
 
-      <c>mimetype</c><c>32</c><c>utf8&lt;></c><c>R/W</c>
+      <c>mimetype</c><c>32</c><c>utf8&lt;&gt;</c><c>R/W</c>
       <c>
 	<xref target="attrdef_mimetype" />
       </c>
@@ -531,13 +531,13 @@
       </c>
 
       <c>owner</c>
-      <c>36</c><c>utf8&lt;></c><c>R/W</c>
+      <c>36</c><c>utf8&lt;&gt;</c><c>R/W</c>
       <c>
 	<xref target="attrdef_owner" />
       </c>
 
       <c>owner_group</c>
-      <c>37</c><c>utf8&lt;></c><c>R/W</c>
+      <c>37</c><c>utf8&lt;&gt;</c><c>R/W</c>
       <c>
 	<xref target="attrdef_owner_group" />
       </c>
8< =======================================================

Also, there's a lingering include of type_devlist_item4.x.
This patch fixes that:

diff -u a/dotx.d/head.x b/dotx.d/head.x
--- a/dotx.d/head.x
+++ b/dotx.d/head.x
@@ -355,7 +355,6 @@
 include(type_deviceid4.x)
 include(type_device_addr4.x)
 
-include(type_devlist_item4.x)
 include(type_layoutupdate4.x)
 include(type_layoutreturn4.x)
 include(type_fs4_status.x)
_______________________________________________
nfsv4 mailing list
[email-masked]
http://www.ietf.org/mailman/listinfo/nfsv4

---

From: "Noveck, Dave" <[email-masked]>
Date: Wed, 6 Feb 2008 16:46:11 -0500

> > > the requesting layout type, what should the client do?

I think the spec does need to make clear the the server MUST NOT do this
and what the client should do if it happens.
 
> > device_addr4 structure.  Cc'ing the nfsv4 ietf mailing list...

It is sufficient given that he knows the layout type.  OTOH, it you are
tracing things on the wire and displaying them it is nice to have the
layouttype which defines how things are to be printed out within the
message itself, with no need
to reference the request that this is a response to.

> operations. It has been this way for about a year now.

We've been trying to stay away from changes that are essentially based
on "I would have done it differently/better" even if there is consensus
that the change would be better, unless there is something substantially
wrong with what we have or there is an improvement that is so compelling
we want to take the hit of making a change.

---

From: Benny Halevy <[email-masked]>
Date: Thu, 07 Feb 2008 09:26:11 +0200

> and what the client should do if it happens.

Agreed.

> to reference the request that this is a response to.

That's a good point.

> we want to take the hit of making a change.

Agreed. Clarifying what the server MUST do with this respect and what the
client SHOULD do if the server lost its mind will suffice.

Benny