[PATCH] NFSD: Fix 5 seconds delay when doing inter server copy

[PATCH] NFSD: Fix 5 seconds delay when doing inter server copy

[Dai Ngo]

Since commit b4868b44c5628 ("NFSv4: Wait for stateid updates after
CLOSE/OPEN_DOWNGRADE"), every inter server copy operation suffers 5
seconds delay regardless of the size of the copy. The delay is from
nfs_set_open_stateid_locked when the check by nfs_stateid_is_sequential
fails because the seqid in both nfs4_state and nfs4_stateid are 0.

Fix by modifying the source server to return the stateid for COPY_NOTIFY
request with seqid 1 instead of 0. This is also to conform with
section 4.8 of RFC 7862.

Here is the relevant paragraph from section 4.8 of RFC 7862:

   A copy offload stateid's seqid MUST NOT be zero.  In the context of a
   copy offload operation, it is inappropriate to indicate "the most
   recent copy offload operation" using a stateid with a seqid of zero
   (see Section 8.2.2 of [RFC5661]).  It is inappropriate because the
   stateid refers to internal state in the server and there may be
   several asynchronous COPY operations being performed in parallel on
   the same file by the server.  Therefore, a copy offload stateid with
   a seqid of zero MUST be considered invalid.

Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfsd/nfs4state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d7f27ed6b794..33ee1a6961e3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -793,6 +793,7 @@ struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
 	refcount_set(&cps->cp_stateid.sc_count, 1);
 	if (!nfs4_init_cp_state(nn, &cps->cp_stateid, NFS4_COPYNOTIFY_STID))
 		goto out_free;
+	cps->cp_stateid.stid.si_generation = 1;
 	spin_lock(&nn->s2s_cp_lock);
 	list_add(&cps->cp_list, &p_stid->sc_cp_list);
 	spin_unlock(&nn->s2s_cp_lock);
-- 
2.9.5

Re: [PATCH] NFSD: Fix 5 seconds delay when doing inter server copy

[J. Bruce Fields]

On Mon, Nov 23, 2020 at 10:16:09PM -0500, Dai Ngo wrote:
> Since commit b4868b44c5628 ("NFSv4: Wait for stateid updates after
> CLOSE/OPEN_DOWNGRADE"), every inter server copy operation suffers 5
> seconds delay regardless of the size of the copy. The delay is from
> nfs_set_open_stateid_locked when the check by nfs_stateid_is_sequential
> fails because the seqid in both nfs4_state and nfs4_stateid are 0.
> 
> Fix by modifying the source server to return the stateid for COPY_NOTIFY
> request with seqid 1 instead of 0. This is also to conform with
> section 4.8 of RFC 7862.
> 
> Here is the relevant paragraph from section 4.8 of RFC 7862:
> 
>    A copy offload stateid's seqid MUST NOT be zero.  In the context of a
>    copy offload operation, it is inappropriate to indicate "the most
>    recent copy offload operation" using a stateid with a seqid of zero
>    (see Section 8.2.2 of [RFC5661]).  It is inappropriate because the
>    stateid refers to internal state in the server and there may be
>    several asynchronous COPY operations being performed in parallel on
>    the same file by the server.  Therefore, a copy offload stateid with
>    a seqid of zero MUST be considered invalid.
> 
> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d7f27ed6b794..33ee1a6961e3 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -793,6 +793,7 @@ struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
>  	refcount_set(&cps->cp_stateid.sc_count, 1);
>  	if (!nfs4_init_cp_state(nn, &cps->cp_stateid, NFS4_COPYNOTIFY_STID))
>  		goto out_free;
> +	cps->cp_stateid.stid.si_generation = 1;

This affects the stateid returned by COPY_NOTIFY, but not the one
returned by COPY.  I think we wan to add this to nfs4_init_cp_state()
and cover both.

--b.

>  	spin_lock(&nn->s2s_cp_lock);
>  	list_add(&cps->cp_list, &p_stid->sc_cp_list);
>  	spin_unlock(&nn->s2s_cp_lock);
> -- 
> 2.9.5

Re: [PATCH] NFSD: Fix 5 seconds delay when doing inter server copy

[Chuck Lever]

Hello Dai -

> On Nov 24, 2020, at 3:49 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Mon, Nov 23, 2020 at 10:16:09PM -0500, Dai Ngo wrote:
>> Since commit b4868b44c5628 ("NFSv4: Wait for stateid updates after
>> CLOSE/OPEN_DOWNGRADE"), every inter server copy operation suffers 5
>> seconds delay regardless of the size of the copy. The delay is from
>> nfs_set_open_stateid_locked when the check by nfs_stateid_is_sequential
>> fails because the seqid in both nfs4_state and nfs4_stateid are 0.
>> 
>> Fix by modifying the source server to return the stateid for COPY_NOTIFY
>> request with seqid 1 instead of 0. This is also to conform with
>> section 4.8 of RFC 7862.
>> 
>> Here is the relevant paragraph from section 4.8 of RFC 7862:
>> 
>>   A copy offload stateid's seqid MUST NOT be zero.  In the context of a
>>   copy offload operation, it is inappropriate to indicate "the most
>>   recent copy offload operation" using a stateid with a seqid of zero
>>   (see Section 8.2.2 of [RFC5661]).  It is inappropriate because the
>>   stateid refers to internal state in the server and there may be
>>   several asynchronous COPY operations being performed in parallel on
>>   the same file by the server.  Therefore, a copy offload stateid with
>>   a seqid of zero MUST be considered invalid.
>> 
>> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>> fs/nfsd/nfs4state.c | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index d7f27ed6b794..33ee1a6961e3 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -793,6 +793,7 @@ struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
>> 	refcount_set(&cps->cp_stateid.sc_count, 1);
>> 	if (!nfs4_init_cp_state(nn, &cps->cp_stateid, NFS4_COPYNOTIFY_STID))
>> 		goto out_free;
>> +	cps->cp_stateid.stid.si_generation = 1;
> 
> This affects the stateid returned by COPY_NOTIFY, but not the one
> returned by COPY.  I think we wan to add this to nfs4_init_cp_state()
> and cover both.

Since time is creeping on towards the next merge window, I assume
this particular fix needs to go there, but I don't see the final
version of it (with Bruce's suggested fix) on the list. Did I miss
it?


>> 	spin_lock(&nn->s2s_cp_lock);
>> 	list_add(&cps->cp_list, &p_stid->sc_cp_list);
>> 	spin_unlock(&nn->s2s_cp_lock);
>> -- 
>> 2.9.5

--
Chuck Lever
chucklever@gmail.com

Re: [PATCH] NFSD: Fix 5 seconds delay when doing inter server copy

[Dai Ngo]

Hi Chuck,

Sorry for the delay. I will make update the patch, test it, and re-submit
it by end of today.

Thanks,
-Dai

On 11/30/20 9:57 AM, Chuck Lever wrote:
> Hello Dai -
>
>> On Nov 24, 2020, at 3:49 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>
>> On Mon, Nov 23, 2020 at 10:16:09PM -0500, Dai Ngo wrote:
>>> Since commit b4868b44c5628 ("NFSv4: Wait for stateid updates after
>>> CLOSE/OPEN_DOWNGRADE"), every inter server copy operation suffers 5
>>> seconds delay regardless of the size of the copy. The delay is from
>>> nfs_set_open_stateid_locked when the check by nfs_stateid_is_sequential
>>> fails because the seqid in both nfs4_state and nfs4_stateid are 0.
>>>
>>> Fix by modifying the source server to return the stateid for COPY_NOTIFY
>>> request with seqid 1 instead of 0. This is also to conform with
>>> section 4.8 of RFC 7862.
>>>
>>> Here is the relevant paragraph from section 4.8 of RFC 7862:
>>>
>>>    A copy offload stateid's seqid MUST NOT be zero.  In the context of a
>>>    copy offload operation, it is inappropriate to indicate "the most
>>>    recent copy offload operation" using a stateid with a seqid of zero
>>>    (see Section 8.2.2 of [RFC5661]).  It is inappropriate because the
>>>    stateid refers to internal state in the server and there may be
>>>    several asynchronous COPY operations being performed in parallel on
>>>    the same file by the server.  Therefore, a copy offload stateid with
>>>    a seqid of zero MUST be considered invalid.
>>>
>>> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>> ---
>>> fs/nfsd/nfs4state.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index d7f27ed6b794..33ee1a6961e3 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -793,6 +793,7 @@ struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
>>> 	refcount_set(&cps->cp_stateid.sc_count, 1);
>>> 	if (!nfs4_init_cp_state(nn, &cps->cp_stateid, NFS4_COPYNOTIFY_STID))
>>> 		goto out_free;
>>> +	cps->cp_stateid.stid.si_generation = 1;
>> This affects the stateid returned by COPY_NOTIFY, but not the one
>> returned by COPY.  I think we wan to add this to nfs4_init_cp_state()
>> and cover both.
> Since time is creeping on towards the next merge window, I assume
> this particular fix needs to go there, but I don't see the final
> version of it (with Bruce's suggested fix) on the list. Did I miss
> it?
>
>
>>> 	spin_lock(&nn->s2s_cp_lock);
>>> 	list_add(&cps->cp_list, &p_stid->sc_cp_list);
>>> 	spin_unlock(&nn->s2s_cp_lock);
>>> -- 
>>> 2.9.5
> --
> Chuck Lever
> chucklever@gmail.com
>
>
>

Re: [PATCH] NFSD: Fix 5 seconds delay when doing inter server copy

[Dai Ngo]

On 11/24/20 12:49 PM, J. Bruce Fields wrote:
> On Mon, Nov 23, 2020 at 10:16:09PM -0500, Dai Ngo wrote:
>> Since commit b4868b44c5628 ("NFSv4: Wait for stateid updates after
>> CLOSE/OPEN_DOWNGRADE"), every inter server copy operation suffers 5
>> seconds delay regardless of the size of the copy. The delay is from
>> nfs_set_open_stateid_locked when the check by nfs_stateid_is_sequential
>> fails because the seqid in both nfs4_state and nfs4_stateid are 0.
>>
>> Fix by modifying the source server to return the stateid for COPY_NOTIFY
>> request with seqid 1 instead of 0. This is also to conform with
>> section 4.8 of RFC 7862.
>>
>> Here is the relevant paragraph from section 4.8 of RFC 7862:
>>
>>     A copy offload stateid's seqid MUST NOT be zero.  In the context of a
>>     copy offload operation, it is inappropriate to indicate "the most
>>     recent copy offload operation" using a stateid with a seqid of zero
>>     (see Section 8.2.2 of [RFC5661]).  It is inappropriate because the
>>     stateid refers to internal state in the server and there may be
>>     several asynchronous COPY operations being performed in parallel on
>>     the same file by the server.  Therefore, a copy offload stateid with
>>     a seqid of zero MUST be considered invalid.
>>
>> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4state.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index d7f27ed6b794..33ee1a6961e3 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -793,6 +793,7 @@ struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
>>   	refcount_set(&cps->cp_stateid.sc_count, 1);
>>   	if (!nfs4_init_cp_state(nn, &cps->cp_stateid, NFS4_COPYNOTIFY_STID))
>>   		goto out_free;
>> +	cps->cp_stateid.stid.si_generation = 1;
> This affects the stateid returned by COPY_NOTIFY, but not the one
> returned by COPY.  I think we wan to add this to nfs4_init_cp_state()
> and cover both.

Hi Bruce, thank you for your suggestion. Updated patch tested and submitted.

-Dai

P.S sorry for the delay, I was on leave last few days.

>
> --b.
>
>>   	spin_lock(&nn->s2s_cp_lock);
>>   	list_add(&cps->cp_list, &p_stid->sc_cp_list);
>>   	spin_unlock(&nn->s2s_cp_lock);
>> -- 
>> 2.9.5

[PATCH] NFSD: Fix 5 seconds delay when doing inter server copy

[Dai Ngo]

Since commit b4868b44c5628 ("NFSv4: Wait for stateid updates after
CLOSE/OPEN_DOWNGRADE"), every inter server copy operation suffers 5
seconds delay regardless of the size of the copy. The delay is from
nfs_set_open_stateid_locked when the check by nfs_stateid_is_sequential
fails because the seqid in both nfs4_state and nfs4_stateid are 0.

Fix by modifying nfs4_init_cp_state to return the stateid with seqid 1
instead of 0. This is also to conform with section 4.8 of RFC 7862.

Here is the relevant paragraph from section 4.8 of RFC 7862:

   A copy offload stateid's seqid MUST NOT be zero.  In the context of a
   copy offload operation, it is inappropriate to indicate "the most
   recent copy offload operation" using a stateid with a seqid of zero
   (see Section 8.2.2 of [RFC5661]).  It is inappropriate because the
   stateid refers to internal state in the server and there may be
   several asynchronous COPY operations being performed in parallel on
   the same file by the server.  Therefore, a copy offload stateid with
   a seqid of zero MUST be considered invalid.

Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfsd/nfs4state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d7f27ed6b794..47006eec724e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -769,6 +769,7 @@ static int nfs4_init_cp_state(struct nfsd_net *nn, copy_stateid_t *stid,
 	spin_lock(&nn->s2s_cp_lock);
 	new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, stid, 0, 0, GFP_NOWAIT);
 	stid->stid.si_opaque.so_id = new_id;
+	stid->stid.si_generation = 1;
 	spin_unlock(&nn->s2s_cp_lock);
 	idr_preload_end();
 	if (new_id < 0)
-- 
2.9.5

Re: [PATCH] NFSD: Fix 5 seconds delay when doing inter server copy

[Chuck Lever]

> On Nov 30, 2020, at 4:24 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
> 
> Since commit b4868b44c5628 ("NFSv4: Wait for stateid updates after
> CLOSE/OPEN_DOWNGRADE"), every inter server copy operation suffers 5
> seconds delay regardless of the size of the copy. The delay is from
> nfs_set_open_stateid_locked when the check by nfs_stateid_is_sequential
> fails because the seqid in both nfs4_state and nfs4_stateid are 0.
> 
> Fix by modifying nfs4_init_cp_state to return the stateid with seqid 1
> instead of 0. This is also to conform with section 4.8 of RFC 7862.
> 
> Here is the relevant paragraph from section 4.8 of RFC 7862:
> 
>   A copy offload stateid's seqid MUST NOT be zero.  In the context of a
>   copy offload operation, it is inappropriate to indicate "the most
>   recent copy offload operation" using a stateid with a seqid of zero
>   (see Section 8.2.2 of [RFC5661]).  It is inappropriate because the
>   stateid refers to internal state in the server and there may be
>   several asynchronous COPY operations being performed in parallel on
>   the same file by the server.  Therefore, a copy offload stateid with
>   a seqid of zero MUST be considered invalid.
> 
> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>

Applied for the next merge window. See the cel-next topic branch in
this repo:

git://git.linux-nfs.org/projects/cel/cel-2.6.git

See also:

http://git.linux-nfs.org/?p=cel/cel-2.6.git;a=shortlog;h=refs/heads/cel-next


> ---
> fs/nfsd/nfs4state.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d7f27ed6b794..47006eec724e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -769,6 +769,7 @@ static int nfs4_init_cp_state(struct nfsd_net *nn, copy_stateid_t *stid,
> 	spin_lock(&nn->s2s_cp_lock);
> 	new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, stid, 0, 0, GFP_NOWAIT);
> 	stid->stid.si_opaque.so_id = new_id;
> +	stid->stid.si_generation = 1;
> 	spin_unlock(&nn->s2s_cp_lock);
> 	idr_preload_end();
> 	if (new_id < 0)
> -- 
> 2.9.5
> 

--
Chuck Lever

[PATCH] NFSD: Fix 5 seconds delay when doing inter server copy

[Dai Ngo]

Since commit b4868b44c5628, every inter server copy operation suffers
5 seconds delay regardless of the size of the copy. The delay is from
nfs_set_open_stateid_locked when the check by nfs_stateid_is_sequential
fails because the seqid in both nfs4_state and nfs4_stateid are 0. 

The nfs_stateid_is_sequential is the new replacement for the old
nfs_need_update_open_stateid check.

Fix by modifying the source server to return the stateid for COPY_NOTIFY
request with seqid 1 instead of 0. This is also to conform with section
4.8 of RFC 7682. And on the destination server, delaying the setting of
NFS_OPEN_STATE in nfs4_state to indicate this is the 1st open to pass
the check by nfs_stateid_is_sequential.

Here is the relevant paragraph from section 4.8 of RFC 7682:

   A copy offload stateid's seqid MUST NOT be zero.  In the context of a
   copy offload operation, it is inappropriate to indicate "the most
   recent copy offload operation" using a stateid with a seqid of zero
   (see Section 8.2.2 of [RFC5661]).  It is inappropriate because the
   stateid refers to internal state in the server and there may be
   several asynchronous COPY operations being performed in parallel on
   the same file by the server.  Therefore, a copy offload stateid with
   a seqid of zero MUST be considered invalid.

Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
Signed-off-by: Dai Ngo <dai.ngo@oracle.com>

---
 fs/nfs/nfs4file.c   | 2 +-
 fs/nfsd/nfs4state.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 9d354de613da..57b3821d975a 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -377,10 +377,10 @@ static struct file *__nfs42_ssc_open(struct vfsmount *ss_mnt,
 		goto out_stateowner;
 
 	set_bit(NFS_SRV_SSC_COPY_STATE, &ctx->state->flags);
-	set_bit(NFS_OPEN_STATE, &ctx->state->flags);
 	memcpy(&ctx->state->open_stateid.other, &stateid->other,
 	       NFS4_STATEID_OTHER_SIZE);
 	update_open_stateid(ctx->state, stateid, NULL, filep->f_mode);
+	set_bit(NFS_OPEN_STATE, &ctx->state->flags);
 
 	nfs_file_set_open_context(filep, ctx);
 	put_nfs_open_context(ctx);
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d7f27ed6b794..33ee1a6961e3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -793,6 +793,7 @@ struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
 	refcount_set(&cps->cp_stateid.sc_count, 1);
 	if (!nfs4_init_cp_state(nn, &cps->cp_stateid, NFS4_COPYNOTIFY_STID))
 		goto out_free;
+	cps->cp_stateid.stid.si_generation = 1;
 	spin_lock(&nn->s2s_cp_lock);
 	list_add(&cps->cp_list, &p_stid->sc_cp_list);
 	spin_unlock(&nn->s2s_cp_lock);
-- 
2.9.5