Re: [RFC 1/1] NFSD add vfs_fsync after async copy is done

[dai.ngo@oracle.com]

On 4/22/21 10:27 AM, Olga Kornievskaia wrote:
> On Thu, Apr 22, 2021 at 12:54 PM <dai.ngo@oracle.com> wrote:
>>
>> On 4/22/21 9:19 AM, Olga Kornievskaia wrote:
>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>
>>> 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.
>>>
>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>> ---
>>>    fs/nfsd/nfs4proc.c | 22 +++++++++++++++++-----
>>>    1 file changed, 17 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index 66dea2f1eed8..c6f45f67d59b 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(&copy->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)
>>>    {
>>>        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,15 @@ 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 && bytes_copied > 0) {
>> I think (bytes_copied > 0) should be (bytes_total < copy->cp_count).
> I don't think so. A successful async copy should never be less the
> requested bytes.

nfsd_copy_file_range can return 0 on the last write in the loop which
causes the test (bytes_copied > 0) to fail which then skipping the
commit. The check (bytes_total < copy->cp_count) says do the commit
if there are any bytes successfully written so far.

-Dai

>
>> -Dai
>>
>>> +             down_write(&copy->nf_dst->nf_rwsem);
>>> +             status = vfs_fsync_range(copy->nf_dst->nf_file,
>>> +                                      copy->cp_dst_pos, bytes_copied, 0);
>>> +             up_write(&copy->nf_dst->nf_rwsem);
>>> +             if (!status)
>>> +                     *committed = true;
>>> +     }
>>>        return bytes_copied;
>>>    }
>>>
>>> @@ -1570,15 +1581,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;
>>>        }
>>>