Subject: [nfsv4] [PATCH] allow cloneѕ inside a single file

From: Christoph Hellwig <[email-masked]>
Date: Fri, 6 Nov 2015 18:58:58 +0100

Hi Tom,

I hope this isn't too late for NFSv4.2, but it's a trivial patch found
while testing an actual implementation:

---
From: Christoph Hellwig <[email-masked]>
Subject: [PATCH] allow cloneѕ inside a single file

This is a feature Linux file systems support just fine, so disallowing
it by principle would be wrong.  Found while testing the new Linux clone
implementation with the test cases we use for local file system clone
support.

diff --git a/nfsv42_middle_op_clone.xml b/nfsv42_middle_op_clone.xml
index 73d45b6..ffb8205 100644
--- a/nfsv42_middle_op_clone.xml
+++ b/nfsv42_middle_op_clone.xml
@@ -42,12 +42,6 @@
     </t>
 
     <t>
-      SAVED_FH and CURRENT_FH must be different files.  If SAVED_FH and
-      CURRENT_FH refer to the same file, the operation MUST fail with
-      NFS4ERR_INVAL.
-    </t>
-
-    <t>
       The ca_dst_stateid MUST refer to a stateid that is valid for
       a WRITE operation and follows the rules for stateids in
       Sections 8.2.5 and 18.32.3 of <xref target='RFC5661' />.

---

From: Benjamin Kaduk <[email-masked]>
Date: Sat, 7 Nov 2015 01:38:33 -0500 (EST)

> This is a feature Linux file systems support just fine, so disallowing

At the risk of sounding like a broken record, "all the world's not Linux".
It seems unreasonable to make such a proposal without doing some
investigation into whether other OSes do have such a restriction.

-Ben

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

---

From: Christoph Hellwig <[email-masked]>
Date: Sat, 7 Nov 2015 09:29:17 +0100

Hi Benjamin,

do you know another OS supporting clone-like sematics?  I searched
extensively when adding it to the spec and couldn't find one.

Note that just because the spec doesn't explicily say intra-file clones
must be rejected it doesn't mean every implementation has to support
them - we have all kinds of weird limitations of protocol features due
to lacking backend implementations.

---

From: David Noveck <[email-masked]>
Date: Sat, 7 Nov 2015 08:36:17 -0500

> we have all kinds of weird limitations of protocol features due

If you have enough such weird limitations, a feature becomes basically
unusable, since the client has no assurance that the feature will, on any
given request, do what it is supposed to do.  If the spec did not, as you
propose, say anything about intra-file clones, a client implementer would
be justified in assuming they work the same as inter-file clones.

---

From: Tom Haynes <[email-masked]>
Date: Sat, 7 Nov 2015 22:28:14 -0500

> On Nov 7, 2015, at 1:38 AM, Benjamin Kaduk <[email-masked]> wrote:
> 

BTW - nothing prevents an implementation from returning NFS4ERR_INVALID in such circumstances.

If we feel strongly about including this wording, but want to allow for flexibility, we could go with:

If SAVED_FH and  CURRENT_FH refer to the same file, the operation may fail with NFS4ERR_INVAL.

---

From: Benny Halevy <[email-masked]>
Date: Sun, 8 Nov 2015 07:13:23 +0200

On Sun, Nov 8, 2015 at 5:28 AM, Tom Haynes <[email-masked]>
wrote:

>
>
> On Nov 7, 2015, at 1:38 AM, Benjamin Kaduk <[email-masked] <[email-masked]>>
> wrote:
>

Since the XDR is valid and its only the combination of parameters that's
not supported NFS4ERR_INVAL
doesn't seem appropriate for an optional server behavior. Overloading
NFS4ERR_SAME (officially ;))
would be more informational in my opinion.

Benny

---

From: Christoph Hellwig <[email-masked]>
Date: Mon, 9 Nov 2015 09:07:27 +0100

> If we feel strongly about including this wording, but want to allow for flexibility, we could go with:

I would prefer not to bother with the wording, but if there is a consensus
for it I can resend the patch with a wording like yours or Bennys.  (And it
probably should say MAY instead of may)

---

From: David Noveck <[email-masked]>
Date: Mon, 9 Nov 2015 12:31:05 -0500

There are a number of issues to resolve here:

   - Is it too late?

Since Tom seems to have indicated that he is OK with producing a -40, then
it isn't too late.  I don't think the consensus model applies here.

   - Is there some change needed, given that it is not too late?

My opinion is that there needs to be a change.  Not only is there no reason
for the REQUIREMENT that source and destination be different, even if there
were, NFS4ERR_INVAL would not be the correct error in this case.

   - Is there any reason to do anything else but delete the text in
   question.

I don't see any reason to try to allow servers to not handle this case.  I
agree with Ben that we should not consider the fact that Linux allows this
as dispositive, but for us to go in this direction, we would need some
indication that some implementation might find this troublesome.  I can't
see that this would be difficult to support.  It should be up to the person
who has the issue with same-file use of clone to object but so far all
objections have been speculative.  The fact that some future implementation
might have an issue with this should not be enough for us to include
special text

BTW, I do not agree that someone could validly return NFS4ERR_INVAL simply
because he has an issue/problem with supporting some particular case of
this operation.  So, if we delete this text we are saying that, if you
don't support same-file use of CLONE, you don't really support CLONE.  That
being said, we have to recognize that the IETF has no enforcement
mechanism.  The only real sanction is if people stop using your server.

---

From: Mike Kupfer <[email-masked]>
Date: Mon, 09 Nov 2015 11:05:49 -0800

David Noveck wrote:

> see that this would be difficult to support.

I confess I haven't been paying much attention to 4.2 recently. :-(

The current spec talks about CLONE as "without actually copying the
data".  But that's not a requirement, is it?  The server could actually
implement CLONE by copying?

If a server supports intra-file CLONE, must it allow overlapping source
and target areas?  If yes, I think that should be called out in the
spec.  If no, what error code should it return?

By the way, there are two typos at the top of page 92 of the -39 I-D.

  "multiple o" -> "multiple of"
  "wlll" -> "will"

mike

---

From: David Noveck <[email-masked]>
Date: Mon, 9 Nov 2015 14:51:42 -0500

> data".  But that's not a requirement, is it?

I don't see how it could be.

> The server could actually implement CLONE by copying?

In theory, yes.  However, for a large copy, it would be very difficult to
do and
still be both synchronous and atomic.

> and target areas?

With the proposed change, that of deleting the prohibition, the spec would
be very
unclear about the effect, which is one reason that dealing with this is more
difficult than it seems.

> If yes, I think that should be called out in the
> spec.

and also it should say what happens.

> If no, what error code should it return?

How about NFS4ERR_OVERLAPPING?

BTW, COPY has the same prohibition.  I think I now understand why this was
put in CLONE.  It was simply copied from COPY.  I suspect that the original
reason for the prohibition was to simply avoid dealing with the overlap
case.  It
is almost certainly is not the case that servers would have any particular
difficulty
dealing with in intra-file case of copy.  I think the same logic applies to
CLONE.

It appears that whatever we do on this should maintain the parallelism
between CLONE and
intra-server COPY.  In any case, the fix for this is going to be more
complicated than
anticipated.

---

From: Tom Haynes <[email-masked]>
Date: Mon, 9 Nov 2015 22:43:21 -0500

The document is past LC and off to the IESG - but issues from the WG still need to be considered.

> On Nov 9, 2015, at 12:31 PM, David Noveck <[email-masked]> wrote:
> 
> Is it too late? 
> Since Tom seems to have indicated that he is OK with producing a -40, then it isn't too late.  I don't think the consensus model applies here.
> Is there some change needed, given that it is not too late?
> My opinion is that there needs to be a change.  Not only is there no reason for the REQUIREMENT that source and destination be different, even if there were, NFS4ERR_INVAL would not be the correct error in this case.  
> Is there any reason to do anything else but delete the text in question.
> I don't see any reason to try to allow servers to not handle this case.  I agree with Ben that we should not consider the fact that Linux allows this as dispositive, but for us to go in this direction, we would need some indication that some implementation might find this troublesome.  I can't see that this would be difficult to support.  It should be up to the person who has the issue with same-file use of clone to object but so far all objections have been speculative.  The fact that some future implementation might have an issue with this should not be enough for us to include special text 

So you are for just deleting the text?

> 
> BTW, I do not agree that someone could validly return NFS4ERR_INVAL simply because he has an issue/problem with supporting some particular case of this operation.  So, if we delete this text we are saying that, if you don't support same-file use of CLONE, you don't really support CLONE.  That being said, we have to recognize that the IETF has no enforcement mechanism.  The only real sanction is if people stop using your server.
>

---

From: David Noveck <[email-masked]>
Date: Mon, 9 Nov 2015 23:52:38 -0500

I was until Mike pointed out the overlap issue.  Given that, I'm not sure
what to do.  It is probably best to just leave -39 as it is until someone
makes a solid proposal that is better. Given the overlap issue and the
parallelism between Copy and Clone, it may be difficult to resolve this
quickly.
On Nov 9, 2015 10:43 PM, "Tom Haynes" <[email-masked]> wrote:

> My opinion is that there needs to be a change.  Not only is there no
> reason for the REQUIREMENT that source and destination be different, even
> if there were, NFS4ERR_INVAL would not be the correct error in this case.
>
>

---

From: Benny Halevy <[email-masked]>
Date: Tue, 10 Nov 2015 07:52:04 +0200

I wonder if it won't be best to leave it to the server implementation to
figure out.

Benny
On Nov 10, 2015 06:52, "David Noveck" <[email-masked]> wrote:

>> Since Tom seems to have indicated that he is OK with producing a -40,
>> then it isn't too late.  I don't think the consensus model applies here.
>>
>>
>> I don't see any reason to try to allow servers to not handle this case.
>> I agree with Ben that we should not consider the fact that Linux allows
>> this as dispositive, but for us to go in this direction, we would need some
>> BTW, I do not agree that someone could validly return NFS4ERR_INVAL
>> simply because he has an issue/problem with supporting some particular case
>> of this operation.  So, if we delete this text we are saying that, if you

---

From: Christoph Hellwig <[email-masked]>
Date: Tue, 10 Nov 2015 08:34:37 +0100

> parallelism between Copy and Clone, it may be difficult to resolve this

The parallelism between Copy and Clone is mostly because Clone started
as copy stripped down to the bones + atomicy requirements.

Overlapping clones aren't supported by the existing implementations, which
is a strong argument to reject them.  Proposed text below:

diff --git a/nfsv42_middle_op_clone.xml b/nfsv42_middle_op_clone.xml
index 73d45b6..12ac3e0 100644
--- a/nfsv42_middle_op_clone.xml
+++ b/nfsv42_middle_op_clone.xml
@@ -42,12 +42,6 @@
     </t>
 
     <t>
-      SAVED_FH and CURRENT_FH must be different files.  If SAVED_FH and
-      CURRENT_FH refer to the same file, the operation MUST fail with
-      NFS4ERR_INVAL.
-    </t>
-
-    <t>
       The ca_dst_stateid MUST refer to a stateid that is valid for
       a WRITE operation and follows the rules for stateids in
       Sections 8.2.5 and 18.32.3 of <xref target='RFC5661' />.
@@ -78,6 +72,11 @@
       NFS4ERR_INVAL. The destination offset or destination offset plus
       count may be greater than the size of the destination file.
     </t>
+ 
+    <t>
+      If SAVED_FH and CURRENT_FH refer to the same file and the source and
+      target ranges overlap, the operation MUST fail with NFS4ERR_INVAL.
+    </t>
 
     <t>
       If the target area of the clone operation ends beyond the end of the

---

From: David Noveck <[email-masked]>
Date: Tue, 10 Nov 2015 10:18:57 -0500

> is a strong argument to reject them.

That's a good reason to allow their rejection.  It isn't a reason to
require their
rejection.  I think the reason to require their rejection, and it is a good
one, is
simply because it is very difficult to specify this case in way that
clients can
rely on and that clients don't need the functionality.  I think that is the
source of the same-file prohibition in COPY but it seems to be that they
went overboard.

> Proposed text below:

This is a good solution to the problem.

The analogous change makes sense for COPY.  One could delete the third
paragraph
of the DESCRIPTION and add a paragraph exactly the same as the one you
added to
CLONE right after the (current) seventh paragraph of the DESCRIPTION.

Given that we have to produce a -40, there are a number of other issues
that it makes
sense to address.  Some of these are based on things Mike found.  I found a
bunch of
other things looking through the CLONE description and related sections.

   - I suggest clarifying the first paragraph of 15.13.3 by replacing it by:

The CLONE operation is used to transfer file content from a source file

specified by SAVED_FH into a destination file specified by CURRENT_FH.

This operation is performed synchronously and atomically (see below).  This

transfer is typically performed without actually copying the data.  For
example,

a copy-on-write mechanism may be used.

   - I suggest clarifying/correcting the antepenultimate paragraph of
   15.13.3 by replacing it by:

If the size of the area to be cloned is not a multiple of the clone block
size and

the end of the destination file is beyond the end of the target area, the
region between

the end of the target area and the next multiple of the clone block size
will be zeroed.

   - There is some additional work needed in Section 4.

   - Part of the problem is that CLONE is not mentioned at all.
      - The treatment of what is required doesn't match section 13.
      - It speaks of 'a feature' while there were actually two separate
      features and

now are three.

I'll send a detailed proposal for changes to section 4 in the next few
days.

---

From: Christoph Hellwig <[email-masked]>
Date: Wed, 11 Nov 2015 18:47:36 +0100

>   "multiple o" -> "multiple of"

this already seems to be fixed in Tom's repository.

>   "wlll" -> "will"

patch below:

diff --git a/nfsv42_middle_op_clone.xml b/nfsv42_middle_op_clone.xml
index 12ac3e0..ffc83bd 100644
--- a/nfsv42_middle_op_clone.xml
+++ b/nfsv42_middle_op_clone.xml
@@ -90,7 +90,7 @@
       If the area to be cloned is not a multiple of the clone block size and the
       size of the destination file is past the end of the target area, the area
       between the end of the target area and the next multiple of the clone block
-      size wlll be zeroed.
+      size will be zeroed.
     </t>
 
     <t>

---

From: Christoph Hellwig <[email-masked]>
Date: Wed, 11 Nov 2015 18:49:22 +0100

> CLONE right after the (current) seventh paragraph of the DESCRIPTION.

I'd prefer it someone who actually knows and implements COPY would
perform those changes.

> a copy-on-write mechanism may be used.

fine with me.

> the end of the target area and the next multiple of the clone block size

sounds fine as well.

---

From: Tom Haynes <[email-masked]>
Date: Wed, 11 Nov 2015 10:42:47 -0800

> On Nov 11, 2015, at 9:47 AM, Christoph Hellwig <[email-masked]> wrote:
> 

I’ve taken both this patch and the last one you supplied:
index 73d45b6..12ac3e0 100644