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, 10 Nov 2020 15:12:39 -0500
From:   "J. Bruce Fields" <bfields@fieldses.org>
To:     Dai Ngo <dai.ngo@oracle.com>
Cc:     linux-nfs@vger.kernel.org
Subject: Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter
 server copy
Message-ID: <20201110201239.GA17755@fieldses.org>
References: <20201019034249.27990-1-dai.ngo@oracle.com>
 <20201020170114.GF1133@fieldses.org>
 <fb514565-cd47-9180-2adc-f3ba4459202b@oracle.com>
 <20201109183054.GD11144@fieldses.org>
 <eeafd9e2-5d04-848e-d330-670e2185098d@oracle.com>
 <20201109204206.GA20261@fieldses.org>
 <7a18452a-3120-ea5b-f676-9d7e18a65446@oracle.com>
 <470b690f-c919-2c48-95b7-18cc75f71f70@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <470b690f-c919-2c48-95b7-18cc75f71f70@oracle.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Mon, Nov 09, 2020 at 10:46:12PM -0800, Dai Ngo wrote:
> 
> On 11/9/20 2:26 PM, Dai Ngo wrote:
> >
> >On 11/9/20 12:42 PM, J. Bruce Fields wrote:
> >>On Mon, Nov 09, 2020 at 11:34:08AM -0800, Dai Ngo wrote:
> >>>On 11/9/20 10:30 AM, J. Bruce Fields wrote:
> >>>>On Tue, Oct 20, 2020 at 11:34:35AM -0700, Dai Ngo wrote:
> >>>>>On 10/20/20 10:01 AM, J. Bruce Fields wrote:
> >>>>>>On Sun, Oct 18, 2020 at 11:42:49PM -0400, Dai Ngo wrote:
> >>>>>>>NFS_FS=y as dependency of CONFIG_NFSD_V4_2_INTER_SSC still have
> >>>>>>>build errors and some configs with NFSD=m to get NFS4ERR_STALE
> >>>>>>>error when doing inter server copy.
> >>>>>>>
> >>>>>>>Added ops table in nfs_common for knfsd to access NFS
> >>>>>>>client modules.
> >>>>>>OK, looks reasonable to me, applying.  Does this resolve all the
> >>>>>>problems you've seen, or is there any bad case left?
> >>>>>Thanks Bruce.
> >>>>>
> >>>>>With this patch, I no longer see the NFS4ERR_STALE in any config.
> >>>>>
> >>>>>The problem with NFS4ERR_STALE was because of a bug in
> >>>>>nfs42_ssc_open.
> >>>>>When CONFIG_NFSD_V4_2_INTER_SSC is not defined, nfs42_ssc_open
> >>>>>returns NULL which is incorrect allowing the operation to continue
> >>>>>until nfsd4_putfh which does not have the code to handle
> >>>>>nfserr_stale.
> >>>>>
> >>>>>With this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not defined the
> >>>>>new nfs42_ssc_open returns ERR_PTR(-EIO) which causes the NFS client
> >>>>>to switch over to the split copying (read src and write to dst).
> >>>>That sounds reasonable, but I don't see any of the patches you've sent
> >>>>changing that error return.  Did I overlook something, or did you mean
> >>>>to append a patch to this message?
> >>>Since with the patch, I did not run into the condition where
> >>>NFS4ERR_STALE
> >>>is returned so I did not fix this return error code. Do you want me to
> >>>submit another patch to change the returned error code from
> >>>NFS4ERR_STALE
> >>>to NFS4ERR_NOTSUPP if it ever runs into that condition?
> >>That would be great, thanks.  (I mean, it is still possible to hit that
> >>case, right?  You just didn't test with !CONFIG_NFSD_V4_2_INTER_SSC ?)
> >
> >will do. I did tested with (!CONFIG_NFSD_V4_2_INTER_SSC) but did not hit
> >this case.
> 
> I need to qualify this, the copy_file_range syscall did not return
> ESTALE in the test.
> 
> >Because with this patch, when CONFIG_NFSD_V4_2_INTER_SSC is not
> >defined the new nfs42_ssc_open returns ERR_PTR(-EIO), instead of NULL in
> >the old code, which causes the NFS client to switch over to the split
> >copying (read src and write to dst).
> 
> This is not the reason why the client switches to generic_copy_file_range.
> 
> >Returning NULL in the old nfs42_ssc_open is not correct, it allows
> >the copy
> >operation to proceed and hits the NFS4ERR_STALE case in the COPY
> >operation.
> 
> I retested with (!CONFIG_NFSD_V4_2_INTER_SSC) and saw NFS4ERR_STALE
> returned for the PUTFH of the SRC in the COPY compound. However on the
> client nfs42_proc_copy (with commit 7e350197a1c10) replaced the ESTALE
> with EOPNOTSUPP causing nfs4_copy_file_range to use generic_copy_file_range
> to do the copy.
> 
> The ESTALE error is only returned by copy_file_range if the client
> does not have commit 7e350197a1c10. So I think there is no need to
> make any change on the source server for the NFS4ERR_STALE error.

I don't believe NFS4ERR_STALE is the correct error for the server to
return.  It's nice that the client is able to do the right thing despite
the server returning the wrong error, but we should still try to get
this right on the server.

--b.