Re: [nfsv4] seeking clarifications in server-side copy CB_OFFLOAD feature

[Olga Kornievskaia <[e-mail]>]

On Fri, Mar 3, 2017 at 9:07 PM, J. Bruce Fields <[e-mail]> wrote:
> On Fri, Mar 03, 2017 at 05:28:17PM -0500, Olga Kornievskaia wrote:
>> Thanks for the comments Bruce!
>>
>> On Fri, Mar 3, 2017 at 1:48 PM, J. Bruce Fields <[e-mail]> wrote:
>> > On Thu, Mar 02, 2017 at 04:07:42PM -0500, Olga Kornievskaia wrote:
>> >> I'm seeking some clarification for doing the server-side copy feature.
>> >> EINVAL in read beyond the end of the file has been already raised in
>> >> another thread so I'll leave that aside. So here are a few more :
>> >> -- possible inconsistent behavior in presence of an error from doing
>> >> an async copy vs sync. CB_OFFLOAD can return an error and a partial
>> >> copy result but there is no mechanism to do so in synchronous COPY. If
>> >> CB_OFFLOAD returned an error and a partial copy, is that really an
>> >> error or should it be a short copy?
>> >
>> > On the client side, in the Linux case, the client is implementing
>> > copy_file_range(), which doesn't have the option of returning both an
>> > error and a number of bytes copied.  Returning an error to the
>> > application when the destination was actually modified is unacceptable.
>>
>> Are you saying it is not acceptable to fail copy_file_range() and do a
>> partial copy or am I not reading it correctly? Why is this not
>> acceptable? Currently if I do a "cp /mnt/foo /mnt2/bar" and say there
>> is not enough space "cp" will display ENOSPC error but it will copy as
>> much as it could in /mnt/bar until it failed.
>
> Maybe cp doesn't use the information that the write() system call gives
> it, but I think the system call interface should still try to provide
> this information.
>
> So, I'd still be in favor of turning nonzero copy + error reply from
> server into a succesful copy_file_range() call in every case.
>
> The one exception might be errors that the client can handle itself (say
> by retrying)?  I don't know what those might be.
>
>> > Returning success and a short copy is OK, I think--if the error's
>> > important then the application will get it when it tries to copy the
>> > rest.
>> >
>> >> I can see a case where an error should not be ignore -- if error is
>> >> ENOSPC. Should the client if received an error in CB_OFFLOAD propagate
>> >> the error to the application?
>> >
>> > I think it's OK to ignore the ENOSPC.  The application will try to write
>> > the rest, the client will turn that into a new COPY request, this time
>> > the response will probably be 0 bytes copied and ENOSPC, so this time
>> > the client can return the error to the client.
>>
>> Ok I agree about ENOSPC.
>>
>> But, in general, 0 + some ERROR might not necessarily mean an error.
>> When the server rebooted and copied 0 bytes and maybe returned an
>> error (?), it should not be considered an error on the (NFS) client.
>
> I'm trying to think about the server reboot cases, but I'm confused....
>
> In the case of just a single server, or in the server-to-server case
> when the target server reboots, I think the client just retries any
> COPYs it had in progress, the same as it would if it had a write that
> was in progress.  I don't there's anything else it can do.  The server
> doesn't have any way to return an error to the COPY after it reboots.
> Our server won't even remember it was in the middle of a copy after it
> comes back up.

Correct client will reboot the copy from scratch.

> If the source server reboots.... Yes, I guess it could just return a
> short copy result, but as you say if it didn't manage to copy any bytes,
> that might be confusing, as it might look like it hit end of file.
>
> Maybe it should return BAD_STATEID to indicate that the stateid that the
> server got in ca_src_stateid isn't accepted by the source server any
> more?  Or maybe PARTNER_NO_AUTH?

Due to the fact that VFS copy_file_range() can't return anything of
that sort, it returns EBADF. NFSD will return partial copy and
BAD_FILEHANDLE.

>> I'm actually hoping that server will return -1 and ENOSPC. But the
>> spec doesn't dictate that failure should return -1.
>
> I'm not sure what you mean by returning -1.  You mean returning with
> wr_count == -1?

As far as I can tell, "wr_count" could never be -1. wr_count is only
present on a successful copy (meaning CB_OFFLOAD error is NFS4_OK or
for synchronous case COPY's status is NFS4_OK). If error is set, then
coa_bytes_copied will be set to something. I'm suggesting it should be
-1.

>> >> Should it be server's responsibility to then if some
>> >> error occurred but a partial copy succeeded to then not return the
>> >> error to the client (that leads to the 2nd question).
>> >
>> > I'd rather servers didn't use this odd quirk of the protocol, but maybe
>> > it's too late to stop them.  And it seems easy enough just to ignore the
>> > error on the client side in the case bytes were copied.
>>
>> I can argue that it's easier for the server to ignore the error due to
>> the reboot. There is no error to represent server reboot. There is no
>> wording in the spec to state that if CB_OFFLOAD.error = X it means
>> source server reboot. So if client got CB_OFFLOAD.error = X it is
>> really guessing if it means server rebooted or something else.
>>
>> I guess I'm afraid that by ignoring all errors I'm not thinking of
>> some example where it was the wrong thing to do. Currently this is how
>> I've coded up the client to ignore the errors (all except ENOSPC which
>> I guess I can ignore as well let client got another ENOSPC) but I have
>> a nagging feeling I'm missing something.
>>
>> >> -- related to errors returned in CB_OFFLOAD, there is no guidance with
>> >> regards to the reboot recovery. If the destination server unable to
>> >> finish the copy due to the source server rebooting, should it reply
>> >> back with a successful short copy or should it return some error (and
>> >> short copy of possibly 0bytes).
>> >
>> > A short copy sounds reasonable to me.
>>
>> short copy with an error, right?
>
> I was thinking without an error, but at least in the case where there
> were 0 bytes I think you're right there needs to be an error.

I'm confused :( The way I read things you say server shouldn't hide
errors but here you are saying it should return a short copy without
an error. I guess I'll wait for you to comment on the actual code to
let me know what you think should happen there.