Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 24 Nov 2020 15:49:56 -0500
From:   "J. Bruce Fields" <bfields@fieldses.org>
To:     Dai Ngo <dai.ngo@oracle.com>
Cc:     linux-nfs@vger.kernel.org
Subject: Re: [PATCH] NFSD: Fix 5 seconds delay when doing inter server copy
Message-ID: <20201124204956.GB7173@fieldses.org>
References: <20201124031609.67297-1-dai.ngo@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201124031609.67297-1-dai.ngo@oracle.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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