| Message ID | 20210422161922.56080-1-olga.kornievskaia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/1] NFSD add vfs_fsync after async copy is done | expand |
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(©->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). -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; > } >
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(©->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. > > -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; > > } > >
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(©->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(©->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; >>> } >>>
On Thu, Apr 22, 2021 at 1:37 PM <dai.ngo@oracle.com> wrote: > > > 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(©->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; > >>> } > >>>
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) { + 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; }