| Message ID | 1647051215-2873-5-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 11, 2022, at 9:13 PM, Dai Ngo <dai.ngo@oracle.com> wrote: > > Update nfsd_breaker_owns_lease() to handle delegation conflict > with courtesy clients. If conflict was caused by courtesy client > then discard the courtesy client by setting CLIENT_EXPIRED and > return conflict resolved. Client with CLIENT_EXPIRED is expired > by the laundromat. > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > --- > fs/nfsd/nfs4state.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 583ac807e98d..2beb0972de88 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -4713,6 +4713,28 @@ nfsd_break_deleg_cb(struct file_lock *fl) > return ret; > } > > +static bool > +nfs4_check_and_expire_courtesy_client(struct nfs4_client *clp) > +{ > + /* > + * need to sync with courtesy client trying to reconnect using > + * the cl_cs_lock, nn->client_lock can not be used since this > + * function is called with the fl_lck held. > + */ > + spin_lock(&clp->cl_cs_lock); > + if (test_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags)) { > + spin_unlock(&clp->cl_cs_lock); > + return true; > + } > + if (test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags)) { > + set_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags); > + spin_unlock(&clp->cl_cs_lock); > + return true; > + } > + spin_unlock(&clp->cl_cs_lock); > + return false; > +} > + > /** > * nfsd_breaker_owns_lease - Check if lease conflict was resolved > * @fl: Lock state to check > @@ -4727,6 +4749,10 @@ static bool nfsd_breaker_owns_lease(struct file_lock *fl) > struct svc_rqst *rqst; > struct nfs4_client *clp; > > + clp = dl->dl_stid.sc_client; > + if (nfs4_check_and_expire_courtesy_client(clp)) Since you'll need to fix the kernel robot issue in 1/11, when you repost, can you also just pass dl->dl_stid.sc_client directly to nfs4_check_and_expire_courtesy_client() here? > + return true; > + > if (!i_am_nfsd()) > return false; > rqst = kthread_data(current); > -- > 2.9.5 > -- Chuck Lever
On Sun, Mar 13, 2022 at 04:04:21PM +0000, Chuck Lever III wrote: > > > > On Mar 11, 2022, at 9:13 PM, Dai Ngo <dai.ngo@oracle.com> wrote: > > > > Update nfsd_breaker_owns_lease() to handle delegation conflict > > with courtesy clients. If conflict was caused by courtesy client > > then discard the courtesy client by setting CLIENT_EXPIRED and > > return conflict resolved. Client with CLIENT_EXPIRED is expired > > by the laundromat. > > > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > > --- > > fs/nfsd/nfs4state.c | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 583ac807e98d..2beb0972de88 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -4713,6 +4713,28 @@ nfsd_break_deleg_cb(struct file_lock *fl) > > return ret; > > } > > > > +static bool > > +nfs4_check_and_expire_courtesy_client(struct nfs4_client *clp) > > +{ > > + /* > > + * need to sync with courtesy client trying to reconnect using > > + * the cl_cs_lock, nn->client_lock can not be used since this > > + * function is called with the fl_lck held. > > + */ > > + spin_lock(&clp->cl_cs_lock); > > + if (test_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags)) { > > + spin_unlock(&clp->cl_cs_lock); > > + return true; > > + } > > + if (test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags)) { > > + set_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags); > > + spin_unlock(&clp->cl_cs_lock); > > + return true; > > + } > > + spin_unlock(&clp->cl_cs_lock); Same comment as on the previous patch. ALso, this is the second time we've used this logic, please define a static bool expire_client_if_courtesy(struct nfs4_client *clp) and call it from both places. > > + return false; > > +} > > + > > /** > > * nfsd_breaker_owns_lease - Check if lease conflict was resolved > > * @fl: Lock state to check > > @@ -4727,6 +4749,10 @@ static bool nfsd_breaker_owns_lease(struct file_lock *fl) > > struct svc_rqst *rqst; > > struct nfs4_client *clp; > > > > + clp = dl->dl_stid.sc_client; > > + if (nfs4_check_and_expire_courtesy_client(clp)) > > Since you'll need to fix the kernel robot issue in 1/11, when you > repost, can you also just pass dl->dl_stid.sc_client directly to > nfs4_check_and_expire_courtesy_client() here? Agreed. This client is quite different from the one we look up from the task a little later, and I'd just rather not have both represented by the same variable. --b.
On 3/15/22 8:13 AM, Bruce Fields wrote: > On Sun, Mar 13, 2022 at 04:04:21PM +0000, Chuck Lever III wrote: >> >>> On Mar 11, 2022, at 9:13 PM, Dai Ngo <dai.ngo@oracle.com> wrote: >>> >>> Update nfsd_breaker_owns_lease() to handle delegation conflict >>> with courtesy clients. If conflict was caused by courtesy client >>> then discard the courtesy client by setting CLIENT_EXPIRED and >>> return conflict resolved. Client with CLIENT_EXPIRED is expired >>> by the laundromat. >>> >>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >>> --- >>> fs/nfsd/nfs4state.c | 26 ++++++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>> index 583ac807e98d..2beb0972de88 100644 >>> --- a/fs/nfsd/nfs4state.c >>> +++ b/fs/nfsd/nfs4state.c >>> @@ -4713,6 +4713,28 @@ nfsd_break_deleg_cb(struct file_lock *fl) >>> return ret; >>> } >>> >>> +static bool >>> +nfs4_check_and_expire_courtesy_client(struct nfs4_client *clp) >>> +{ >>> + /* >>> + * need to sync with courtesy client trying to reconnect using >>> + * the cl_cs_lock, nn->client_lock can not be used since this >>> + * function is called with the fl_lck held. >>> + */ >>> + spin_lock(&clp->cl_cs_lock); >>> + if (test_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags)) { >>> + spin_unlock(&clp->cl_cs_lock); >>> + return true; >>> + } >>> + if (test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags)) { >>> + set_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags); >>> + spin_unlock(&clp->cl_cs_lock); >>> + return true; >>> + } >>> + spin_unlock(&clp->cl_cs_lock); > Same comment as on the previous patch. ALso, this is the second time > we've used this logic, please define a > > static bool expire_client_if_courtesy(struct nfs4_client *clp) > > and call it from both places. I will replace the similar code in nfsd4_lm_lock_expired with a call to nfs4_check_and_expire_courtesy_client. > >>> + return false; >>> +} >>> + >>> /** >>> * nfsd_breaker_owns_lease - Check if lease conflict was resolved >>> * @fl: Lock state to check >>> @@ -4727,6 +4749,10 @@ static bool nfsd_breaker_owns_lease(struct file_lock *fl) >>> struct svc_rqst *rqst; >>> struct nfs4_client *clp; >>> >>> + clp = dl->dl_stid.sc_client; >>> + if (nfs4_check_and_expire_courtesy_client(clp)) >> Since you'll need to fix the kernel robot issue in 1/11, when you >> repost, can you also just pass dl->dl_stid.sc_client directly to >> nfs4_check_and_expire_courtesy_client() here? > Agreed. > > This client is quite different from the one we look up from the task a > little later, and I'd just rather not have both represented by the same > variable. fix in v17. -Dai > > --b.
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 583ac807e98d..2beb0972de88 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -4713,6 +4713,28 @@ nfsd_break_deleg_cb(struct file_lock *fl) return ret; } +static bool +nfs4_check_and_expire_courtesy_client(struct nfs4_client *clp) +{ + /* + * need to sync with courtesy client trying to reconnect using + * the cl_cs_lock, nn->client_lock can not be used since this + * function is called with the fl_lck held. + */ + spin_lock(&clp->cl_cs_lock); + if (test_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags)) { + spin_unlock(&clp->cl_cs_lock); + return true; + } + if (test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags)) { + set_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags); + spin_unlock(&clp->cl_cs_lock); + return true; + } + spin_unlock(&clp->cl_cs_lock); + return false; +} + /** * nfsd_breaker_owns_lease - Check if lease conflict was resolved * @fl: Lock state to check @@ -4727,6 +4749,10 @@ static bool nfsd_breaker_owns_lease(struct file_lock *fl) struct svc_rqst *rqst; struct nfs4_client *clp; + clp = dl->dl_stid.sc_client; + if (nfs4_check_and_expire_courtesy_client(clp)) + return true; + if (!i_am_nfsd()) return false; rqst = kthread_data(current);
Update nfsd_breaker_owns_lease() to handle delegation conflict with courtesy clients. If conflict was caused by courtesy client then discard the courtesy client by setting CLIENT_EXPIRED and return conflict resolved. Client with CLIENT_EXPIRED is expired by the laundromat. Signed-off-by: Dai Ngo <dai.ngo@oracle.com> --- fs/nfsd/nfs4state.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)