| Message ID | 20210422202908.60995-1-olga.kornievskaia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2,1/1] NFSD add vfs_fsync after async copy is done | expand |
Hi Olga- > On Apr 22, 2021, at 4:29 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> 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. 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. > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > --- > 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? > { > 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
On Sat, Apr 24, 2021 at 1:52 PM Chuck Lever III <chuck.lever@oracle.com> wrote: > > Hi Olga- > > > On Apr 22, 2021, at 4:29 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> 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. > > 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@netapp.com> > > --- > > 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 > > >
> On Apr 25, 2021, at 10:05 AM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote: > > On Sat, Apr 24, 2021 at 1:52 PM Chuck Lever III <chuck.lever@oracle.com> wrote: >> >> Hi Olga- >> >>> On Apr 22, 2021, at 4:29 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> 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. >> >> 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@netapp.com> >>> --- >>> 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
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) { 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; }