Re: [PATCH v4 1/1] NFSv4.2: Fix NFS4ERR_STALE error when doing inter server copy

[Dai Ngo <dai.ngo@oracle.com>]

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.

-Dai

>
>
> -Dai
>
>>
>> --b.