From: Chuck Lever <cel@kernel.org>
To: NeilBrown <neil@brown.name>
Cc: Chuck Lever <chuck.lever@oracle.com>,
Jeff Layton <jlayton@kernel.org>,
Olga Kornievskaia <okorniev@redhat.com>,
Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
linux-nfs@vger.kernel.org
Subject: Re: [PATCH v6 04/14] nfsd: allow unrecognisable filehandle for foreign servers in COPY
Date: Sun, 23 Nov 2025 11:43:13 -0500 [thread overview]
Message-ID: <aSM5oSoiw_6wDU_K@morisot.1015granger.net> (raw)
In-Reply-To: <aSIoEqzuT6seYiq0@morisot.1015granger.net>
On Sat, Nov 22, 2025 at 04:16:02PM -0500, Chuck Lever wrote:
> On Sat, Nov 22, 2025 at 11:47:02AM +1100, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > RFC-7862 acknowledges that a filehandle provided as the source of an
> > inter-server copy might result in NFS4ERR_STALE when given to PUTFH, and
> > gives guidance on how this error can be ignored (section 15.2.3).
> >
> > NFS4ERR_BADHANDLE is also a possible error in this circumstance if the
> > foreign server is running a different implementation of NFS than the
> > current one.
>
> The RFC uses the terms "source" and "destination" server, fwiw.
>
> It would be interesting to see if nfserr_badhandle can be triggered
> during an NFSD <-> NFSD copy.
>
>
> > This appears to be a simple omission in the RFC.
>
> Perhaps. It might also be the result of the RFC authors giving
> implementers flexibility to innovate.
>
> I would like to consult with the WG and possibly file an errata, or
> add this observation to the "NFSv4.2 COPY implementation experience"
> document I'm helping Olga with:
>
> https://datatracker.ietf.org/doc/draft-cel-nfsv4-copy-implementation-experience/
>
> They might want to consider NFS4ERR_MOVED as well.
>
>
> > There can be no harm in delaying a BADHANDLE error in the same situation
> > where we already delay STALE errors, and no harm in sending a locally
> > "bad" handle to a foreign server to request a COPY.
>
> These are plausible claims. But IMO, they need firmer rationale.
My comments aren't terribly clear about this, but I agree that PUTFH
handling NFS4ERR_BADHANDLE is a concern when there is a subsequent
COPY operation in the COMPOUND.
I think we can't rely on the letter of RFC 7862, as you pointed out.
However, the spirit of RFC 7862 is that PUTFH mustn't fail if a
client presents a foreign file handle via a PUTFH that precedes a
COPY operation. The lack of BCP14 language specific to other status
codes shouldn't stop NFSD from handling other status codes as it
needs to (in fact, the absence of such language affords us the
opportunity of doing exactly that).
The reason for this potential PUTFH failure is an artifact of NFSD's
PUTFH implementation, which unconditionally invokes fh_verify.
I'm wondering if PUTFH needs to check for /specific/ status codes
from fh_verify before looking ahead in the incoming COMPOUND, or if
it ought to look ahead on /any/ failure of fh_verify.
And, is this patch a fix that needs to be backported to LTS kernels?
If so, then perhaps it needs to go before the previous patch.
(It seems like NFSv4.2 could have added a PUT_FOREIGN_FH operation
to avoid all this nonsense).
(I'll also note that now that our MAX_OPS_PER_COMPOUND has increased
and might increase again, moving the COPY look ahead checking out of
nfsd4_proc_compound is a huge bonus for handling large COMPOUNDs).
> > So extend the test in nfsd4_putfh to also check for nfserr_badhandle.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> > fs/nfsd/nfs4proc.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 112e62b6b9c6..ae34b816371c 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -693,7 +693,8 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > putfh->pf_fhlen);
> > ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
> > #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> > - if (ret == nfserr_stale && inter_copy_offload_enable) {
> > + if ((ret == nfserr_badhandle || ret == nfserr_stale) &&
> > + inter_copy_offload_enable) {
> > struct nfsd4_compoundargs *args = rqstp->rq_argp;
> > struct nfsd4_compoundres *resp = rqstp->rq_resp;
> >
> > @@ -713,7 +714,11 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > * NOT return NFS4ERR_STALE for either
> > * operation.
> > * We limit this to when there is a COPY
> > - * in the COMPOUND.
> > + * in the COMPOUND, and extend it to
> > + * also ignore NFS4ERR_BADHANDLE despite the
> > + * RFC not requiring this. If the remote
> > + * server is running a different NFS implementation,
> > + * NFS4ERR_BADHANDLE is a likely error.
> > */
> > ret = 0;
> > }
> > --
> > 2.50.0.107.gf914562f5916.dirty
> >
> >
>
> --
> Chuck Lever
>
--
Chuck Lever
next prev parent reply other threads:[~2025-11-23 16:43 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-22 0:46 [PATCH v7 00/14] nfsd: assorted cleanups involving v4 special stateids NeilBrown
2025-11-22 0:46 ` [PATCH v6 01/14] nfsd: rename ALLOWED_WITHOUT_FH to ALLOWED_WITHOUT_LOCAL_FH and revise use NeilBrown
2025-11-22 20:40 ` Chuck Lever
2025-11-22 0:47 ` [PATCH v6 02/14] nfsd: discard NFSD4_FH_FOREIGN NeilBrown
2025-11-22 0:47 ` [PATCH v6 03/14] nfsd: simplify foreign-filehandle handling to better match RFC-7862 NeilBrown
2025-11-22 0:47 ` [PATCH v6 04/14] nfsd: allow unrecognisable filehandle for foreign servers in COPY NeilBrown
2025-11-22 21:16 ` Chuck Lever
2025-11-23 16:43 ` Chuck Lever [this message]
2025-11-27 0:55 ` NeilBrown
2026-01-23 17:03 ` Chuck Lever
2025-11-22 0:47 ` [PATCH v6 05/14] nfsd: report correct error for attempt to use foreign filehandle NeilBrown
2025-11-22 0:47 ` [PATCH v6 06/14] nfsd: drop explicit tests for special stateids which would be invalid NeilBrown
2025-11-22 0:47 ` [PATCH v6 07/14] nfsd: revise names of special stateid, and predicate functions NeilBrown
2025-11-22 0:47 ` [PATCH v6 08/14] nfsd: pass parent_fh explicitly to nfsd4_process_open2() NeilBrown
2025-11-22 0:47 ` [PATCH v6 09/14] nfsd: revert nfsd4: delay setting current_fh in open NeilBrown
2025-11-22 0:47 ` [PATCH v6 10/14] nfsd: simplify clearing of current-state-id NeilBrown
2025-11-22 0:47 ` [PATCH v6 11/14] nfsd: simplify use of the current stateid NeilBrown
2025-11-22 0:47 ` [PATCH v6 12/14] nfsd: simplify saving " NeilBrown
2025-11-22 0:47 ` [PATCH v6 13/14] nfsd: discard current_stateid.h NeilBrown
2025-11-22 0:47 ` [PATCH v6 14/14] nfsd: conditionally clear seqid when current_stateid is used NeilBrown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aSM5oSoiw_6wDU_K@morisot.1015granger.net \
--to=cel@kernel.org \
--cc=Dai.Ngo@oracle.com \
--cc=chuck.lever@oracle.com \
--cc=jlayton@kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=neil@brown.name \
--cc=okorniev@redhat.com \
--cc=tom@talpey.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox