| Message ID | 1646440633-3542-8-git-send-email-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: Initial implementation of NFSv4 Courteous Server | expand |
> On Mar 4, 2022, at 7:37 PM, Dai Ngo <dai.ngo@oracle.com> wrote: > > Update find_in_sessionid_hashtbl to: > . skip client with CLIENT_EXPIRED flag; discarded courtesy client. > . if courtesy client was found then clear CLIENT_COURTESY and > set CLIENT_RECONNECTED so callers can take appropriate action. > > Update nfsd4_sequence and nfsd4_bind_conn_to_session to create client > record for client with CLIENT_RECONNECTED set. > > Update nfsd4_destroy_session to discard client with CLIENT_RECONNECTED > set. > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > --- > fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index f42d72a8f5ca..34a59c6f446c 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1963,13 +1963,22 @@ find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid, struct net *net, > { > struct nfsd4_session *session; > __be32 status = nfserr_badsession; > + struct nfs4_client *clp; > > session = __find_in_sessionid_hashtbl(sessionid, net); > if (!session) > goto out; > + clp = session->se_client; > + if (clp && nfs4_is_courtesy_client_expired(clp)) { > + session = NULL; > + goto out; > + } > status = nfsd4_get_session_locked(session); > - if (status) > + if (status) { > session = NULL; > + if (clp && test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags)) > + nfsd4_discard_courtesy_clnt(clp); > + } Here and above: I'm not seeing how @clp can be NULL, but I'm kind of new to fs/nfsd/nfs4state.c. -- Chuck Lever
On 3/9/22 2:42 PM, Chuck Lever III wrote: > >> On Mar 4, 2022, at 7:37 PM, Dai Ngo <dai.ngo@oracle.com> wrote: >> >> Update find_in_sessionid_hashtbl to: >> . skip client with CLIENT_EXPIRED flag; discarded courtesy client. >> . if courtesy client was found then clear CLIENT_COURTESY and >> set CLIENT_RECONNECTED so callers can take appropriate action. >> >> Update nfsd4_sequence and nfsd4_bind_conn_to_session to create client >> record for client with CLIENT_RECONNECTED set. >> >> Update nfsd4_destroy_session to discard client with CLIENT_RECONNECTED >> set. >> >> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >> --- >> fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++++-- >> 1 file changed, 32 insertions(+), 2 deletions(-) >> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index f42d72a8f5ca..34a59c6f446c 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -1963,13 +1963,22 @@ find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid, struct net *net, >> { >> struct nfsd4_session *session; >> __be32 status = nfserr_badsession; >> + struct nfs4_client *clp; >> >> session = __find_in_sessionid_hashtbl(sessionid, net); >> if (!session) >> goto out; >> + clp = session->se_client; >> + if (clp && nfs4_is_courtesy_client_expired(clp)) { >> + session = NULL; >> + goto out; >> + } >> status = nfsd4_get_session_locked(session); >> - if (status) >> + if (status) { >> session = NULL; >> + if (clp && test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags)) >> + nfsd4_discard_courtesy_clnt(clp); >> + } > Here and above: I'm not seeing how @clp can be NULL, but I'm kind > of new to fs/nfsd/nfs4state.c. it seems like @clp can not be NULL since existing code does not check for it. I will remove it to avoid any confusion. Can this be done a a separate clean up patch? Thanks, -Dai > > > -- > Chuck Lever > > >
> On Mar 9, 2022, at 10:12 PM, Dai Ngo <dai.ngo@oracle.com> wrote: > > >> On 3/9/22 2:42 PM, Chuck Lever III wrote: >> >>>> On Mar 4, 2022, at 7:37 PM, Dai Ngo <dai.ngo@oracle.com> wrote: >>> >>> Update find_in_sessionid_hashtbl to: >>> . skip client with CLIENT_EXPIRED flag; discarded courtesy client. >>> . if courtesy client was found then clear CLIENT_COURTESY and >>> set CLIENT_RECONNECTED so callers can take appropriate action. >>> >>> Update nfsd4_sequence and nfsd4_bind_conn_to_session to create client >>> record for client with CLIENT_RECONNECTED set. >>> >>> Update nfsd4_destroy_session to discard client with CLIENT_RECONNECTED >>> set. >>> >>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >>> --- >>> fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++++-- >>> 1 file changed, 32 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>> index f42d72a8f5ca..34a59c6f446c 100644 >>> --- a/fs/nfsd/nfs4state.c >>> +++ b/fs/nfsd/nfs4state.c >>> @@ -1963,13 +1963,22 @@ find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid, struct net *net, >>> { >>> struct nfsd4_session *session; >>> __be32 status = nfserr_badsession; >>> + struct nfs4_client *clp; >>> >>> session = __find_in_sessionid_hashtbl(sessionid, net); >>> if (!session) >>> goto out; >>> + clp = session->se_client; >>> + if (clp && nfs4_is_courtesy_client_expired(clp)) { >>> + session = NULL; >>> + goto out; >>> + } >>> status = nfsd4_get_session_locked(session); >>> - if (status) >>> + if (status) { >>> session = NULL; >>> + if (clp && test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags)) By the way, I don’t understand why this checks CLIENT_COURTESY to see if the clp should be discarded. Shouldn’t it check CLIENT_RECONNECTED like the other sites? >>> + nfsd4_discard_courtesy_clnt(clp); >>> + } >> Here and above: I'm not seeing how @clp can be NULL, but I'm kind >> of new to fs/nfsd/nfs4state.c. > > it seems like @clp can not be NULL since existing code does not > check for it. I will remove it to avoid any confusion. Can this > be done as a separate clean up patch? I don’t think this series is going to make v5.18. We can keep working on improving each of the patches. And I would rather ensure that the series is properly bisectable. I don’t think we’re at a point where the patches are immutable. > Thanks, > -Dai > >> >> >> -- >> Chuck Lever >> >> >>
On 3/9/22 7:33 PM, Chuck Lever III wrote: >> On Mar 9, 2022, at 10:12 PM, Dai Ngo <dai.ngo@oracle.com> wrote: >> >> >>> On 3/9/22 2:42 PM, Chuck Lever III wrote: >>> >>>>> On Mar 4, 2022, at 7:37 PM, Dai Ngo <dai.ngo@oracle.com> wrote: >>>> Update find_in_sessionid_hashtbl to: >>>> . skip client with CLIENT_EXPIRED flag; discarded courtesy client. >>>> . if courtesy client was found then clear CLIENT_COURTESY and >>>> set CLIENT_RECONNECTED so callers can take appropriate action. >>>> >>>> Update nfsd4_sequence and nfsd4_bind_conn_to_session to create client >>>> record for client with CLIENT_RECONNECTED set. >>>> >>>> Update nfsd4_destroy_session to discard client with CLIENT_RECONNECTED >>>> set. >>>> >>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >>>> --- >>>> fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 32 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>>> index f42d72a8f5ca..34a59c6f446c 100644 >>>> --- a/fs/nfsd/nfs4state.c >>>> +++ b/fs/nfsd/nfs4state.c >>>> @@ -1963,13 +1963,22 @@ find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid, struct net *net, >>>> { >>>> struct nfsd4_session *session; >>>> __be32 status = nfserr_badsession; >>>> + struct nfs4_client *clp; >>>> >>>> session = __find_in_sessionid_hashtbl(sessionid, net); >>>> if (!session) >>>> goto out; >>>> + clp = session->se_client; >>>> + if (clp && nfs4_is_courtesy_client_expired(clp)) { >>>> + session = NULL; >>>> + goto out; >>>> + } >>>> status = nfsd4_get_session_locked(session); >>>> - if (status) >>>> + if (status) { >>>> session = NULL; >>>> + if (clp && test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags)) > By the way, I don’t understand why this checks CLIENT_COURTESY to see if the clp should be discarded. Shouldn’t it check CLIENT_RECONNECTED like the other sites? Thanks, this should check for CLIENT_RECONNECTED as described in the commit message. > > >>>> + nfsd4_discard_courtesy_clnt(clp); >>>> + } >>> Here and above: I'm not seeing how @clp can be NULL, but I'm kind >>> of new to fs/nfsd/nfs4state.c. >> it seems like @clp can not be NULL since existing code does not >> check for it. I will remove it to avoid any confusion. Can this >> be done as a separate clean up patch? > I don’t think this series is going to make v5.18. We can keep working on improving each of the patches. And I would rather ensure that the series is properly bisectable. I don’t think we’re at a point where the patches are immutable. okay, I will submit v16 to address review comments. -Dai
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index f42d72a8f5ca..34a59c6f446c 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1963,13 +1963,22 @@ find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid, struct net *net, { struct nfsd4_session *session; __be32 status = nfserr_badsession; + struct nfs4_client *clp; session = __find_in_sessionid_hashtbl(sessionid, net); if (!session) goto out; + clp = session->se_client; + if (clp && nfs4_is_courtesy_client_expired(clp)) { + session = NULL; + goto out; + } status = nfsd4_get_session_locked(session); - if (status) + if (status) { session = NULL; + if (clp && test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags)) + nfsd4_discard_courtesy_clnt(clp); + } out: *ret = status; return session; @@ -3671,6 +3680,7 @@ __be32 nfsd4_bind_conn_to_session(struct svc_rqst *rqstp, struct nfsd4_session *session; struct net *net = SVC_NET(rqstp); struct nfsd_net *nn = net_generic(net, nfsd_net_id); + struct nfs4_client *clp; if (!nfsd4_last_compound_op(rqstp)) return nfserr_not_only_op; @@ -3703,6 +3713,13 @@ __be32 nfsd4_bind_conn_to_session(struct svc_rqst *rqstp, nfsd4_init_conn(rqstp, conn, session); status = nfs_ok; out: + clp = session->se_client; + if (test_bit(NFSD4_CLIENT_RECONNECTED, &clp->cl_flags)) { + if (status == nfs_ok) + nfsd4_client_record_create(clp); + else + nfsd4_discard_courtesy_clnt(clp); + } nfsd4_put_session(session); out_no_session: return status; @@ -3725,6 +3742,7 @@ nfsd4_destroy_session(struct svc_rqst *r, struct nfsd4_compound_state *cstate, int ref_held_by_me = 0; struct net *net = SVC_NET(r); struct nfsd_net *nn = net_generic(net, nfsd_net_id); + struct nfs4_client *clp; status = nfserr_not_only_op; if (nfsd4_compound_in_session(cstate, sessionid)) { @@ -3737,6 +3755,12 @@ nfsd4_destroy_session(struct svc_rqst *r, struct nfsd4_compound_state *cstate, ses = find_in_sessionid_hashtbl(sessionid, net, &status); if (!ses) goto out_client_lock; + clp = ses->se_client; + if (test_bit(NFSD4_CLIENT_RECONNECTED, &clp->cl_flags)) { + status = nfserr_badsession; + nfsd4_discard_courtesy_clnt(clp); + goto out_put_session; + } status = nfserr_wrong_cred; if (!nfsd4_mach_creds_match(ses->se_client, r)) goto out_put_session; @@ -3841,7 +3865,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_compoundres *resp = rqstp->rq_resp; struct xdr_stream *xdr = resp->xdr; struct nfsd4_session *session; - struct nfs4_client *clp; + struct nfs4_client *clp = NULL; struct nfsd4_slot *slot; struct nfsd4_conn *conn; __be32 status; @@ -3951,6 +3975,12 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, if (conn) free_conn(conn); spin_unlock(&nn->client_lock); + if (clp && test_bit(NFSD4_CLIENT_RECONNECTED, &clp->cl_flags)) { + if (status == nfs_ok) + nfsd4_client_record_create(clp); + else + nfsd4_discard_courtesy_clnt(clp); + } return status; out_put_session: nfsd4_put_session_locked(session);
Update find_in_sessionid_hashtbl to: . skip client with CLIENT_EXPIRED flag; discarded courtesy client. . if courtesy client was found then clear CLIENT_COURTESY and set CLIENT_RECONNECTED so callers can take appropriate action. Update nfsd4_sequence and nfsd4_bind_conn_to_session to create client record for client with CLIENT_RECONNECTED set. Update nfsd4_destroy_session to discard client with CLIENT_RECONNECTED set. Signed-off-by: Dai Ngo <dai.ngo@oracle.com> --- fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-)