diff mbox series

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

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

Commit Message

Olga Kornievskaia April 22, 2021, 4:19 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 | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Comments

Dai Ngo April 22, 2021, 4:54 p.m. UTC | #1
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(&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,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(&copy->nf_dst->nf_rwsem);
> +		status = vfs_fsync_range(copy->nf_dst->nf_file,
> +					 copy->cp_dst_pos, bytes_copied, 0);
> +		up_write(&copy->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;
>   	}
>
Olga Kornievskaia April 22, 2021, 5:27 p.m. UTC | #2
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(&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,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(&copy->nf_dst->nf_rwsem);
> > +             status = vfs_fsync_range(copy->nf_dst->nf_file,
> > +                                      copy->cp_dst_pos, bytes_copied, 0);
> > +             up_write(&copy->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;
> >       }
> >
Dai Ngo April 22, 2021, 5:37 p.m. UTC | #3
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(&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,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(&copy->nf_dst->nf_rwsem);
>>> +             status = vfs_fsync_range(copy->nf_dst->nf_file,
>>> +                                      copy->cp_dst_pos, bytes_copied, 0);
>>> +             up_write(&copy->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;
>>>        }
>>>
Olga Kornievskaia April 22, 2021, 6:20 p.m. UTC | #4
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(&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,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(&copy->nf_dst->nf_rwsem);
> >>> +             status = vfs_fsync_range(copy->nf_dst->nf_file,
> >>> +                                      copy->cp_dst_pos, bytes_copied, 0);
> >>> +             up_write(&copy->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 mbox series

Patch

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(&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,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(&copy->nf_dst->nf_rwsem);
+		status = vfs_fsync_range(copy->nf_dst->nf_file,
+					 copy->cp_dst_pos, bytes_copied, 0);
+		up_write(&copy->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;
 	}