Subject: Re: [nfsv4] various minor NFSv4.2 review comments

From: Tom Haynes <[email-masked]>
Date: Thu, 1 May 2014 09:22:44 -0700

On Apr 30, 2014, at 11:16 AM, Christoph Hellwig <[email-masked]> wrote:

> On Wed, Apr 30, 2014 at 10:51:54AM -0700, Thomas Haynes wrote:
>> I think we drop it. You made a recent change that dropped this
>> sentence from draft-22:
>> 
>>   If the server elects to
>>   return the hole as data, then it can set the d_allocted to FALSE in
>>   the rpc_data to indicate it is a hole.
>> 
>> And the correct question is why would the server ever elect
>> to return the hole as data?
> 
> Because the NFS server might not get that information from the actual
> filesystem.  Of course in that case it couldn't set d_allocated either..

You had also asked:

>  Should we convert this to a
> 
> 	space_info4	d_reserved
> 
> and document it, or simply drop it?

If we convert, then we save a SEEK call?

I don’t care either way. Thoughts?

---

From: Christoph Hellwig <[email-masked]>
Date: Thu, 1 May 2014 18:25:25 +0200

> If we convert, then we save a SEEK call?

What is the use case for reading data and querying the reservation
status at the same time?  Seems like two unrelated operations to me,
and with NFSv4 clients that worry about the roundtrip time could
always combine the two in a single COMPOUND if they really care.

My vote goes for removing it.

---

From: Tom Haynes <[email-masked]>
Date: Thu, 1 May 2014 10:00:42 -0700

On Apr 30, 2014, at 10:51 AM, Thomas Haynes <[email-masked]> wrote:

> 
> On Apr 30, 2014, at 4:14 AM, Christoph Hellwig <[email-masked]> wrote:
> 
> 
>> 
>> === SEEK nitpicks ==
>> 
>> 15.12.3.:
>> 
>> "From the given sa_offset, find the next data_content4 of type sa_what in the
>> file. For a hole, this must return the data_content4 in its entirety."
>> 
> 
> must -> MAY
> 
>> What does an entire hole mean here, especially if we have allocated
>> and non-allocated holes next to each other?  Why does the return of
>> the entire range no apply to data?
>> 
> 
> The intent is that if the server detects a hole, it returns the end of
> the hole as well as the start.
> 
> If the server detects data, then finding the end would require I/O to
> find the end!
> 
> Well, no, actually it would not. Either we can detect when a hole
> will be found next or we can detect EOF.
> 
> So, I'd be willing to change that. I'll hold off on suggesting to what
> for a bit.
> 
> The other question of interest is allocated next to non-allocated. This
> wasn't an issue before we allowed byte-range space reservation. :-)
> 
> The obvious choices are:
> 
> 1) SEEK stops when the space_info4 changes
> 2) SEEK reports the entire data_content4 but returns
> SPACE_UNKNOWN4 when it consolidates different allocations.
> 

For consideration:

KinSlayer:NFSv4.2 loghyr$ git diff !$
git diff nfsv42_middle_op_seek.xml
diff --git a/nfsv42_middle_op_seek.xml b/nfsv42_middle_op_seek.xml
index 3f08c17..ba8abe5 100644
--- a/nfsv42_middle_op_seek.xml
+++ b/nfsv42_middle_op_seek.xml
@@ -22,8 +22,22 @@
 
     <t>
       From the given sa_offset, find the next data_content4 of type
-      sa_what in the file. For a hole, this must return
-      the data_content4 in its entirety.
+      sa_what in the file. The di_length in the result SHOULD be
+      the length of the sa_what, but as a hole may be a result of
+      compression of a block of zeros, di_offset + di_length might
+      not indicate the start of the next type of data_content4. If
+      the server does not know the final length of the sa_what,
+      then it can either return a di_length of zero or provide as
+      much information as it is aware. I.e., it might be difficult
+      to detect the end of the data content.
+    </t>
+
+    <t>
+      There is also no expectation that the server pay attention
+      to the byte range space reservations as it calculates the
+      data_content4 type.  If it presents a range that spans
+      both reserved and unreserved space, then it MUST set
+      di_reserved to be SPACE_UNKNOWN4.
     </t>
 
     <t>

---

From: Christoph Hellwig <[email-masked]>
Date: Thu, 1 May 2014 20:11:47 +0200

> +      to detect the end of the data content.

I don't think di_offset + di_length should be used as a shorthand
for the next SEEK call with opposite sa_what ever.  Besides the
different di_reserved value that already make this a bad idea on
the NFS-spec level filesystems might have different kinds of data
(or even holes) that they don't really want to merge.  So I think
a layout like

+------+------+------+------+------+
| HOLE | DATA | DATA | HOLE | DATA |
+------+------+------+------+------+

should very much be expected by the client.

I like the di_length = 0 shortcut, though - this allows implementing
a valid SEEK on top of SEEK_HOLE/SEEK_DATA trivially.

Guessing the next sa_what from the length does not seem very useful
same applies to lseek SEEK_DATA/HOLE.

> +      di_reserved to be SPACE_UNKNOWN4.

I think we should just say:

"A server MAY merge multiple consecuitve range as long as they
 have the same reservation status."

---

From: Tom Haynes <[email-masked]>
Date: Thu, 1 May 2014 13:44:40 -0500

> On May 1, 2014, at 1:11 PM, Christoph Hellwig <[email-masked]> wrote:
> 

So you are advocating that if the reservation changes, even if the content type is the same, we stop calculating the length?

---

From: Christoph Hellwig <[email-masked]>
Date: Fri, 2 May 2014 14:35:26 +0200

> So you are advocating that if the reservation changes, even if the content type is the same, we stop calculating the length?

We still calculate the length if we can, but we report two contents
isntead of one, so that we get an exact reservation status returned.

That assumes the server can figure out the reservation status, if not
it's all SPACE_UNKNOWN4 anyway.

---

From: Tom Haynes <[email-masked]>
Date: Fri, 2 May 2014 11:01:13 -0400

How about:

index 3f08c17..4dece07 100644
@@ -22,8 +22,21 @@
 
     <t>
       From the given sa_offset, find the next data_content4 of type
-      sa_what in the file. For a hole, this must return
-      the data_content4 in its entirety.
+      sa_what in the file. The di_length in the result SHOULD be
+      the length of the sa_what, but even for a hole, as the hole
+      may be a result of compression of a block of zeros, di_length
+      might not be easy to determine. If the server does not know
+      the final length of the sa_what, then it can either return a
+      di_length of zero or provide as much information as it is
+      aware.
+    </t>
+
+    <t>
+      There is also no expectation that the server pay attention
+      to the byte range space reservations as it calculates the
+      data_content4 type.  If it presents a range that spans
+      both reserved and unreserved space, then it MUST set
+      di_reserved to be SPACE_UNKNOWN4.
     </t>
 
     <t>

---

From: Christoph Hellwig <[email-masked]>
Date: Fri, 2 May 2014 17:50:57 +0200

> +      di_length of zero or provide as much information as it is

I don't understand the fixation about mentioning compression.  What
happens on the physical layer is fairly irrelevant for a network file system
protocol.  I also don't understand why we want to allow to return both
shorter and zero di_length.  Isn't one chicken route enough?

> +      di_reserved to be SPACE_UNKNOWN4.

Isn't that contradicting?  First it says the server doesn't nees to
care about space reservations, but the second sentence sais it actually
does have to take reservations into account.

Also you were asking earlier that we should be able to report the range
of a reservation, to do this we need the di_length to be correct (or 0 if it
can't be supported), and we need to respect the reservation status.

I think the idea of either the server not supporting the feature at all
(di_length = 0 and/or di_reserved - SPACE_UNKNOWN4) or reporting the
exact data is what's most sensible instead of leaving various grey areas,
but I'd love to hear argument where giving the server more slack would
help server implementers - it surely can't help the client..

---

From: Tom Haynes <[email-masked]>
Date: Fri, 2 May 2014 12:35:10 -0400

On May 2, 2014, at 11:50 AM, Christoph Hellwig <[email-masked]> wrote:

> protocol.

Part of READ_PLUS is the ability of the server to detect a block of 0s
and return it as a hole. So the compression is not part of the filesystem,
but in the NFS layer. In such a scenario, we might not know how
many blocks have 0s in them. And scanning forward is costly.

Hmm, I see that concept has been stripped out of the drafts with
the recent changes. In draft-22, we had:

5.3.1.  READ_PLUS

   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 the result would appear to be a
   hole.  I.e., if the result was a data block composed entirely of
   zeros, then it is easier to return a hole.  Returning data blocks of
   uninitialized data wastes computational and network resources, thus
   reducing performance. 

>  I also don't understand why we want to allow to return both

I wasn’t thinking of it as a chicken route!

I can argue both ways, but if a zero di_length is used to indicate
“I don’t know!”, I’m okay with that.

> help server implementers - it surely can't help the client..

I think that attaching the reservation to the output of SEEK is a slippery slope.

I don’t think that we can say anything at all about the reservation of a range
if we cannot compute the end of the range.

I.e., in my view, the reservation is a hint at best and only applicable to the
block containing the starting offset.

If we want the ability to query how big a reservation is, we should provide
a new operation to get that.

Going back to di_length, I’d change it such that:

di_length MUST be 0 for data
dl_length MUST be the end of the hole or 0 if the end is not easy to calculate.

---

From: Christoph Hellwig <[email-masked]>
Date: Fri, 2 May 2014 18:50:18 +0200

> many blocks have 0s in them. And scanning forward is costly.

Agreed with all that, but it's an implementation detail that doesn't
really have a place in the NFS spec.  The intent is to transfer
file data that is all zero efficiently.  How the NFS server came to
the conclusion that it is all zero is a matter of the various software
layers in the server, and should be as transparent as possible to the
client.

I'm perfectly fine with allowing an opt-out from reporting the length,
jsut like I designed it into the reservation wording, I just don't like
to justify it with a low-level implementation detail that is entirely
irrelevant to the client.

> “I don’t know!”, I’m okay with that.

If the server can't get the length without a lot of effort this is the
shortcut we give it.  Maybe "chicken route" isn't quite right, I certainly
don't want those words to appear in the standard :)

> if we cannot compute the end of the range.

Yes, that was my thought as well.  Probably make sense to clarify that
setting di_length = 0 is incompatible with reporting the reservation status.

> a new operation to get that.

This brings us back to the old discussion if 'di_allocated' as it was known
there is a hint or not.

My proposal was that if it is not unknown the client can rely on it, and
if the server can't provide a defintive answer it should set di_reserved
should be set to SPACE_UNKNOWN4.

> dl_length MUST be the end of the hole or 0 if the end is not easy to calculate.

I fully agree with that!

---

From: "McDonald, Alex" <[email-masked]>
Date: Fri, 2 May 2014 17:16:33 +0000

[snip]

> shorter and zero di_length.  Isn't one chicken route enough?

Chicken route? Whassat?

---

From: Christoph Hellwig <[email-masked]>
Date: Mon, 12 May 2014 11:48:19 +0200

On Wed, Apr 30, 2014 at 01:14:41PM +0200, Christoph Hellwig wrote:
> === OFFLOAD_* description text ===
> 
> All these talk about copy operations, but also apply to WRITE_SAME?

I'd like to draw some more attention to OFFLOAD_ in the current draft.

Besides the trivial wording issues of talking about copies instead of
generic asynchronous operations there are a few more issues:

 - why is OFFLOAD_REVOKE not called COPY_REVOKE given that it doesn't
   have a meaning outside of an intra-server copy?

 - requiring the client to send OFFLOAD_ABORT to the _destination_ server
   seem to massively complicate the model in that it requires additional
   client interaction for the remote copy, shouldn't the servers handle
   that behind that clients back? 

 - the OFFLOAD_ABORT description talks about being required when an
   "application" exits before the copy is completed.  In general I'd expect
   COPY to be used by a copyfile-like system call, and not applications
   being directly involved.  What was the purpose behind this wording?

---

From: Tom Haynes <[email-masked]>
Date: Mon, 12 May 2014 07:05:18 -0700

> On May 12, 2014, at 2:48 AM, Christoph Hellwig <[email-masked]> wrote:
> 

Yes (or are a generic framework to handle future asynchronous operations)

>   have a meaning outside of an intra-server copy?

In case we ever added another asynchronous operation that needed to be revoked.

>   that behind that clients back? 

I believe the intent was that if the source was not responsive to the client, the client could inform the destination to stop the copy.

I.e., assume the destination is doing the copy over a back-end network that the client is not connected to.

---

From: Tom Haynes <[email-masked]>
Date: Mon, 12 May 2014 09:35:15 -0700

>   being directly involved.  What was the purpose behind this wording?

My guess is that it was part of the sell job of the original I-D that I did not
remove. The sentence can go and not affect the flow of the document.

My biggest issue with the COPY stuff is that we never did a separate
requirements document like we did for Labeled NFS. As such, there
is still too much of an effort to specify requirements, redundant text
about the basic functionality, etc.

I’ll make a pass to clean that stuff out.

---

From: Christoph Hellwig <[email-masked]>
Date: Mon, 12 May 2014 18:49:25 +0200

> Yes (or are a generic framework to handle future asynchronous operations)

I tried to reword it that way, but there are so many copy specifics in
the description that I gave up for now.

> In case we ever added another asynchronous operation that needed to be revoked.

I don't think it is anywhere near as generic as the other operations,
and the netloc4 argument to specify a remote server makes it very tied
to COPY.  In fact I would consider it a counterpart to COPY_NOTIFY, which
also is copy specific.

> I believe the intent was that if the source was not responsive to the client, the client could inform the destination to stop the copy.

Hmm.  The whole idea of a remote to remote copy that isn't fire and forget
to the client seem pretty dangerous to me, but I'd need to read more about
the original rationale for this design.

---

From: David Noveck <[email-masked]>
Date: Mon, 12 May 2014 22:13:38 +0300

> remove.

I took a look at draft-lentini-nfsv4-server-side-copy-00 and it didn't have
that
much of  sales flavor.

I think Christoph is right to assume that this will be done via a system
call.  We have
to remember that this document was written over five years ago.  My guess
is that,
it might have seemed presumptuous, at that time, to assume that a system
call would
be created to nicely package up this feature.  It might have been a
reasonable assumption,
at that time, that this would be initially used by special-purpose clients
integrated with
applications.

I agree that, given our (reasonable) assumptions now,  this text should be
fixed up.

On Mon, May 12, 2014 at 7:35 PM, Tom Haynes
<[email-masked]>wrote:

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

---

From: Tom Haynes <[email-masked]>
Date: Mon, 12 May 2014 14:25:43 -0700

I see what you mean about the use of “copy” in these sections. I’ll
fix that up once we agree on _REVOKE.

> to COPY.  In fact I would consider it a counterpart to COPY_NOTIFY, which

Difficult with only two examples of offloading, so yes, this is not applicable to
WRITE_SAME. I.e., it applies to an asynchronous operation that occurs between
two servers.

Famous last words, but all interesting server-to-server ops can be built on top
of COPY, so I can’t see why it has to be OFFLOAD_REVOKE.

I’m more stuck with the following:

      The client uses OFFLOAD_ABORT to inform the destination to
      stop the active transfer and OFFLOAD_REVOKE to inform the
      source to not allow any more copy requests from the destination.
^^ Implies it is for all operations from the destination, not just the
specific copy.

      The OFFLOAD_REVOKE operation is also useful in situations in
      which the source server granted a very long or infinite lease
      on the destination server's ability to read the source file
      and all copy operations on the source file have been completed.

And actually, to my above point, see the earlier text:

   This operation is used for an inter-server copy.  A client sends this
   operation in a COMPOUND request to the source server to revoke the
   authorization of a destination server identified by
   ora_destination_server from reading the file specified by CURRENT_FH
   on behalf of given user.

But the destination might have several active copies going on with the source
for that file. Note that OFFLOAD_ABORT does not have that issue, i.e., it sends
a stateid.

So we need to add a stateid here.

And actually, we don’t even need the netloc4 at that point. (And to be fair
in the original drafts, COPY_NOTIFY did not take the stateid.)

At that point, what is the difference between OFFLOAD_ABORT and OFFLOAD_REVOKE?
The server in question will know what role it has in the copy and can take
the appropriate action.

Why not just have the one operation?

---

From: Christoph Hellwig <[email-masked]>
Date: Tue, 13 May 2014 17:20:47 +0200

> Why not just have the one operation?

Using OFFLOAD_ABORT to both the source and destination server seems like
a major improvement to me, and is much easier to explain in terms of
a generic asynchronous operation framework.

If we do that should we also allow OFFLOAD_STATUS to the source and
destination server to get either ones view of the progress?

---

From: Tom Haynes <[email-masked]>
Date: Tue, 13 May 2014 08:58:28 -0700

> destination server to get either ones view of the progress?

Interesting, nothing prevents it in the text - there is no MUST support statement
in Section 4.2.2. (from my copy branch)

What if the source is not tracking the progress? What can it return to indicate
that to the client?

I’m okay with adding a new code to track this if we want to allow it.

---

From: Christoph Hellwig <[email-masked]>
Date: Tue, 13 May 2014 18:42:10 +0200

> I’m okay with adding a new code to track this if we want to allow it.

I don't have a strong opinion here.  If it makes life too hard for servers
to always support the status on the source server I'd rather add language
to only support it for the destination server.

---

From: Tom Haynes <[email-masked]>
Date: Tue, 13 May 2014 21:58:58 -0500

> to only support it for the destination server.

Actually, I’d like us to go with the model that this operation cannot fail if
the CURRENT_FH and stateid are correct. I.e., let the client throw this
into a random compound and it will not fail the rest of the compound.

We could add a osr_tracking to the result to indicate if the server was
tracking this operation or not. Then let the client decide what to do with it
if the server indicates it is not tracking it.