On 11/10/20 2:21 PM, J. Bruce Fields wrote:
On Tue, Nov 10, 2020 at 05:08:59PM -0500, Olga Kornievskaia wrote:
On Tue, Nov 10, 2020 at 4:52 PM J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
On Tue, Nov 10, 2020 at 04:07:41PM -0500, Olga Kornievskaia wrote:
On Tue, Nov 10, 2020 at 3:14 PM J. Bruce Fields <bfields@xxxxxxxxxxxx> 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 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.
The case we're discussing is the case where we support COPY but not
server-to-server copy.
My point is still the same, that's an appropriate error for when
server-to-server copy is not supported.
Uh, OK, if it backs up and returns it to the PUTFH, I guess?
Was it really the intention of nfsd4_do_async_copy() that it return
STALE in the case NFS42_ssc_open() returns NULL? That's pretty
confusing.
In this scenario, the COPY compound fails at the PUTFH op and
NFS4ERR_NOTSUPP is not a valid error code for PUTFH, NFS4ERR_STALE is.
From section 15.2 of RFC 8881:
| PUTFH | NFS4ERR_BADHANDLE, NFS4ERR_BADXDR, |
| | NFS4ERR_DEADSESSION, NFS4ERR_DELAY, |
| | NFS4ERR_MOVED, |
| | NFS4ERR_OP_NOT_IN_SESSION, |
| | NFS4ERR_REP_TOO_BIG, |
| | NFS4ERR_REP_TOO_BIG_TO_CACHE, |
| | NFS4ERR_REQ_TOO_BIG, |
| | NFS4ERR_RETRY_UNCACHED_REP, |
| | NFS4ERR_SERVERFAULT, NFS4ERR_STALE, |
| | NFS4ERR_TOO_MANY_OPS, NFS4ERR_WRONGSEC |
Regarding fh_verify returns NFS4ERR_STALE, I think the code works as the spec
describes in 15.23 of RFC 7862:
If the request is for an inter-server copy, the source-fh is a
filehandle from the source server and the COMPOUND procedure is being
executed on the destination server. In this case, the source-fh is a
foreign filehandle on the server receiving the COPY request. If
either PUTFH or SAVEFH checked the validity of the filehandle, the
operation would likely fail and return NFS4ERR_STALE.
-Dai
--b.
--b.
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.
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.
