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

[Olga Kornievskaia <olga.kornievskaia@xxxxxxxxx>]

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. 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(&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)
>
> 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(&copy->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(&copy->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
>
>
>