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

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

On 11/10/20 1:07 PM, Olga Kornievskaia wrote:
> On Tue, Nov 10, 2020 at 3:14 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>> 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.

Hi Olga,

> Hi Bruce,
>
> ERR_STALE is the appropriate error to be returned by the server that
> gets a COPY compound when it doesn't support COPY. Since server can't
> understand the filehandle so it can't process it so we can't get to
> processing COPY opcode where the server could have returned
> EOPNOTSUPP. Thus a client side patch is needed and the server is doing
> everything it can in the situation.
>
> I'm confused about the title of this patch. I thought what it does is
> removes NFSD dependency on the NFS and instead loads the needed
> function dynamically.

Yes, the title of the patch is misleading. The main goal of the patch
is to add the ops table in nfs_common for knfsd to access NFS client
modules without calling these functions directly. This also prevents
build errors in config such as when NFSD=y and NFS_FS=m.

-Dai

>
> Honestly, I don't understand why that allows removal of the NFS_FS
> from the dependencies I don't understand. nfs4_ssc_open calls nfs
> client functions that are built when NFS_FS is compiled but I'm
> assuming will not be otherwise.
>
>
>> --b.