Subject: [nfsv4] some cleanup for the WRITE_PLUS split

From: Christoph Hellwig <[email-masked]>
Date: Mon, 21 Apr 2014 18:00:48 +0200

Clean up a few bits after the WRITE_PLUS split: only mention WRITE_HOLE
in the sparse file related sections, and only WRITE_SAME in the ADH-related
section, don't pair the two in enumerations of operations and update
the actual command descriptions to reflect reality.  The two descriptions
still refer to the 4.1 WRITE description a lot, which might be another
target to rewrite into a standalone form as there's not much shared with
WRITE now.

diff --git a/nfsv42_middle_introduction.xml b/nfsv42_middle_introduction.xml
index 72ce829..5ee9208 100644
--- a/nfsv42_middle_introduction.xml
+++ b/nfsv42_middle_introduction.xml
@@ -123,8 +123,8 @@
     <section title="Application Data Hole (ADH) Support">
       <t>
         Some applications treat a file as if it were a disk and as such
-        want to initialize (or format) the file image. We extend both
-        READ_PLUS and introduce WRITE_SAME (see <xref target='op:write_same' />)
+        want to initialize (or format) the file image. We extend READ_PLUS
+	and introduce WRITE_SAME (see <xref target='op:write_same' />)
         to understand this metadata as a new form of a hole.
       </t>
     </section>
diff --git a/nfsv42_middle_op_cb_offload.xml b/nfsv42_middle_op_cb_offload.xml
index dcd3d57..843c130 100644
--- a/nfsv42_middle_op_cb_offload.xml
+++ b/nfsv42_middle_op_cb_offload.xml
@@ -30,7 +30,10 @@
           the COPY operation
         </t>
         <t>
-          either the WRITE_HOLE or WRITE_SAME operations
+          the WRITE_HOLE,
+        </t>
+        <t>
+          or the WRITE_SAME operations
         </t>
       </list>
 
@@ -54,14 +57,21 @@
       </t>
     </section>
 
-    <section toc='exclude' title='WRITE_HOLE and WRITE_SAME'>
+    <section toc='exclude' title='WRITE_HOLE'>
       <t>
-        CB_OFFLOAD is used to report the completion of either
-        a hole punch or an ADH initialization. Upon success,
-        the coa_resok4 will contain the same information that
-        the corresponding synchronous WRITE_HOLE or WRITE_SAME
-        would have returned.
+        CB_OFFLOAD is used to report the completion of a hole punch.
+	Upon success, the coa_resok4 will contain the same information
+	that the corresponding synchronous WRITE_HOLE would have returned.
       </t>
     </section>
+
+    <section toc='exclude' title='WRITE_SAME'>
+      <t>
+        CB_OFFLOAD is used to report the completion of a hole punch.
+	Upon success, the coa_resok4 will contain the same information
+	that the corresponding synchronous WRITE_SAME would have returned.
+      </t>
+    </section>
+
   </section>
 </section>
diff --git a/nfsv42_middle_op_write_hole.xml b/nfsv42_middle_op_write_hole.xml
index 5cf4704..0b49753 100644
--- a/nfsv42_middle_op_write_hole.xml
+++ b/nfsv42_middle_op_write_hole.xml
@@ -14,11 +14,38 @@
 
   <section toc='exclude' anchor='op:write_hole:desc'  title="DESCRIPTION">
     <t>
-      The WRITE_HOLE operation is an extension of the NFSv4.1 WRITE
-      operation (see Section 18.2 of <xref target="RFC5661" />) and
-      writes holes to the regular file identified by the
-      current filehandle.  The server MAY write fewer bytes than
-      requested by the client.
+      Whenever a client wishes to reinitialize the blocks backing a
+      particular region in the file, it calls the WRITE_HOLE operation with
+      the current filehandle set to the filehandle of the file in question,
+      and the equivalent of start offset and length in bytes of the region
+      set in wh_hole.di_offset and wh_hole.di_length respectively.  If the
+      wh_hole.di_allocated is set to TRUE, then the blocks will be zeroed and
+      if it is set to FALSE, then they will be deallocated. All
+      further reads to this region MUST return zeros until overwritten.
+      The filehandle specified must be that of a regular file.
+
+      The client specifies with the ws_stable parameter the method of how the
+      data is to be processed by the server.  It is treated like the stable
+      parameter in the NFSv4.1 WRITE operation (see Section 18.2 of
+      <xref target="RFC5661" />).
+    </t>
+
+    <t>
+      Situations may arise where di_offset and/or di_offset + di_length will
+      not be aligned to a boundary for which the server does allocations or
+      deallocations.  For most file systems, this is the block size of
+      the file system.  In such a case, the server can deallocate as many
+      bytes as it can in the region.  The blocks that cannot be deallocated
+      MUST be zeroed.  Except for the block deallocation and maximum hole
+      punching capability, a WRITE_HOLE operation is to be treated similar
+      to a write of zeroes.
+    </t>
+
+    <t>
+      The server is not required to complete deallocating the blocks
+      specified in the operation before returning.  The server SHOULD
+      return an asynchronous result if it can determine the operation will
+      be long running (see <xref target='op:write_hole:async' />).
     </t>
 
     <t>
@@ -34,55 +61,21 @@
       If the client supports WRITE_HOLE, it MUST support CB_OFFLOAD. 
     </t>
 
-    <section title="Hole punching">
-      <t>
-        Whenever a client wishes to reinitialize the blocks backing a
-        particular region in the file, it calls the WRITE_HOLE operation with
-        the current filehandle set to the filehandle of the file in question,
-        and the equivalent of start offset and length in bytes of the region
-        set in wh_hole.di_offset and wh_hole.di_length respectively.  If the
-        wh_hole.di_allocated is set to TRUE, then the blocks will be zeroed and
-        if it is set to FALSE, then they will be deallocated. All
-        further reads to this region MUST return zeros until overwritten.
-        The filehandle specified must be that of a regular file.
-      </t>
-
-      <t>
-        Situations may arise where di_offset and/or
-        di_offset + di_length will not be aligned to a
-        boundary for which the server does allocations/deallocations.  For
-        most file systems, this is the block size of
-        the file system.  In such a case, the server can deallocate as many
-        bytes as it can in the region.  The blocks that cannot be deallocated
-        MUST be zeroed.  Except for the block deallocation and maximum hole
-        punching capability, a WRITE_HOLE operation is to be treated similar
-        to a write of zeroes.
-      </t>
-
-      <t>
-        The server is not required to complete deallocating the blocks
-        specified in the operation before returning.  The server SHOULD
-        return an asynchronous result if it can determine the
-        operation will be long running
-        (see <xref target='op:write_hole:async' />).
-      </t>
-
-      <t>
-        If used to hole punch, WRITE_HOLE will result in the space_used
-        attribute being decreased by the number of bytes that were
-        deallocated.  The space_freed attribute
-        may or may not decrease, depending on the support and whether the
-        blocks backing the specified range were shared or not.  The size
-        attribute will remain unchanged.
-      </t>
+    <t>
+      If used to hole punch, WRITE_HOLE will result in the space_used
+      attribute being decreased by the number of bytes that were
+      deallocated.  The space_freed attribute
+      may or may not decrease, depending on the support and whether the
+      blocks backing the specified range were shared or not.  The size
+      attribute will remain unchanged.
+    </t>
 
-      <t>
-        The WRITE_HOLE operation MUST NOT change the space reservation
-        guarantee of the file.  While the server can deallocate the blocks
-        specified by di_offset and di_length, future writes to this region
-        MUST NOT fail with NFSERR_NOSPC.
-      </t>
-    </section>
+    <t>
+      The WRITE_HOLE operation MUST NOT change the space reservation
+      guarantee of the file.  While the server can deallocate the blocks
+      specified by di_offset and di_length, future writes to this region
+      MUST NOT fail with NFSERR_NOSPC.
+    </t>
 
     <section anchor='op:write_hole:async' title='Asynchronous Transactions'>
       <t>
diff --git a/nfsv42_middle_op_write_same.xml b/nfsv42_middle_op_write_same.xml
index 01c695f..59ed04b 100644
--- a/nfsv42_middle_op_write_same.xml
+++ b/nfsv42_middle_op_write_same.xml
@@ -14,22 +14,17 @@
 
   <section toc='exclude' anchor='op:write_same:desc'  title="DESCRIPTION">
     <t>
-      The WRITE_SAME operation is an extension of the NFSv4.1 WRITE
-      operation (see Section 18.2 of <xref target="RFC5661" />) and
-      writes data to the regular file identified by the
-      current filehandle.  The server MAY write fewer bytes than
-      requested by the client.
-    </t>
-
-    <t>
-      The WRITE_SAME argument 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.
-      The array contents MUST be contiguous in the file. A successful
-      WRITE_SAME will construct a reply for wr_count, wr_committed,
-      and wr_writeverf as per the NFSv4.1 WRITE operation results.
-      If wr_callback_id is set, it indicates an asynchronous reply
-      (see <xref target='op:write_same:async' />).
+      The WRITE_SAME operation writes an application data hole to the
+      regular file identified by the current filehandle.
+      The WRITE operation is used to an application data hole to a regular
+      file.  The target file is specified by the current filehandle.  The
+      data to be written is specified by an app_data_hole4 structure
+      (<xref target="ss:adh:dh"/>).
+
+      The client specifies with the ws_stable parameter the method of how the
+      data is to be processed by the server.  It is treated like the stable
+      parameter in the NFSv4.1 WRITE operation (see Section 18.2 of
+      <xref target="RFC5661" />).
     </t>
 
     <t>
@@ -51,7 +46,7 @@
       </t>
 
       <t>
-        For ADHs, when the client invokes the WRITE_SAME operation, it has
+        When the client invokes the WRITE_SAME operation, it has
         two desired results:
       </t>
 
diff --git a/nfsv42_middle_space.xml b/nfsv42_middle_space.xml
index 701b6ff..14c3702 100644
--- a/nfsv42_middle_space.xml
+++ b/nfsv42_middle_space.xml
@@ -95,11 +95,6 @@
           This attribute specifies the space freed when a file is
           deleted, taking block sharing into consideration.
         </t>
-
-        <t hangText="WRITE_HOLE and WRITE_SAME">
-          These operations zero and/or deallocate the blocks
-          backing a region of the file.
-        </t>
       </list>
     </t>
 
diff --git a/nfsv42_middle_sparse.xml b/nfsv42_middle_sparse.xml
index ab095d4..3d0f0f9 100644
--- a/nfsv42_middle_sparse.xml
+++ b/nfsv42_middle_sparse.xml
@@ -45,16 +45,12 @@
     </t>
 
     <t>
-      Three new operations WRITE_HOLE (<xref target="op:write_hole" />),
-      WRITE_SAME (<xref target="op:write_same" />), and READ_PLUS
-      (<xref target="op:read_plus" />) are introduced.  WRITE_HOLE
-      allows for the hole punching.  I.e., an application might want
-      to reinitialize a range of the file.  WRITE_SAME allows for
-      the creation of application
-      specific block structures in a file which is treated by the
-      application as if it were a disk.  READ_PLUS supports all the
+      Two new operations WRITE_HOLE (<xref target="op:write_hole" />),
+      and READ_PLUS (<xref target="op:read_plus" />) are introduced.
+      WRITE_HOLE allows for the hole punching.  I.e., an application might
+      want to reinitialize a range of the file. READ_PLUS supports all the
       features of READ but includes extensions to support both sparse
-      files and sparse pattern files (<xref target="ss:adh:dc" />).  READ_PLUS is
+      files.  READ_PLUS is
       guaranteed to perform no worse than READ, and can dramatically
       improve performance with sparse files.  READ_PLUS does not
       depend on pNFS protocol features, but can be used by pNFS to
@@ -90,24 +86,10 @@
   </section>
 
   <section anchor="ss:sparse:new_ops" title="New Operations">
-    <t>
-      READ_PLUS, WRITE_HOLE, and WRITE_SAME are new variants of the NFSv4.1
-      READ and WRITE operations <xref target="RFC5661" />.
-      Besides being able to support all of the data semantics of
-      those operations, they can also be used by the client
-      and server to efficiently transfer both holes and ADHs
-      (see <xref target='ss:adh:dh' />). As READ 
-      is inefficient for transfer of sparse sections of the file,
-      it is marked as OBSOLESCENT in NFSv4.2. Instead, a client
-      should utilize READ_PLUS. Note that as
-      the client has no a priori knowledge of whether either
-      an ADH or a hole is present or not, if it supports
-      these operations and so does the server, then it should
-      always use these operations.
-    </t>
-
     <section anchor="ss:sparse:read_plus" title="READ_PLUS">
       <t>
+       READ_PLUS is a new variant of the NFSv4.1 READ operation
+       <xref target="RFC5661" />.
        For holes, READ_PLUS extends the response to avoid returning
        data for portions of the file which are initialized and
        contain no backing store.  Additionally it will do so if
@@ -119,6 +101,15 @@
        the metadata describing the portions of the file which are
        initialized and contain no backing store.
       </t>
+    
+      <t>
+        As READ  is inefficient for transfer of sparse sections of the
+        file, it is marked as OBSOLESCENT in NFSv4.2. Instead, a client
+        should utilize READ_PLUS. Note that as the client has no a priori
+        knowledge of whether either an ADH or a hole is present or not,
+        if it supports these operations and so does the server, then it
+        should always use these operations.
+      </t>
 
       <t>
         If the client sends a READ operation, it is explicitly
@@ -133,17 +124,14 @@
       </t>
     </section>
 
-    <section anchor="ss:sparse:write_hole_same" title="WRITE_HOLE and WRITE_SAME">
+    <section anchor="ss:sparse:write_hole" title="WRITE_HOLE">
       <t>
-        WRITE_HOLE can be used to hole punch and WRITE_SAME
-        can be used to initialize ADHs. For either purpose,
-        the client can avoid the
-        transfer of a repetitive pattern across the network.
-        If the filesystem on the server does not support
-        sparse files, the WRITE_HOLE and WRITE_SAME operations may return
-        the result asynchronously via the CB_OFFLOAD operation.
-        As a hole punch may entail deallocating data blocks,
-        even if the filesystem supports sparse files, it
+        WRITE_HOLE can be used to hole punch, so that the client can avoid
+	the transfer of a repetitive pattern across the network. If the
+	filesystem on the server does not support sparse files, the
+	WRITE_HOLE operation may return the result asynchronously via the
+	CB_OFFLOAD operation.  As a hole punch may entail deallocating
+	data blocks, even if the filesystem supports sparse files, it
         may still have to return the result via CB_OFFLOAD.
       </t>
     </section>

---

From: Tom Haynes <[email-masked]>
Date: Mon, 21 Apr 2014 12:17:16 -0700

Thanks for the text.

I don’t really have a problem with it until you get to the changes starting
in nfsv42_middle_sparse.xml.

If we are going to say that ADHs are not holes, then we should do that.

Personally, I am torn between keeping them as holes versus pushing them
off as transferable as holes until usage has shown the need (i.e., NFSv4.3).
I.e., ADB in NFSv4.2 and ADH in NFSv4.3.

If they are holes, we know READ_PLUS will go faster. But the cost is complicating
the SEEK operation (I don’t say READ_PLUS because that can be handled via
a wrapper in the client to produce the blocks).

Even if we change ADH back to ADB, we should still solve the issue of the
client informing the server of which arms of the union it will support for READ_PLUS.
I’ll add the text for that.

Let me also craft up some text for the ADB approach in NFSv4.2...

> +++ b/nfsv42_middle_introduction.xml

Indentation is off.

> +++ b/nfsv42_middle_op_cb_offload.xml

Indentation is off

> +	that the corresponding synchronous WRITE_SAME would have returned.

Indentation is off

> +++ b/nfsv42_middle_op_write_hole.xml

Is your intent to have a new paragraph? You will need a </t> and <t>.

> +      be long running (see <xref target='op:write_hole:async' />).

The WRITE operation is used to do what to an application data hole 

> +      <xref target="RFC5661" />).

Why have you removed this bullet item?

At the very least you would want to leave WRITE_HOLE here?

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

---

From: Christoph Hellwig <[email-masked]>
Date: Mon, 21 Apr 2014 21:26:34 +0200

> If we are going to say that ADHs are not holes, then we should do that.

They aren't holes, they are magic in their current form and are in no
way related to "regular" sparse files.  That's not just my opinion, but
also how most of the current draft treats them. e.g. by dedicating
section 7 to them, while sparse files are covered in in section 5.

Having tiny bits of ADH knowledge leak into section 5 does seem helpful
to me in this greater context.

---

From: Christoph Hellwig <[email-masked]>
Date: Mon, 21 Apr 2014 21:31:38 +0200

Second mail to keep this discussion of this patch and ADHs in general
separate.

> a wrapper in the client to produce the blocks).

I think ADHs as is are utterly broken.  They try to shoe-horn some form
of structured files into a fundamentally flat model, and will confuse
all existing client implementations, and aren't easily implementable for
most servers.

I think a proper WRITE_SAME that doesn't impose additional structure is
useful for NFS, and would satisfy all use cases I could come up with.

Now if we can come up with a coherent model for structured files I would
not mind adding them as long as they are their own type of file instead
of pretending to be a regular file.

> client informing the server of which arms of the union it will support for READ_PLUS.

Agreed.