On Thu, Apr 22, 2021 at 1:37 PM <dai.ngo@xxxxxxxxxx> wrote:
>
>
> On 4/22/21 10:27 AM, Olga Kornievskaia wrote:
> > On Thu, Apr 22, 2021 at 12:54 PM <dai.ngo@xxxxxxxxxx> wrote:
> >>
> >> On 4/22/21 9:19 AM, Olga Kornievskaia 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.
> >>>
> >>> Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
> >>> ---
> >>> 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(©->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.
Ops, right, so actually what I used to have "if (bytes_total > 0)". I
think typically we should have bytes_total = copy_cp_count but the
reason I don't have it to be bytes_total <= copy->cp_count is then if
bytes_total=0 it would call the vfs_fsync() which I didn't what to do.
>
> -Dai
>
> >
> >> -Dai
> >>
> >>> + down_write(©->nf_dst->nf_rwsem);
> >>> + status = vfs_fsync_range(copy->nf_dst->nf_file,
> >>> + copy->cp_dst_pos, bytes_copied, 0);
> >>> + up_write(©->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;
> >>> }
> >>>
