Subject: [nfsv4] Server-side copy question

From: "Mora, Jorge" <[email-masked]>
Date: Wed, 15 Feb 2017 22:19:13 +0000

Hello,

The NFSv4.2 spec (RFC 7862) on section 15.2.3 (COPY description) says the following:

If the source offset or the source offset plus count is greater than
the size of the source file, the operation MUST fail with
NFS4ERR_INVAL.

I can understand failing with NFS4ERR_INVAL when the source offset is beyond the end of the file,
but do you think failing with NFS4ERR_INVAL is too strict when the source offset plus the count is beyond the end of the file?
What is the rationalization for failing on this specific instance?
Why not return a short copy instead?

As of right now, the implementation on the Linux server adheres to the spec but copy_file_range succeeds
when it is called against the local file system, NFSv4.x or NFSv3.
For the local file system, NFSv4.x or NFSv3 copy_file_range falls back to regular copy by
reading from the source file and then writing to the destination file but I do believe the
syscall should be consistent regardless of the underlying file system.

--Jorge

---

From: David Noveck <[email-masked]>
Date: Thu, 16 Feb 2017 08:00:36 -0500

> but do you think failing with NFS4ERR_INVAL is too strict when the source
offset plus the count is beyond the end of the file?

Yes.

> What is the rationalization for failing on this specific instance?

I don't know of any.

> Why not return a short copy instead?

If the question is "why not actually implement a server to return a short
copy", you could probably do that and get way with it but it is generally
better to follow the spec, even if mistaken, unless something horrendous
would occur by following it.

If the question is "why doesn't the spec allow you to return a short
copy?", I don't know.  I think this is probably just a mistake that wasn't
caught in time.

If the question is "why can't the spec be fixed to allow return of a short
copy?", it can, in theory, but it is probably not worth the effort.  You
can file an erratta, but since this is asking for a change in behavior,
there will be a lot of resistance, especially since the word "MUST" is used.

BTW, the word "MUST" probably shouldn't be there since RFC 2119 says:

Imperatives of the type defined in this memo must be used with care
and sparingly. In particular, they MUST only be used where it is
actually required for interoperation or to limit behavior which has
potential for causing harm (e.g., limiting retransmisssions)

> syscall should be consistent regardless of the underlying file system.

I agree but think that the most expedient way of addressing this problem is
in the Linux client,  It can check against the file size before issuing the
COPY request  to prevent an erroneous INVAL error being returned.

On Wed, Feb 15, 2017 at 5:19 PM, Mora, Jorge <[email-masked]> wrote:

> but do you think failing with NFS4ERR_INVAL is too strict when the source
> offset plus the count is beyond the end of the file?
>
> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4
>
>

---

From: Thomas Haynes <[email-masked]>
Date: Thu, 16 Feb 2017 18:17:32 +0000

Yes.

I don't know of any.

> Why not return a short copy instead?

If the question is "why not actually implement a server to return a short copy", you could probably do that and get way with it but it is generally better to follow the spec, even if mistaken, unless something horrendous would occur by following it.

If the question is "why doesn't the spec allow you to return a short copy?", I don't know.  I think this is probably just a mistake that wasn't caught in time.

If the question is "why can't the spec be fixed to allow return of a short copy?", it can, in theory, but it is probably not worth the effort.  You can file an erratta, but since this is asking for a change in behavior, there will be a lot of resistance, especially since the word "MUST" is used.

No, I’d file an errata with the justification that implementations we are trying to be compatible with are not compatible with the existing language.

Imperatives of the type defined in this memo must be used with care and sparingly. In particular, they MUST only be used where it is actually required for interoperation or to limit behavior which has potential for causing harm (e.g., limiting retransmisssions)

> syscall should be consistent regardless of the underlying file system.

I agree but think that the most expedient way of addressing this problem is in the Linux client,  It can check against the file size before issuing the COPY request  to prevent an erroneous INVAL error being returned.

On Wed, Feb 15, 2017 at 5:19 PM, Mora, Jorge <[email-masked]<mailto:[email-masked]>> wrote:
Hello,

--Jorge

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

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

---

From: [email-masked] (J. Bruce Fields)
Date: Fri, 17 Feb 2017 14:51:49 -0500

> COPY request  to prevent an erroneous INVAL error being returned.

That doesn't work, due to races with concurrent modifications to the
file.

The server itself would seem succeptible to such races: if it implements
the copy as a read-write loop then blocking concurrent truncates
probably isn't reasonable, so its only choice is a short copy.  (Too
late to return INVAL once you've written to the destination....).

I really think the spec's just wrong here.

--b.

---

From: David Noveck <[email-masked]>
Date: Sat, 18 Feb 2017 07:45:50 -0500

> That doesn't work, due to races with concurrent modifications to the

I'm not sure whether it works or not.  I can't find an obvious breakage
but I can't prove that none exists.

In any case, we can't consider this an easy fix for the issue.

> The server itself would seem susceptible to such races: if it implements

That's an implementation problem that would exist no matter
what we do to the spec text under discussion.

> I really think the spec's just wrong here.'

I agree, as does Tom and Jorge as well I suppose.   Nobody has said it
was a good choice.

The question is what to do to fix it.  This is particularly difficult given
the misuse of "MUST".  The problem I'm worried about is that
the existing approach is presented as required for interoperability.
We know of cases where that approach has been followed and
the problem is that the existing spec essentially says that clients
might break if servers do something else. :-(

We know that isn't true but sombody (Spencer D?) is going to have
to convince people in the IESG of that.

So I think the focus of any correction has to be on the fact that the "MUST"
is wrong rather than on the fact (also true) that the wrong behavior was
chosen.

So here is my proposed corrected text:

When the source offset is greater than the size of the source file, the
error

is reported by failing the operation with NFS4ERR_INVAL.  Otherwise, If

the source offset plus count is greater than the size of the source file,
the

server MAY fail the operation with NFS4ERR_INVAL.or limit the area to

be copied to reflect the size of the source file.

---

From: Trond Myklebust <[email-masked]>
Date: Sat, 18 Feb 2017 13:29:47 -0500

> When the source offset is greater than the size of the source file, the error
> 
> the source offset plus count is greater than the size of the source file, the 
> 

Could we please make that message to implementors stronger, but saying “the server SHOULD limit the are to be copied to reflect the size of the source file, but it MAY fail the operation with NFS4ERR_INVAL”?

Cheers
  Trond

> On Feb 18, 2017, at 07:45, David Noveck <[email-masked]> wrote:
> 
> is wrong rather than on the fact (also true) that the wrong behavior was chosen.
> 
> When the source offset is greater than the size of the source file, the error
> 
> the source offset plus count is greater than the size of the source file, the 
> 
> On Fri, Feb 17, 2017 at 2:51 PM, J. Bruce Fields <[email-masked] <mailto:[email-masked]>> wrote:

---

From: David Noveck <[email-masked]>
Date: Sat, 18 Feb 2017 15:57:20 -0500

> Could we please make that message to implementors stronger, but saying “the server SHOULD limit the are to be copied to reflect the size of the source file, but it MAY fail the operation with NFS4E…

I think we can do something in this regard but your text is not in
accord with RFC2119, although I recall seeing things like this in
other documents.  SHOULD means a client is allowed to do something
else under some fairly restrictive conditions while MAY says he can
choose to do something simply because he wants to.  While it is
reasonable to say you "should do X by may do Y", saying you "SHOULD do
X but MAY do Y" is almost always self-contradictory.

How about leaving the proposed text and adding the following sentence?

The latter is generally preferable since the former might  force
clients implementing primitives for copying byte ranges to check the
size of the file before issuing the copy, which in turn raises issues
because such a check and the copy are done at different times and the
size might change between the two operations.

On Sat, Feb 18, 2017 at 1:29 PM, Trond Myklebust <[email-masked]> wrote:

> Could we please make that message to implementors stronger, but saying
> “the server SHOULD limit the are to be copied to reflect the size of the
> source file, but it MAY fail the operation with NFS4ERR_INVAL”?
>
> Cheers
>   Trond
>
>
>

---

From: Trond Myklebust <[email-masked]>
Date: Sat, 18 Feb 2017 23:23:45 -0500

On 18 February 2017 at 15:57, David Noveck <[email-masked]> wrote:

> I think we can do something in this regard but your text is not in accord with RFC2119, although I recall seeing things like this in other documents.  SHOULD means a client is allowed to do something else under some fairly restrictive conditions while MAY says he can choose to do something simply because he wants to.  While it is reasonable to say you "should do X by may do Y", saying you "SHOULD do X but MAY do Y" is almost always self-contradictory.
>
> The latter is generally preferable since the former might  force clients implementing primitives for copying byte ranges to check the size of the file before issuing the copy, which in turn raises issues because such a check and the copy are done at different times and the size might change between the two operations.
>
>
The real problem is not so much that the size can change between a client
check and COPY. It is that, unlike CLONE, the COPY operation itself is not
required to be atomic by the protocol. That means that most implementations
will have to accept one of either two options:

   1. The server on which the source file resides must somehow cooperate to
   prevent truncation of the file while the copy is in progress. Note that
   since the client MAY be holding a file byte range or share lock, it is
   impossible for either one of the servers to make use of file locks.
   2. Accept the inevitability that a truncate on the source may cut the
   COPY short at any time. i.e. that the TOCTOU race condition lasts for the
   entire duration of the (synchronous or asynchronous) copy.

Note also that if the _client_ wants to enforce this EINVAL semantic, it
can easily throw in a VERIFY operation in order to check the file length
before attempting the COPY operation. Granted that is also non-atomic, but
that's about as good as it gets here.

---

From: David Noveck <[email-masked]>
Date: Thu, 23 Feb 2017 08:00:09 -0500

required to be
> atomic by the protocol.

And while the protocol could define it as atomic, there isn't a lot of
point in doing that.  There are not many servers that could implement it
that way, and a non-atomic copy that avoids a lot of OTW traffic is better
than a non-atomic copy that wastes network bandwidth.  In any case, if you
don't want the file changed while it is copied, you can open it with
DENY=WRITE before doing the COPY.

Let's get back to the issue of the erroneous specification of COPY
semantics in RFC 7862.

Let's call the behavior currently specified as a MUST in ETC 7862,
returning INVAL if the area to be copied spans the file's EOF, Behavior A.

The more sensible behavior, allowing the copy to proceed, terminating the
range to be copied at the EOF, will be called behavior B.

I don't have any doubt that RFC 7862 is, as Bruce says, "just wrong" and
that behavior B is the right choice.  The question is what to do about the
issue.

Apart from the question of any possible errata one might or might not
submit, the important point is that servers that manifest behavior A
actually exist and will continue to exist for some time.

In light of this fact, clients might need to adapt to the situation that
such servers exist.  Whether they do so, depends on how serious they
consider the issue that Jorge has brought to our attention.  I can see some
clients ignoring this issue since copying a range, as opposed to a whole
file, is an unusual case.

Despite that, it is reasonable for a client to deal with the possibility of
behavior-A servers by checking for a copy ending point past EOF and
shortening the range to be copied.  Behavior-B servers would not be
affected.

With regard to Bruce's concerns about potential bad consequences resulting
from changes in EOF after the check and before the copy, I haven't been
able to come up with a case where anything bad happens. You can't make the
check-copy combination atomic, because copy itself isn't atomic.

With regard to Trond's suggestion to use VERIFY, it does narrow the
window but the fundamental problem of the non-atomicity of COPY remains.
Also, I'd be reluctant to implement things this way because it is hard to
figure out what to do if the VERIFY fails.  You may need to retry for a
while and then try to do the copy using READs and WRITEs.  You wind up with
a big bunch of code that is almost never tested.  I wouldn't want to go
there.

So this brings up back to the errata question.  The goal of an errata would
be to prevent more behavior-A servers from being written, since we can't be
sure that any that exist are fixed/decommissioned.

The simplest and most obvious errata is one that simply switches from
mandating behavior A to mandating behavior B

I am worried about the time/difficulty of getting such an errata approved.
The existing text, by using "MUST" really says two things:

   1. Servers have to implement behavior A.
   2. Clients depend on behavior A.

We know that these two statements are wrong, and, in other situations, the
editor  (Tom) would quickly issue a correction.

I don't know the details of the errata process but I'm pretty sure it will
be nothing like the editor simply considering the matter and making the
necessary correction.  It will involve one or more cycles of  review,
inquiry, and reassurance, to make sure that existing clients and servers
will not be adversely affected.  As a result we will have to tell people:

a) That all (or most) existing servers will be considered non-compliant

b) That the clients who have been interacting successfully with behavior-A
servers will have no problems interacting with behavior-B servers, despite
statement 2 above.

Given that, approval should not be considered a sure thing and is not going
to happen quickly.

Other possible errata texts have been proposed but they don't really
address the issue of preventing new Behavior-A servers from being created.

Given the potential difficulties of the errata process and the fact that
behavior-A servers will continue to exist for a while no matter what
happens, I think the focus has to be on client changes to work around
behavior A, if this issue is important enough to do anything about.

On Feb 18, 2017 11:23 PM, "Trond Myklebust" <[email-masked]> wrote:

---

From: "J. Bruce Fields" <[email-masked]>
Date: Thu, 23 Feb 2017 10:03:06 -0500

> that behavior B is the right choice.  The question is what to do about the

I think what we really want is to change the MUST to something weaker
(MAY?).

Maybe it doesn't work, but I the intention was to allow COPY to be
implemented by either a copy-like or clone-like backend.

> sure that any that exist are fixed/decommissioned.

So, my only worry would be to avoid clients that are unprepared to deal
with a short read.  Thankfully I think short reads are already allowed
for other reasons, so I think the risk of a major interoperability
problem here is low?

--b.

---

From: David Noveck <[email-masked]>
Date: Thu, 23 Feb 2017 16:46:33 -0500

> I think what we really want is to change the MUST to something weaker

I think we should stay away from the question of what we really want,
since everybody's preferences are different.

Fortunately, we don't need agreement/consensus on that.  Anyone,
can submit an errata.  All you need is a suggested corrected text that
people will not object strongly to and will regard as an improvement.
Given how broken the existing text is, that is a pretty low bar.

I have already suggested using MAY so I'm on board.  Trond
had some objections but if he can't come up with something better,
I don't think he'll object to your approach.

> implemented by either a copy-like or clone-like backend.

That's fine but the question is going to be about how such a client
deals with server's who manifest behavior A, mandated by RFC 7862
and allowed by the corrected text we are discussing.

> for other reasons, so I think the risk of a major interoperability

Given that this only occurs in the range case, which is quite rarely used,
the chance of a "major" interoperability problem is not low or
infinitesimal.
It is zero.

As far significant interoperability problems. we already have the one
pointed out by Jorge, which we know exists, and the chance of any
other hypothetical problem has to be lower than that.

If you want this change, go for  it and submit the errata.  I don't expect
opposition from within the working group but this is not going to be a
walkover.

On Thu, Feb 23, 2017 at 10:03 AM, J. Bruce Fields <[email-masked]>
wrote:

> > > > Could we please make that message to implementors stronger, but
> saying “the server SHOULD limit the are to be copied to reflect the size of
> the source file, but it MAY fail the operation with NFS4ERR_INVAL”?
> > >
> accord with RFC2119, although I recall seeing things like this in other
> documents.  SHOULD means a client is allowed to do something else under
> some fairly restrictive conditions while MAY says he can choose to do
> something simply because he wants to.  While it is reasonable to say you
> "should do X by may do Y", saying you "SHOULD do X but MAY do Y" is almost
> always self-contradictory.
> > >
> clients implementing primitives for copying byte ranges to check the size
> of the file before issuing the copy, which in turn raises issues because
> such a check and the copy are done at different times and the size might
> change between the two operations.
> > >
> > >

---

From: "J. Bruce Fields" <[email-masked]>
Date: Fri, 24 Feb 2017 17:21:35 -0500

> the chance of a "major" interoperability problem is not low or

The Linux server implementation currently caps the amount of data copied
at 4MB, returning a short result instead, to prevent arbitrary long
copies.  (Partly because we didn't want to implement async copy at
first.) 

So an app might do something like:

	offset = 0;
	while (copied = copy_file_range(fdin, offset, fdout, offset,
							len, flags) > 0)
		offset += copied;

So I expect the range case to be the normal case there even when copying
the whole file.  But we're also counting on clients and apps to handle
short copy results.

(Hm, linux's copy_file_range isn't documented to have the "0 == copy to
end of file convention".  That's a potentially ugly mismatch?  Not sure
if our API can or should be fixed.)

The possibility that the copy could return either a short read or EINVAL
make this all kind of annoying.  You might end up having to do something
like

	offset = 0;
	while (1) {
		copied = copy_file_range(fdin, offset, fdout, offset,
							len, flags) > 0)
		if (copied == -EINVAL) {
			/* Uh, I dunno, maybe stat the file for a new
			 * length and retry? */
		} else if (copied < 0)
			/* error out */
		else 
			offset += copied;
	}

But, anyway, I guess that's doable.

--b.

---

From: Olga Kornievskaia <[email-masked]>
Date: Tue, 28 Feb 2017 11:18:22 -0500

>                         /* Uh, I dunno, maybe stat the file for a new

Bruce, just checking in general for returned EINVAL isn't great
because there is also the case when the source offset is beyond the
end of the file. So now we are forcing the client (application) to
distinguish between this case that is not OK and another case where it
might be ok?

> But, anyway, I guess that's doable.

An an implementor, I'm still a bit confused about the direction to
take especially on the client side. On the server side, I'm concluding
from this discussion that it's beneficial to ignore the error of
copying beyond the end of the file and returning to the client a
partial copy.

On the client side, I see 3 possibilities that the client can receive
in asynchronous copy case

say client sent COPY bytes 0-100 but say there are only 50bytes in the file.
server can send back
1. CB_OFFLOAD status NFS4_OK bytes copied 50.
 -- given that client needs to be prepared for a short read anyway,
should it retry from offset 50 and now offset is beyond the end of the
file and should the whole copy fail?
2. CB_OFFLOAD status EINVAL bytes copied 50.
 -- should the client application (or the NFS layer) fail since EINVAL
was returned or should it ignore it and return success to the user?
3. CB_OFFLOAD status EINVAL bytes copied -1 (or 0).
  -- this is the case where the server strictly enforces the current
spec. This is the clearest case, client can't do anything but fail.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 28 Feb 2017 11:44:20 -0500

> file and should the whole copy fail?

I think that's right, yes.

> 2. CB_OFFLOAD status EINVAL bytes copied 50.

Fortunately, this isn't possible--the xdr for bytes copied isn't even
there in the error case.

--b.

---

From: David Noveck <[email-masked]>
Date: Tue, 28 Feb 2017 11:52:34 -0500

The bytes copied is only included in the success case, but .... The text
talks about the bytes copied being returned in the failure case :-(

On Feb 28, 2017 11:44 AM, "J. Bruce Fields" <[email-masked]> wrote:

> > >                                                         len, flags) >
> 0)
> > >                                                         len, flags) >
> 0)

---

From: "Adamson, Andy" <[email-masked]>
Date: Tue, 28 Feb 2017 17:04:27 +0000

I also thought that bytes copied is only included in the success case – but as Olga pointed out, not true. The coa_status of NFS4_OK has the coa_resok4->wr_count showing the bytes copied, and the default (e.g. failure cases) has the coa_bytes_copied.

So the server can send a CB_OFFLOAD with an error (such as the –EINVAL in the example) and send the bytes copied in coa_bytes_copied.

   union offload_info4 switch (nfsstat4 coa_status) {
   case NFS4_OK:
           write_response4 coa_resok4;
   default:
           length4         coa_bytes_copied;
   };

-->Andy

From: nfsv4 <[email-masked]> on behalf of David Noveck <[email-masked]>
Date: Tuesday, February 28, 2017 at 11:52 AM
To: "J. Bruce Fields" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>
Subject: Re: [nfsv4] Server-side copy question

The bytes copied is only included in the success case, but .... The text talks about the bytes copied being returned in the failure case :-(

On Feb 28, 2017 11:44 AM, "J. Bruce Fields" <[email-masked]<mailto:[email-masked]>> wrote:
On Tue, Feb 28, 2017 at 11:18:22AM -0500, Olga Kornievskaia wrote:
> On Fri, Feb 24, 2017 at 5:21 PM, J. Bruce Fields <[email-masked]<mailto:[email-masked]>> wrote:

--b.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 28 Feb 2017 12:32:15 -0500

>            length4         coa_bytes_copied;

Gah, I'm sorry, Olga said CB_OFFLOAD but for some reason I was looking
at COPY.

I have no idea what to think of that.  I don't see why it should be
different from COPY.

--b.