> On Apr 25, 2021, at 10:05 AM, Olga Kornievskaia <olga.kornievskaia@xxxxxxxxx> wrote:
>
> On Sat, Apr 24, 2021 at 1:52 PM Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
>>
>> Hi Olga-
>>
>>> On Apr 22, 2021, at 4:29 PM, Olga Kornievskaia <olga.kornievskaia@xxxxxxxxx> wrote:
>>>
>>> From: Olga Kornievskaia <kolga@xxxxxxxxxx>
>>>
>>> Currently, the server does all copies as NFS_UNSTABLE. For synchronous
>>> copies linux client will append a COMMIT to the COPY compound but for
>>> async copies it does not (because COMMIT needs to be done after all
>>> bytes are copied and not as a reply to the COPY operation).
>>>
>>> However, in order to save the client doing a COMMIT as a separate
>>> rpc, the server can reply back with NFS_FILE_SYNC copy. This patch
>>> proposed to add vfs_fsync() call at the end of the async copy.
>>
>> I'm having trouble understanding the description. Are you saying
>> the client does a COPY then a COMMIT, or that the source server
>> is doing WRITEs and then a COMMIT? Just suggesting a little more
>> clarity (or an ASCII diagram) might help the weary reviewer.
>
> Client is doing a COMMIT after receiving the reply of the asynchronous
> copy in the CB_OFFLOAD where the server indicates that copy was done
> as NFS_UNSTABLE.
IIUC, then, the sequence of operations between the servers is not
changing. My concern was the patch would cause more FILE_SYNC WRITEs,
and that does not seem to be happening.
No objection from me.
> If the server replied that the copy was done as
> NFS_FILE_SYNC, then the client wouldn't need to send the additional
> COMMIT rpc. That's what this patch proposes to do. The disadvantage to
> this approach is that if some other implementation has a design where
> multiple copies are sent to satisfy a larger copy then that
> implementation might prefer to do a single commit later. But a linux
> client only sends a whole copy that was requested by the application
> which is always followed then by COMMIT so to me it makes sense to say
> the round trip and do the copy with fsync.
>
>>> Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
>>> ---
>>> fs/nfsd/nfs4proc.c | 23 ++++++++++++++++++-----
>>> 1 file changed, 18 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index 66dea2f1eed8..f63a2cb14a5e 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -1536,19 +1536,21 @@ static const struct nfsd4_callback_ops nfsd4_cb_offload_ops = {
>>> .done = nfsd4_cb_offload_done
>>> };
>>>
>>> -static void nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
>>> +static void nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync,
>>> + bool committed)
>>> {
>>> - copy->cp_res.wr_stable_how = NFS_UNSTABLE;
>>> + copy->cp_res.wr_stable_how = committed ? NFS_FILE_SYNC : NFS_UNSTABLE;
>>> copy->cp_synchronous = sync;
>>> gen_boot_verifier(©->cp_res.wr_verifier, copy->cp_clp->net);
>>> }
>>>
>>> -static ssize_t _nfsd_copy_file_range(struct nfsd4_copy *copy)
>>> +static ssize_t _nfsd_copy_file_range(struct nfsd4_copy *copy, bool *committed)
>>
>> Nit: Instead of adding an output parameter, would it make sense
>> to add the boolean to struct nfsd4_copy?
>
> Sure thing.
>
>>> {
>>> ssize_t bytes_copied = 0;
>>> size_t bytes_total = copy->cp_count;
>>> u64 src_pos = copy->cp_src_pos;
>>> u64 dst_pos = copy->cp_dst_pos;
>>> + __be32 status;
>>>
>>> do {
>>> if (kthread_should_stop())
>>> @@ -1563,6 +1565,16 @@ static ssize_t _nfsd_copy_file_range(struct nfsd4_copy *copy)
>>> src_pos += bytes_copied;
>>> dst_pos += bytes_copied;
>>> } while (bytes_total > 0 && !copy->cp_synchronous);
>>> + /* for a non-zero asynchronous copy do a commit of data */
>>> + if (!copy->cp_synchronous && copy->cp_res.wr_bytes_written > 0) {
>>> + down_write(©->nf_dst->nf_rwsem);
>>> + status = vfs_fsync_range(copy->nf_dst->nf_file,
>>> + copy->cp_dst_pos,
>>> + copy->cp_res.wr_bytes_written, 0);
>>> + up_write(©->nf_dst->nf_rwsem);
>>> + if (!status)
>>> + *committed = true;
>>> + }
>>> return bytes_copied;
>>> }
>>>
>>> @@ -1570,15 +1582,16 @@ static __be32 nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
>>> {
>>> __be32 status;
>>> ssize_t bytes;
>>> + bool committed = false;
>>>
>>> - bytes = _nfsd_copy_file_range(copy);
>>> + bytes = _nfsd_copy_file_range(copy, &committed);
>>> /* for async copy, we ignore the error, client can always retry
>>> * to get the error
>>> */
>>> if (bytes < 0 && !copy->cp_res.wr_bytes_written)
>>> status = nfserrno(bytes);
>>> else {
>>> - nfsd4_init_copy_res(copy, sync);
>>> + nfsd4_init_copy_res(copy, sync, committed);
>>> status = nfs_ok;
>>> }
>>>
>>> --
>>> 2.27.0
>>>
>>
>> --
>> Chuck Lever
--
Chuck Lever
