diff mbox series

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

Message ID 20210422202908.60995-1-olga.kornievskaia@gmail.com (mailing list archive)
State New
Headers show
Series [RFC,v2,1/1] NFSD add vfs_fsync after async copy is done | expand

Commit Message

Olga Kornievskaia April 22, 2021, 8:29 p.m. UTC
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 | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Comments

Chuck Lever III April 24, 2021, 5:51 p.m. UTC | #1
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(&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?


> {
> 	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
Olga Kornievskaia April 25, 2021, 2:05 p.m. UTC | #2
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(&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
>
>
>
Chuck Lever III April 26, 2021, 2:22 p.m. UTC | #3
> 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(&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

--
Chuck Lever
diff mbox series

Patch

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)
 {
 	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;
 	}