Subject: Re: [nfsv4] Server-side copy question

From: Olga Kornievskaia <[email-masked]>
Date: Wed, 18 Oct 2017 17:22:08 -0400

On Tue, Feb 28, 2017 at 12:32 PM, J. Bruce Fields <[email-masked]> wrote:
> On Tue, Feb 28, 2017 at 05:04:27PM +0000, Adamson, Andy wrote:
>> 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;
>>    };
>
> 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.

I'd like to resurrect this thread...

This discussion was initiated to allow for the server to do a partial
copy instead of failing the copy with EINVAL when copy is requested
beyond the end of the file.

However, the problem I'm running into is that because CB_OFFLOAD only
returns "coa_bytes_copied" and does not include stable_how and
verifier. So even if client were to then send a COMMIT for the bytes
that were copied, it has no verifier to match from the COMMIT to
anything.

Should the client then ignore the lack of the matching verifier and
just assume the data was committed for the partial bytes?

>
> --b.
>
>>
>> -->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:
>> > > On Thu, Feb 23, 2017 at 04:46:33PM -0500, David Noveck wrote:
>> > >> > I think what we really want is to change the MUST to something weaker
>> > >> > (MAY?).
>> > >>
>> > >> 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.
>> > >>
>> > >> > 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.
>> > >>
>> > >> 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.
>> > >>
>> > >> > 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?
>> > >>
>> > >> 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.
>> > >
>> > > 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? */
>> >
>> > 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?
>> >
>> > >                 } else if (copied < 0)
>> > >                         /* error out */
>> > >                 else
>> > >                         offset += copied;
>> > >         }
>> > >
>> > > 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?
>>
>> 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.
>>
>> >  -- 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.
>>
>> _______________________________________________
>> nfsv4 mailing list
>> [email-masked]<mailto:[email-masked]>
>> https://www.ietf.org/mailman/listinfo/nfsv4
>
> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4

---

From: "J. Bruce Fields" <[email-masked]>
Date: Fri, 20 Oct 2017 15:33:06 -0400

> just assume the data was committed for the partial bytes?

Yes, sounds like in the asynchronous case the server has to wait for the
writes to reach disk before calling CB_OFFLOAD.  (Do we currently do
that?).

In theory that shouldn't be too expensive because asynchronous COPY
should normally be done with a very large range.

(I wonder if the server should just do a synchronous copy if the copy
would be less than a few megs?)

--b.

---

From: Olga Kornievskaia <[email-masked]>
Date: Mon, 23 Oct 2017 17:43:42 -0400

> writes to reach disk before calling CB_OFFLOAD.  (Do we currently do

Why? No the spec does not require the server to do no such thing. The
server sends back in CB_OFFLOAD if the bytes were written as UNSTABLE
or FILE_SYNC.

> would be less than a few megs?)

Yes the server could determine to convert an asynchronous copy into a
synchronous one based on size.

I guess I'm still confused how to handle the implementation so that it
doesn't kill interoperability with other possible (future)
implementations...

---

From: "J. Bruce Fields" <[email-masked]>
Date: Mon, 23 Oct 2017 21:36:46 -0400

> server sends back in CB_OFFLOAD if the bytes were written as UNSTABLE

Look back at what you said before....  So the problem you were
explaining (missing stable_how and verifier) was only for the short
write case?

So in the short write case it sounds like it *is* the server's
responsibility to commit.  That's weird, but as long as the short write
case is rare, maybe it's not a big deal?

Hm: "The coa_bytes_copied value indicates the number of bytes copied but
not which specific bytes have been copied."  That makes coa_bytes_copied
pretty much worthless.

I guess I'd be in favor of using the NFS_OK branch of CB_OFFLOAD
whenever a nonzero number of bytes is copied.  If less than the
requested number of bytes were copied, the client can always find out
the error (if any) by attempting to copy the rest.

--b.

---

From: David Noveck <[email-masked]>
Date: Tue, 24 Oct 2017 07:43:10 -0400

> responsibility to commit.

Perhaps it should be the server's responsibility, but it is not,
according to RFC7862

> That's weird,

I agree that it is wierd.  I feel it is well-intentioned but not viable
since
you are essentially creating your own protocol, if clients depend on
this behavior an there is no point in doing it, if clients do not.

> but as long as the short write

>From a performance point of view, it is not a big deal.  However it is
a big deal that the spec is broken. I hate when that happens :-(

> not which specific bytes have been copied."  That makes coa_bytes_copied

Any non-zero value of coa_bytes_copied is worthless because the client does
not know:

   - Which particular bytes were copied
   - Whether those bytes were copied to the destination stably
   - How to commit any  non-stably-written bytes.

> whenever a nonzero number of bytes is copied.

That's likely to create an interoperability problem, since there are almost
certainly clients who assume that when there is no error, all requested
bytes
have been copied.  Basically, the server is pretending there is no error
and then
relying on the client to figure out that there was one, which could only be
done if the
spec tells the clients about this.  If RFC7862 doesn't do this, you wind up
essentially creating a private protocol.

> If less than the

Unfortunately there is no way of determining which bytes were copied so
there
is no way of knowing "the rest", i.e. the bytes not copied

On Mon, Oct 23, 2017 at 9:36 PM, J. Bruce Fields <[email-masked]>
wrote:

> > >> >> 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.
> > >> >>

---

From: Olga Kornievskaia <[email-masked]>
Date: Tue, 24 Oct 2017 11:52:17 -0400

On Tue, Oct 24, 2017 at 7:43 AM, David Noveck <[email-masked]> wrote:
> Which particular bytes were copied
> Whether those bytes were copied to the destination stably
> How to commit any  non-stably-written bytes.
>
> have been copied.

I don't think it's true that clients would assume this. I think the
client can only assume server copied as many bytes as it was specified
in the CB_OFFLOAD bytes_copied regardless of how many bytes the client
had asked for. At least that's how I read the spec.

> Basically, the server is pretending there is no error and

Yes, pretending that there was no error is the part I'm not totally
comfortable with. And I agree with the comments that the client can
try again to get the same error.

> then

I would like to understand the comment by Bruce and you about not
knowing which bytes were copied. I don't understand why it would not
be the bytes from the offset to offset+bytes_copied? And Dave,
wouldn't the "rest of the bytes" be the request length - bytes_copied?
What am I missing?

If you are suggested that the server is allowed to copy a requested
range in any order of smaller chunks then there is a problem with a
non-error case because we don't returned in COPY/CB_OFFLOAD offset and
bytes_copied (just bytes_copied).

I think we know which bytes are copied we just don't know how they
were copied: stable or not (if the server chooses to specify an error
in CB_OFFLOAD).

>> > >> >> coa_resok4->wr_count showing the bytes copied, and the default (e.g. failure
>> > >> >> cases) has the coa_bytes_copied.
>> > >> >>

Yes.

>> responsibility to commit.

I disagree. I don't see anywhere in the spec that requires the server
to do that.

>>   That's weird, but as long as the short write

Yep that's what I thought.

>> the error (if any) by attempting to copy the rest.

Ok

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 24 Oct 2017 14:38:03 -0400

On Tue, Oct 24, 2017 at 11:52:17AM -0400, Olga Kornievskaia wrote:
> > That's likely to create an interoperability problem, since there are
> > almost certainly clients who assume that when there is no error, all
> > requested bytes have been copied.
> 

Agreed.  CB_OFFLOAD has a wr_count field in the NFS_OK branch, what else
could it possibly be for?  And there's ample precedent (e.g. in READ and
WRITE) for succesful IO operations returning a count value that's
possibly less than what's requested.

--b.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 24 Oct 2017 14:49:35 -0400

On Tue, Oct 24, 2017 at 07:43:10AM -0400, David Noveck wrote:
> Basically, the server is pretending there is no error and then relying
> on the client to figure out that there was one, which could only be
> done if the spec tells the clients about this.

This is exactly the behavior that READ and WRITE (and read() and
write(), for that matter) have always had, and it's the only reasonable
reading I can see of the CB_OFFLOAD spec language.

--b.

---

From: David Noveck <[email-masked]>
Date: Wed, 25 Oct 2017 06:30:33 -0400

> wouldn't the "rest of the bytes" be the request length - bytes_copied?s
have been copied."

It might be and it is resonable if it is but RFC7862, says, as Bruce noted,
"The coa_bytes_copied value indicates the number of bytes copied but
not which specific bytes.".  I don't know wny it says that or how those
words came
to be in the spec.  If the spec did not say that, it would reasonable to
assume that it
would be the bytes from the offset to offset+bytes_copied.  But given
RFC7862,
as it is, I think clients cannot assume that.  It may be that these words
are just
plaiin wrong, but assuming that is risky, and fixing the spec to remove
them is very
difficult, even if everyone were to agree that they shouldn't be there.
,

On Tue, Oct 24, 2017 at 11:52 AM, Olga Kornievskaia <[email-masked]>
wrote:

> almost
> >> > On Fri, Oct 20, 2017 at 3:33 PM, J. Bruce Fields <
> [email-masked]>
> >> > wrote:

---

From: Olga Kornievskaia <[email-masked]>
Date: Wed, 25 Oct 2017 09:28:42 -0400

On Wed, Oct 25, 2017 at 6:30 AM, David Noveck <[email-masked]> wrote:
> plaiin wrong, but assuming that is risky, and fixing the spec to remove them
> is very

Ah, got it. Thank you.

>> >> > On Fri, Oct 20, 2017 at 3:33 PM, J. Bruce Fields
>> >> > <[email-masked]>
>> >> > wrote:
>> >> > > Yes, sounds like in the asynchronous case the server has to wait
>> >> > > for
>> >> > > the