* [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() @ 2019-10-23 21:43 Trond Myklebust 2019-10-25 14:51 ` J. Bruce Fields 0 siblings, 1 reply; 12+ messages in thread From: Trond Myklebust @ 2019-10-23 21:43 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs When we're destroying the client lease, and we call nfsd4_shutdown_callback(), we must ensure that we do not return before all outstanding callbacks have terminated and have released their payloads. Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> --- v2 - Fix a leak of clp->cl_cb_inflight when running null probes fs/nfsd/nfs4callback.c | 97 +++++++++++++++++++++++++++++++++--------- fs/nfsd/state.h | 1 + 2 files changed, 79 insertions(+), 19 deletions(-) diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index 524111420b48..a3c9517bcc64 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -826,6 +826,31 @@ static int max_cb_time(struct net *net) return max(nn->nfsd4_lease/10, (time_t)1) * HZ; } +static struct workqueue_struct *callback_wq; + +static bool nfsd4_queue_cb(struct nfsd4_callback *cb) +{ + return queue_work(callback_wq, &cb->cb_work); +} + +static void nfsd41_cb_inflight_begin(struct nfs4_client *clp) +{ + atomic_inc(&clp->cl_cb_inflight); +} + +static void nfsd41_cb_inflight_end(struct nfs4_client *clp) +{ + + if (atomic_dec_and_test(&clp->cl_cb_inflight)) + wake_up_var(&clp->cl_cb_inflight); +} + +static void nfsd41_cb_inflight_wait_complete(struct nfs4_client *clp) +{ + wait_var_event(&clp->cl_cb_inflight, + !atomic_read(&clp->cl_cb_inflight)); +} + static const struct cred *get_backchannel_cred(struct nfs4_client *clp, struct rpc_clnt *client, struct nfsd4_session *ses) { if (clp->cl_minorversion == 0) { @@ -937,14 +962,21 @@ static void nfsd4_cb_probe_done(struct rpc_task *task, void *calldata) clp->cl_cb_state = NFSD4_CB_UP; } +static void nfsd4_cb_probe_release(void *calldata) +{ + struct nfs4_client *clp = container_of(calldata, struct nfs4_client, cl_cb_null); + + nfsd41_cb_inflight_end(clp); + +} + static const struct rpc_call_ops nfsd4_cb_probe_ops = { /* XXX: release method to ensure we set the cb channel down if * necessary on early failure? */ .rpc_call_done = nfsd4_cb_probe_done, + .rpc_release = nfsd4_cb_probe_release, }; -static struct workqueue_struct *callback_wq; - /* * Poke the callback thread to process any updates to the callback * parameters, and send a null probe. @@ -975,9 +1007,12 @@ void nfsd4_change_callback(struct nfs4_client *clp, struct nfs4_cb_conn *conn) * If the slot is available, then mark it busy. Otherwise, set the * thread for sleeping on the callback RPC wait queue. */ -static bool nfsd41_cb_get_slot(struct nfs4_client *clp, struct rpc_task *task) +static bool nfsd41_cb_get_slot(struct nfsd4_callback *cb, struct rpc_task *task) { - if (test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { + struct nfs4_client *clp = cb->cb_clp; + + if (!cb->cb_holds_slot && + test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { rpc_sleep_on(&clp->cl_cb_waitq, task, NULL); /* Race breaker */ if (test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { @@ -985,10 +1020,32 @@ static bool nfsd41_cb_get_slot(struct nfs4_client *clp, struct rpc_task *task) return false; } rpc_wake_up_queued_task(&clp->cl_cb_waitq, task); + cb->cb_holds_slot = true; } return true; } +static void nfsd41_cb_release_slot(struct nfsd4_callback *cb) +{ + struct nfs4_client *clp = cb->cb_clp; + + if (cb->cb_holds_slot) { + cb->cb_holds_slot = false; + clear_bit(0, &clp->cl_cb_slot_busy); + rpc_wake_up_next(&clp->cl_cb_waitq); + } +} + +static void nfsd41_destroy_cb(struct nfsd4_callback *cb) +{ + struct nfs4_client *clp = cb->cb_clp; + + nfsd41_cb_release_slot(cb); + if (cb->cb_ops && cb->cb_ops->release) + cb->cb_ops->release(cb); + nfsd41_cb_inflight_end(clp); +} + /* * TODO: cb_sequence should support referring call lists, cachethis, multiple * slots, and mark callback channel down on communication errors. @@ -1005,11 +1062,8 @@ static void nfsd4_cb_prepare(struct rpc_task *task, void *calldata) */ cb->cb_seq_status = 1; cb->cb_status = 0; - if (minorversion) { - if (!cb->cb_holds_slot && !nfsd41_cb_get_slot(clp, task)) - return; - cb->cb_holds_slot = true; - } + if (minorversion && !nfsd41_cb_get_slot(cb, task)) + return; rpc_call_start(task); } @@ -1076,9 +1130,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback cb->cb_seq_status); } - cb->cb_holds_slot = false; - clear_bit(0, &clp->cl_cb_slot_busy); - rpc_wake_up_next(&clp->cl_cb_waitq); + nfsd41_cb_release_slot(cb); dprintk("%s: freed slot, new seqid=%d\n", __func__, clp->cl_cb_session->se_cb_seq_nr); @@ -1091,8 +1143,10 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback ret = false; goto out; need_restart: - task->tk_status = 0; - cb->cb_need_restart = true; + if (!test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) { + task->tk_status = 0; + cb->cb_need_restart = true; + } return false; } @@ -1134,9 +1188,9 @@ static void nfsd4_cb_release(void *calldata) struct nfsd4_callback *cb = calldata; if (cb->cb_need_restart) - nfsd4_run_cb(cb); + nfsd4_queue_cb(cb); else - cb->cb_ops->release(cb); + nfsd41_destroy_cb(cb); } @@ -1170,6 +1224,7 @@ void nfsd4_shutdown_callback(struct nfs4_client *clp) */ nfsd4_run_cb(&clp->cl_cb_null); flush_workqueue(callback_wq); + nfsd41_cb_inflight_wait_complete(clp); } /* requires cl_lock: */ @@ -1255,8 +1310,7 @@ nfsd4_run_cb_work(struct work_struct *work) clnt = clp->cl_cb_client; if (!clnt) { /* Callback channel broken, or client killed; give up: */ - if (cb->cb_ops && cb->cb_ops->release) - cb->cb_ops->release(cb); + nfsd41_destroy_cb(cb); return; } @@ -1265,6 +1319,7 @@ nfsd4_run_cb_work(struct work_struct *work) */ if (!cb->cb_ops && clp->cl_minorversion) { clp->cl_cb_state = NFSD4_CB_UP; + nfsd41_destroy_cb(cb); return; } @@ -1290,5 +1345,9 @@ void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp, void nfsd4_run_cb(struct nfsd4_callback *cb) { - queue_work(callback_wq, &cb->cb_work); + struct nfs4_client *clp = cb->cb_clp; + + nfsd41_cb_inflight_begin(clp); + if (!nfsd4_queue_cb(cb)) + nfsd41_cb_inflight_end(clp); } diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h index 46f56afb6cb8..d61b83b9654c 100644 --- a/fs/nfsd/state.h +++ b/fs/nfsd/state.h @@ -367,6 +367,7 @@ struct nfs4_client { struct net *net; struct list_head async_copies; /* list of async copies */ spinlock_t async_lock; /* lock for async copies */ + atomic_t cl_cb_inflight; /* Outstanding callbacks */ }; /* struct nfs4_client_reset -- 2.21.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() 2019-10-23 21:43 [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() Trond Myklebust @ 2019-10-25 14:51 ` J. Bruce Fields 2019-10-25 14:55 ` Trond Myklebust 0 siblings, 1 reply; 12+ messages in thread From: J. Bruce Fields @ 2019-10-25 14:51 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs On Wed, Oct 23, 2019 at 05:43:18PM -0400, Trond Myklebust wrote: > When we're destroying the client lease, and we call > nfsd4_shutdown_callback(), we must ensure that we do not return > before all outstanding callbacks have terminated and have > released their payloads. This is great, thanks! We've seen what I'm fairly sure is the same bug from Red Hat users. I think my blind spot was an assumption that rpc tasks wouldn't outlive rpc_shutdown_client(). However, it's causing xfstests runs to hang, and I haven't worked out why yet. I'll spend some time on it this afternoon and let you know what I figure out. --b. > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > v2 - Fix a leak of clp->cl_cb_inflight when running null probes > > fs/nfsd/nfs4callback.c | 97 +++++++++++++++++++++++++++++++++--------- > fs/nfsd/state.h | 1 + > 2 files changed, 79 insertions(+), 19 deletions(-) > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index 524111420b48..a3c9517bcc64 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -826,6 +826,31 @@ static int max_cb_time(struct net *net) > return max(nn->nfsd4_lease/10, (time_t)1) * HZ; > } > > +static struct workqueue_struct *callback_wq; > + > +static bool nfsd4_queue_cb(struct nfsd4_callback *cb) > +{ > + return queue_work(callback_wq, &cb->cb_work); > +} > + > +static void nfsd41_cb_inflight_begin(struct nfs4_client *clp) > +{ > + atomic_inc(&clp->cl_cb_inflight); > +} > + > +static void nfsd41_cb_inflight_end(struct nfs4_client *clp) > +{ > + > + if (atomic_dec_and_test(&clp->cl_cb_inflight)) > + wake_up_var(&clp->cl_cb_inflight); > +} > + > +static void nfsd41_cb_inflight_wait_complete(struct nfs4_client *clp) > +{ > + wait_var_event(&clp->cl_cb_inflight, > + !atomic_read(&clp->cl_cb_inflight)); > +} > + > static const struct cred *get_backchannel_cred(struct nfs4_client *clp, struct rpc_clnt *client, struct nfsd4_session *ses) > { > if (clp->cl_minorversion == 0) { > @@ -937,14 +962,21 @@ static void nfsd4_cb_probe_done(struct rpc_task *task, void *calldata) > clp->cl_cb_state = NFSD4_CB_UP; > } > > +static void nfsd4_cb_probe_release(void *calldata) > +{ > + struct nfs4_client *clp = container_of(calldata, struct nfs4_client, cl_cb_null); > + > + nfsd41_cb_inflight_end(clp); > + > +} > + > static const struct rpc_call_ops nfsd4_cb_probe_ops = { > /* XXX: release method to ensure we set the cb channel down if > * necessary on early failure? */ > .rpc_call_done = nfsd4_cb_probe_done, > + .rpc_release = nfsd4_cb_probe_release, > }; > > -static struct workqueue_struct *callback_wq; > - > /* > * Poke the callback thread to process any updates to the callback > * parameters, and send a null probe. > @@ -975,9 +1007,12 @@ void nfsd4_change_callback(struct nfs4_client *clp, struct nfs4_cb_conn *conn) > * If the slot is available, then mark it busy. Otherwise, set the > * thread for sleeping on the callback RPC wait queue. > */ > -static bool nfsd41_cb_get_slot(struct nfs4_client *clp, struct rpc_task *task) > +static bool nfsd41_cb_get_slot(struct nfsd4_callback *cb, struct rpc_task *task) > { > - if (test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { > + struct nfs4_client *clp = cb->cb_clp; > + > + if (!cb->cb_holds_slot && > + test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { > rpc_sleep_on(&clp->cl_cb_waitq, task, NULL); > /* Race breaker */ > if (test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { > @@ -985,10 +1020,32 @@ static bool nfsd41_cb_get_slot(struct nfs4_client *clp, struct rpc_task *task) > return false; > } > rpc_wake_up_queued_task(&clp->cl_cb_waitq, task); > + cb->cb_holds_slot = true; > } > return true; > } > > +static void nfsd41_cb_release_slot(struct nfsd4_callback *cb) > +{ > + struct nfs4_client *clp = cb->cb_clp; > + > + if (cb->cb_holds_slot) { > + cb->cb_holds_slot = false; > + clear_bit(0, &clp->cl_cb_slot_busy); > + rpc_wake_up_next(&clp->cl_cb_waitq); > + } > +} > + > +static void nfsd41_destroy_cb(struct nfsd4_callback *cb) > +{ > + struct nfs4_client *clp = cb->cb_clp; > + > + nfsd41_cb_release_slot(cb); > + if (cb->cb_ops && cb->cb_ops->release) > + cb->cb_ops->release(cb); > + nfsd41_cb_inflight_end(clp); > +} > + > /* > * TODO: cb_sequence should support referring call lists, cachethis, multiple > * slots, and mark callback channel down on communication errors. > @@ -1005,11 +1062,8 @@ static void nfsd4_cb_prepare(struct rpc_task *task, void *calldata) > */ > cb->cb_seq_status = 1; > cb->cb_status = 0; > - if (minorversion) { > - if (!cb->cb_holds_slot && !nfsd41_cb_get_slot(clp, task)) > - return; > - cb->cb_holds_slot = true; > - } > + if (minorversion && !nfsd41_cb_get_slot(cb, task)) > + return; > rpc_call_start(task); > } > > @@ -1076,9 +1130,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > cb->cb_seq_status); > } > > - cb->cb_holds_slot = false; > - clear_bit(0, &clp->cl_cb_slot_busy); > - rpc_wake_up_next(&clp->cl_cb_waitq); > + nfsd41_cb_release_slot(cb); > dprintk("%s: freed slot, new seqid=%d\n", __func__, > clp->cl_cb_session->se_cb_seq_nr); > > @@ -1091,8 +1143,10 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > ret = false; > goto out; > need_restart: > - task->tk_status = 0; > - cb->cb_need_restart = true; > + if (!test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) { > + task->tk_status = 0; > + cb->cb_need_restart = true; > + } > return false; > } > > @@ -1134,9 +1188,9 @@ static void nfsd4_cb_release(void *calldata) > struct nfsd4_callback *cb = calldata; > > if (cb->cb_need_restart) > - nfsd4_run_cb(cb); > + nfsd4_queue_cb(cb); > else > - cb->cb_ops->release(cb); > + nfsd41_destroy_cb(cb); > > } > > @@ -1170,6 +1224,7 @@ void nfsd4_shutdown_callback(struct nfs4_client *clp) > */ > nfsd4_run_cb(&clp->cl_cb_null); > flush_workqueue(callback_wq); > + nfsd41_cb_inflight_wait_complete(clp); > } > > /* requires cl_lock: */ > @@ -1255,8 +1310,7 @@ nfsd4_run_cb_work(struct work_struct *work) > clnt = clp->cl_cb_client; > if (!clnt) { > /* Callback channel broken, or client killed; give up: */ > - if (cb->cb_ops && cb->cb_ops->release) > - cb->cb_ops->release(cb); > + nfsd41_destroy_cb(cb); > return; > } > > @@ -1265,6 +1319,7 @@ nfsd4_run_cb_work(struct work_struct *work) > */ > if (!cb->cb_ops && clp->cl_minorversion) { > clp->cl_cb_state = NFSD4_CB_UP; > + nfsd41_destroy_cb(cb); > return; > } > > @@ -1290,5 +1345,9 @@ void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp, > > void nfsd4_run_cb(struct nfsd4_callback *cb) > { > - queue_work(callback_wq, &cb->cb_work); > + struct nfs4_client *clp = cb->cb_clp; > + > + nfsd41_cb_inflight_begin(clp); > + if (!nfsd4_queue_cb(cb)) > + nfsd41_cb_inflight_end(clp); > } > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > index 46f56afb6cb8..d61b83b9654c 100644 > --- a/fs/nfsd/state.h > +++ b/fs/nfsd/state.h > @@ -367,6 +367,7 @@ struct nfs4_client { > struct net *net; > struct list_head async_copies; /* list of async copies */ > spinlock_t async_lock; /* lock for async copies */ > + atomic_t cl_cb_inflight; /* Outstanding callbacks */ > }; > > /* struct nfs4_client_reset > -- > 2.21.0 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() 2019-10-25 14:51 ` J. Bruce Fields @ 2019-10-25 14:55 ` Trond Myklebust 2019-10-25 15:21 ` J. Bruce Fields 0 siblings, 1 reply; 12+ messages in thread From: Trond Myklebust @ 2019-10-25 14:55 UTC (permalink / raw) To: bfields; +Cc: linux-nfs On Fri, 2019-10-25 at 10:51 -0400, J. Bruce Fields wrote: > On Wed, Oct 23, 2019 at 05:43:18PM -0400, Trond Myklebust wrote: > > When we're destroying the client lease, and we call > > nfsd4_shutdown_callback(), we must ensure that we do not return > > before all outstanding callbacks have terminated and have > > released their payloads. > > This is great, thanks! We've seen what I'm fairly sure is the same > bug > from Red Hat users. I think my blind spot was an assumption that > rpc tasks wouldn't outlive rpc_shutdown_client(). > > However, it's causing xfstests runs to hang, and I haven't worked out > why yet. > > I'll spend some time on it this afternoon and let you know what I > figure > out. > Is that happening with v2 or with v1? With v1 there is definitely a hang in __destroy_client() due to the refcount leak that I believe I fixed in v2. > --b. > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > v2 - Fix a leak of clp->cl_cb_inflight when running null probes > > > > fs/nfsd/nfs4callback.c | 97 +++++++++++++++++++++++++++++++++----- > > ---- > > fs/nfsd/state.h | 1 + > > 2 files changed, 79 insertions(+), 19 deletions(-) > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > index 524111420b48..a3c9517bcc64 100644 > > --- a/fs/nfsd/nfs4callback.c > > +++ b/fs/nfsd/nfs4callback.c > > @@ -826,6 +826,31 @@ static int max_cb_time(struct net *net) > > return max(nn->nfsd4_lease/10, (time_t)1) * HZ; > > } > > > > +static struct workqueue_struct *callback_wq; > > + > > +static bool nfsd4_queue_cb(struct nfsd4_callback *cb) > > +{ > > + return queue_work(callback_wq, &cb->cb_work); > > +} > > + > > +static void nfsd41_cb_inflight_begin(struct nfs4_client *clp) > > +{ > > + atomic_inc(&clp->cl_cb_inflight); > > +} > > + > > +static void nfsd41_cb_inflight_end(struct nfs4_client *clp) > > +{ > > + > > + if (atomic_dec_and_test(&clp->cl_cb_inflight)) > > + wake_up_var(&clp->cl_cb_inflight); > > +} > > + > > +static void nfsd41_cb_inflight_wait_complete(struct nfs4_client > > *clp) > > +{ > > + wait_var_event(&clp->cl_cb_inflight, > > + !atomic_read(&clp->cl_cb_inflight)); > > +} > > + > > static const struct cred *get_backchannel_cred(struct nfs4_client > > *clp, struct rpc_clnt *client, struct nfsd4_session *ses) > > { > > if (clp->cl_minorversion == 0) { > > @@ -937,14 +962,21 @@ static void nfsd4_cb_probe_done(struct > > rpc_task *task, void *calldata) > > clp->cl_cb_state = NFSD4_CB_UP; > > } > > > > +static void nfsd4_cb_probe_release(void *calldata) > > +{ > > + struct nfs4_client *clp = container_of(calldata, struct > > nfs4_client, cl_cb_null); > > + > > + nfsd41_cb_inflight_end(clp); > > + > > +} > > + > > static const struct rpc_call_ops nfsd4_cb_probe_ops = { > > /* XXX: release method to ensure we set the cb channel down if > > * necessary on early failure? */ > > .rpc_call_done = nfsd4_cb_probe_done, > > + .rpc_release = nfsd4_cb_probe_release, > > }; > > > > -static struct workqueue_struct *callback_wq; > > - > > /* > > * Poke the callback thread to process any updates to the callback > > * parameters, and send a null probe. > > @@ -975,9 +1007,12 @@ void nfsd4_change_callback(struct nfs4_client > > *clp, struct nfs4_cb_conn *conn) > > * If the slot is available, then mark it busy. Otherwise, set > > the > > * thread for sleeping on the callback RPC wait queue. > > */ > > -static bool nfsd41_cb_get_slot(struct nfs4_client *clp, struct > > rpc_task *task) > > +static bool nfsd41_cb_get_slot(struct nfsd4_callback *cb, struct > > rpc_task *task) > > { > > - if (test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { > > + struct nfs4_client *clp = cb->cb_clp; > > + > > + if (!cb->cb_holds_slot && > > + test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { > > rpc_sleep_on(&clp->cl_cb_waitq, task, NULL); > > /* Race breaker */ > > if (test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { > > @@ -985,10 +1020,32 @@ static bool nfsd41_cb_get_slot(struct > > nfs4_client *clp, struct rpc_task *task) > > return false; > > } > > rpc_wake_up_queued_task(&clp->cl_cb_waitq, task); > > + cb->cb_holds_slot = true; > > } > > return true; > > } > > > > +static void nfsd41_cb_release_slot(struct nfsd4_callback *cb) > > +{ > > + struct nfs4_client *clp = cb->cb_clp; > > + > > + if (cb->cb_holds_slot) { > > + cb->cb_holds_slot = false; > > + clear_bit(0, &clp->cl_cb_slot_busy); > > + rpc_wake_up_next(&clp->cl_cb_waitq); > > + } > > +} > > + > > +static void nfsd41_destroy_cb(struct nfsd4_callback *cb) > > +{ > > + struct nfs4_client *clp = cb->cb_clp; > > + > > + nfsd41_cb_release_slot(cb); > > + if (cb->cb_ops && cb->cb_ops->release) > > + cb->cb_ops->release(cb); > > + nfsd41_cb_inflight_end(clp); > > +} > > + > > /* > > * TODO: cb_sequence should support referring call lists, > > cachethis, multiple > > * slots, and mark callback channel down on communication errors. > > @@ -1005,11 +1062,8 @@ static void nfsd4_cb_prepare(struct rpc_task > > *task, void *calldata) > > */ > > cb->cb_seq_status = 1; > > cb->cb_status = 0; > > - if (minorversion) { > > - if (!cb->cb_holds_slot && !nfsd41_cb_get_slot(clp, > > task)) > > - return; > > - cb->cb_holds_slot = true; > > - } > > + if (minorversion && !nfsd41_cb_get_slot(cb, task)) > > + return; > > rpc_call_start(task); > > } > > > > @@ -1076,9 +1130,7 @@ static bool nfsd4_cb_sequence_done(struct > > rpc_task *task, struct nfsd4_callback > > cb->cb_seq_status); > > } > > > > - cb->cb_holds_slot = false; > > - clear_bit(0, &clp->cl_cb_slot_busy); > > - rpc_wake_up_next(&clp->cl_cb_waitq); > > + nfsd41_cb_release_slot(cb); > > dprintk("%s: freed slot, new seqid=%d\n", __func__, > > clp->cl_cb_session->se_cb_seq_nr); > > > > @@ -1091,8 +1143,10 @@ static bool nfsd4_cb_sequence_done(struct > > rpc_task *task, struct nfsd4_callback > > ret = false; > > goto out; > > need_restart: > > - task->tk_status = 0; > > - cb->cb_need_restart = true; > > + if (!test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) { > > + task->tk_status = 0; > > + cb->cb_need_restart = true; > > + } > > return false; > > } > > > > @@ -1134,9 +1188,9 @@ static void nfsd4_cb_release(void *calldata) > > struct nfsd4_callback *cb = calldata; > > > > if (cb->cb_need_restart) > > - nfsd4_run_cb(cb); > > + nfsd4_queue_cb(cb); > > else > > - cb->cb_ops->release(cb); > > + nfsd41_destroy_cb(cb); > > > > } > > > > @@ -1170,6 +1224,7 @@ void nfsd4_shutdown_callback(struct > > nfs4_client *clp) > > */ > > nfsd4_run_cb(&clp->cl_cb_null); > > flush_workqueue(callback_wq); > > + nfsd41_cb_inflight_wait_complete(clp); > > } > > > > /* requires cl_lock: */ > > @@ -1255,8 +1310,7 @@ nfsd4_run_cb_work(struct work_struct *work) > > clnt = clp->cl_cb_client; > > if (!clnt) { > > /* Callback channel broken, or client killed; give up: > > */ > > - if (cb->cb_ops && cb->cb_ops->release) > > - cb->cb_ops->release(cb); > > + nfsd41_destroy_cb(cb); > > return; > > } > > > > @@ -1265,6 +1319,7 @@ nfsd4_run_cb_work(struct work_struct *work) > > */ > > if (!cb->cb_ops && clp->cl_minorversion) { > > clp->cl_cb_state = NFSD4_CB_UP; > > + nfsd41_destroy_cb(cb); > > return; > > } > > > > @@ -1290,5 +1345,9 @@ void nfsd4_init_cb(struct nfsd4_callback *cb, > > struct nfs4_client *clp, > > > > void nfsd4_run_cb(struct nfsd4_callback *cb) > > { > > - queue_work(callback_wq, &cb->cb_work); > > + struct nfs4_client *clp = cb->cb_clp; > > + > > + nfsd41_cb_inflight_begin(clp); > > + if (!nfsd4_queue_cb(cb)) > > + nfsd41_cb_inflight_end(clp); > > } > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > > index 46f56afb6cb8..d61b83b9654c 100644 > > --- a/fs/nfsd/state.h > > +++ b/fs/nfsd/state.h > > @@ -367,6 +367,7 @@ struct nfs4_client { > > struct net *net; > > struct list_head async_copies; /* list of async copies */ > > spinlock_t async_lock; /* lock for async copies */ > > + atomic_t cl_cb_inflight; /* Outstanding callbacks */ > > }; > > > > /* struct nfs4_client_reset > > -- > > 2.21.0 > > -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() 2019-10-25 14:55 ` Trond Myklebust @ 2019-10-25 15:21 ` J. Bruce Fields 2019-10-25 15:33 ` J. Bruce Fields 0 siblings, 1 reply; 12+ messages in thread From: J. Bruce Fields @ 2019-10-25 15:21 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs On Fri, Oct 25, 2019 at 02:55:45PM +0000, Trond Myklebust wrote: > On Fri, 2019-10-25 at 10:51 -0400, J. Bruce Fields wrote: > > On Wed, Oct 23, 2019 at 05:43:18PM -0400, Trond Myklebust wrote: > > > When we're destroying the client lease, and we call > > > nfsd4_shutdown_callback(), we must ensure that we do not return > > > before all outstanding callbacks have terminated and have > > > released their payloads. > > > > This is great, thanks! We've seen what I'm fairly sure is the same > > bug > > from Red Hat users. I think my blind spot was an assumption that > > rpc tasks wouldn't outlive rpc_shutdown_client(). > > > > However, it's causing xfstests runs to hang, and I haven't worked out > > why yet. > > > > I'll spend some time on it this afternoon and let you know what I > > figure > > out. > > > > Is that happening with v2 or with v1? With v1 there is definitely a > hang in __destroy_client() due to the refcount leak that I believe I > fixed in v2. I thought I was running v2, let me double-check.... --b. > > > --b. > > > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > > --- > > > v2 - Fix a leak of clp->cl_cb_inflight when running null probes > > > > > > fs/nfsd/nfs4callback.c | 97 +++++++++++++++++++++++++++++++++----- > > > ---- > > > fs/nfsd/state.h | 1 + > > > 2 files changed, 79 insertions(+), 19 deletions(-) > > > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > > index 524111420b48..a3c9517bcc64 100644 > > > --- a/fs/nfsd/nfs4callback.c > > > +++ b/fs/nfsd/nfs4callback.c > > > @@ -826,6 +826,31 @@ static int max_cb_time(struct net *net) > > > return max(nn->nfsd4_lease/10, (time_t)1) * HZ; > > > } > > > > > > +static struct workqueue_struct *callback_wq; > > > + > > > +static bool nfsd4_queue_cb(struct nfsd4_callback *cb) > > > +{ > > > + return queue_work(callback_wq, &cb->cb_work); > > > +} > > > + > > > +static void nfsd41_cb_inflight_begin(struct nfs4_client *clp) > > > +{ > > > + atomic_inc(&clp->cl_cb_inflight); > > > +} > > > + > > > +static void nfsd41_cb_inflight_end(struct nfs4_client *clp) > > > +{ > > > + > > > + if (atomic_dec_and_test(&clp->cl_cb_inflight)) > > > + wake_up_var(&clp->cl_cb_inflight); > > > +} > > > + > > > +static void nfsd41_cb_inflight_wait_complete(struct nfs4_client > > > *clp) > > > +{ > > > + wait_var_event(&clp->cl_cb_inflight, > > > + !atomic_read(&clp->cl_cb_inflight)); > > > +} > > > + > > > static const struct cred *get_backchannel_cred(struct nfs4_client > > > *clp, struct rpc_clnt *client, struct nfsd4_session *ses) > > > { > > > if (clp->cl_minorversion == 0) { > > > @@ -937,14 +962,21 @@ static void nfsd4_cb_probe_done(struct > > > rpc_task *task, void *calldata) > > > clp->cl_cb_state = NFSD4_CB_UP; > > > } > > > > > > +static void nfsd4_cb_probe_release(void *calldata) > > > +{ > > > + struct nfs4_client *clp = container_of(calldata, struct > > > nfs4_client, cl_cb_null); > > > + > > > + nfsd41_cb_inflight_end(clp); > > > + > > > +} > > > + > > > static const struct rpc_call_ops nfsd4_cb_probe_ops = { > > > /* XXX: release method to ensure we set the cb channel down if > > > * necessary on early failure? */ > > > .rpc_call_done = nfsd4_cb_probe_done, > > > + .rpc_release = nfsd4_cb_probe_release, > > > }; > > > > > > -static struct workqueue_struct *callback_wq; > > > - > > > /* > > > * Poke the callback thread to process any updates to the callback > > > * parameters, and send a null probe. > > > @@ -975,9 +1007,12 @@ void nfsd4_change_callback(struct nfs4_client > > > *clp, struct nfs4_cb_conn *conn) > > > * If the slot is available, then mark it busy. Otherwise, set > > > the > > > * thread for sleeping on the callback RPC wait queue. > > > */ > > > -static bool nfsd41_cb_get_slot(struct nfs4_client *clp, struct > > > rpc_task *task) > > > +static bool nfsd41_cb_get_slot(struct nfsd4_callback *cb, struct > > > rpc_task *task) > > > { > > > - if (test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { > > > + struct nfs4_client *clp = cb->cb_clp; > > > + > > > + if (!cb->cb_holds_slot && > > > + test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { > > > rpc_sleep_on(&clp->cl_cb_waitq, task, NULL); > > > /* Race breaker */ > > > if (test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { > > > @@ -985,10 +1020,32 @@ static bool nfsd41_cb_get_slot(struct > > > nfs4_client *clp, struct rpc_task *task) > > > return false; > > > } > > > rpc_wake_up_queued_task(&clp->cl_cb_waitq, task); > > > + cb->cb_holds_slot = true; > > > } > > > return true; > > > } > > > > > > +static void nfsd41_cb_release_slot(struct nfsd4_callback *cb) > > > +{ > > > + struct nfs4_client *clp = cb->cb_clp; > > > + > > > + if (cb->cb_holds_slot) { > > > + cb->cb_holds_slot = false; > > > + clear_bit(0, &clp->cl_cb_slot_busy); > > > + rpc_wake_up_next(&clp->cl_cb_waitq); > > > + } > > > +} > > > + > > > +static void nfsd41_destroy_cb(struct nfsd4_callback *cb) > > > +{ > > > + struct nfs4_client *clp = cb->cb_clp; > > > + > > > + nfsd41_cb_release_slot(cb); > > > + if (cb->cb_ops && cb->cb_ops->release) > > > + cb->cb_ops->release(cb); > > > + nfsd41_cb_inflight_end(clp); > > > +} > > > + > > > /* > > > * TODO: cb_sequence should support referring call lists, > > > cachethis, multiple > > > * slots, and mark callback channel down on communication errors. > > > @@ -1005,11 +1062,8 @@ static void nfsd4_cb_prepare(struct rpc_task > > > *task, void *calldata) > > > */ > > > cb->cb_seq_status = 1; > > > cb->cb_status = 0; > > > - if (minorversion) { > > > - if (!cb->cb_holds_slot && !nfsd41_cb_get_slot(clp, > > > task)) > > > - return; > > > - cb->cb_holds_slot = true; > > > - } > > > + if (minorversion && !nfsd41_cb_get_slot(cb, task)) > > > + return; > > > rpc_call_start(task); > > > } > > > > > > @@ -1076,9 +1130,7 @@ static bool nfsd4_cb_sequence_done(struct > > > rpc_task *task, struct nfsd4_callback > > > cb->cb_seq_status); > > > } > > > > > > - cb->cb_holds_slot = false; > > > - clear_bit(0, &clp->cl_cb_slot_busy); > > > - rpc_wake_up_next(&clp->cl_cb_waitq); > > > + nfsd41_cb_release_slot(cb); > > > dprintk("%s: freed slot, new seqid=%d\n", __func__, > > > clp->cl_cb_session->se_cb_seq_nr); > > > > > > @@ -1091,8 +1143,10 @@ static bool nfsd4_cb_sequence_done(struct > > > rpc_task *task, struct nfsd4_callback > > > ret = false; > > > goto out; > > > need_restart: > > > - task->tk_status = 0; > > > - cb->cb_need_restart = true; > > > + if (!test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) { > > > + task->tk_status = 0; > > > + cb->cb_need_restart = true; > > > + } > > > return false; > > > } > > > > > > @@ -1134,9 +1188,9 @@ static void nfsd4_cb_release(void *calldata) > > > struct nfsd4_callback *cb = calldata; > > > > > > if (cb->cb_need_restart) > > > - nfsd4_run_cb(cb); > > > + nfsd4_queue_cb(cb); > > > else > > > - cb->cb_ops->release(cb); > > > + nfsd41_destroy_cb(cb); > > > > > > } > > > > > > @@ -1170,6 +1224,7 @@ void nfsd4_shutdown_callback(struct > > > nfs4_client *clp) > > > */ > > > nfsd4_run_cb(&clp->cl_cb_null); > > > flush_workqueue(callback_wq); > > > + nfsd41_cb_inflight_wait_complete(clp); > > > } > > > > > > /* requires cl_lock: */ > > > @@ -1255,8 +1310,7 @@ nfsd4_run_cb_work(struct work_struct *work) > > > clnt = clp->cl_cb_client; > > > if (!clnt) { > > > /* Callback channel broken, or client killed; give up: > > > */ > > > - if (cb->cb_ops && cb->cb_ops->release) > > > - cb->cb_ops->release(cb); > > > + nfsd41_destroy_cb(cb); > > > return; > > > } > > > > > > @@ -1265,6 +1319,7 @@ nfsd4_run_cb_work(struct work_struct *work) > > > */ > > > if (!cb->cb_ops && clp->cl_minorversion) { > > > clp->cl_cb_state = NFSD4_CB_UP; > > > + nfsd41_destroy_cb(cb); > > > return; > > > } > > > > > > @@ -1290,5 +1345,9 @@ void nfsd4_init_cb(struct nfsd4_callback *cb, > > > struct nfs4_client *clp, > > > > > > void nfsd4_run_cb(struct nfsd4_callback *cb) > > > { > > > - queue_work(callback_wq, &cb->cb_work); > > > + struct nfs4_client *clp = cb->cb_clp; > > > + > > > + nfsd41_cb_inflight_begin(clp); > > > + if (!nfsd4_queue_cb(cb)) > > > + nfsd41_cb_inflight_end(clp); > > > } > > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > > > index 46f56afb6cb8..d61b83b9654c 100644 > > > --- a/fs/nfsd/state.h > > > +++ b/fs/nfsd/state.h > > > @@ -367,6 +367,7 @@ struct nfs4_client { > > > struct net *net; > > > struct list_head async_copies; /* list of async copies */ > > > spinlock_t async_lock; /* lock for async copies */ > > > + atomic_t cl_cb_inflight; /* Outstanding callbacks */ > > > }; > > > > > > /* struct nfs4_client_reset > > > -- > > > 2.21.0 > > > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() 2019-10-25 15:21 ` J. Bruce Fields @ 2019-10-25 15:33 ` J. Bruce Fields 2019-10-29 21:47 ` J. Bruce Fields 0 siblings, 1 reply; 12+ messages in thread From: J. Bruce Fields @ 2019-10-25 15:33 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Trond Myklebust, linux-nfs On Fri, Oct 25, 2019 at 11:21:19AM -0400, J. Bruce Fields wrote: > On Fri, Oct 25, 2019 at 02:55:45PM +0000, Trond Myklebust wrote: > > On Fri, 2019-10-25 at 10:51 -0400, J. Bruce Fields wrote: > > > On Wed, Oct 23, 2019 at 05:43:18PM -0400, Trond Myklebust wrote: > > > > When we're destroying the client lease, and we call > > > > nfsd4_shutdown_callback(), we must ensure that we do not return > > > > before all outstanding callbacks have terminated and have > > > > released their payloads. > > > > > > This is great, thanks! We've seen what I'm fairly sure is the same > > > bug > > > from Red Hat users. I think my blind spot was an assumption that > > > rpc tasks wouldn't outlive rpc_shutdown_client(). > > > > > > However, it's causing xfstests runs to hang, and I haven't worked out > > > why yet. > > > > > > I'll spend some time on it this afternoon and let you know what I > > > figure > > > out. > > > > > > > Is that happening with v2 or with v1? With v1 there is definitely a > > hang in __destroy_client() due to the refcount leak that I believe I > > fixed in v2. > > I thought I was running v2, let me double-check.... Yes, with v2 I'm getting a hang on generic/013. I checked quickly and didn't see anything interesting in the logs, otherwise I haven't done any digging. --b. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() 2019-10-25 15:33 ` J. Bruce Fields @ 2019-10-29 21:47 ` J. Bruce Fields 2019-11-07 22:27 ` J. Bruce Fields 0 siblings, 1 reply; 12+ messages in thread From: J. Bruce Fields @ 2019-10-29 21:47 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Trond Myklebust, linux-nfs On Fri, Oct 25, 2019 at 11:33:36AM -0400, bfields wrote: > On Fri, Oct 25, 2019 at 11:21:19AM -0400, J. Bruce Fields wrote: > > I thought I was running v2, let me double-check.... > > Yes, with v2 I'm getting a hang on generic/013. > > I checked quickly and didn't see anything interesting in the logs, > otherwise I haven't done any digging. Reproduceable just with ./check -nfs generic/013. The last thing I see in wireshark is an asynchronous COPY call and reply. Which means it's probably trying to do a CB_OFFLOAD. Hm. --b. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() 2019-10-29 21:47 ` J. Bruce Fields @ 2019-11-07 22:27 ` J. Bruce Fields 2019-11-08 17:51 ` J. Bruce Fields 2019-11-08 18:44 ` [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() Trond Myklebust 0 siblings, 2 replies; 12+ messages in thread From: J. Bruce Fields @ 2019-11-07 22:27 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Trond Myklebust, linux-nfs On Tue, Oct 29, 2019 at 05:47:05PM -0400, J. Bruce Fields wrote: > On Fri, Oct 25, 2019 at 11:33:36AM -0400, bfields wrote: > > On Fri, Oct 25, 2019 at 11:21:19AM -0400, J. Bruce Fields wrote: > > > I thought I was running v2, let me double-check.... > > > > Yes, with v2 I'm getting a hang on generic/013. > > > > I checked quickly and didn't see anything interesting in the logs, > > otherwise I haven't done any digging. > > Reproduceable just with ./check -nfs generic/013. The last thing I see > in wireshark is an asynchronous COPY call and reply. Which means it's > probably trying to do a CB_OFFLOAD. Hm. Oh, I think it just needs the following. --b. diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index fb71e7f9d0d9..e49604701a71 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -1026,8 +1026,8 @@ static bool nfsd41_cb_get_slot(struct nfsd4_callback *cb, struct rpc_task *task) return false; } rpc_wake_up_queued_task(&clp->cl_cb_waitq, task); - cb->cb_holds_slot = true; } + cb->cb_holds_slot = true; return true; } ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() 2019-11-07 22:27 ` J. Bruce Fields @ 2019-11-08 17:51 ` J. Bruce Fields 2019-11-08 17:52 ` J. Bruce Fields 2019-11-08 18:44 ` [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() Trond Myklebust 1 sibling, 1 reply; 12+ messages in thread From: J. Bruce Fields @ 2019-11-08 17:51 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Trond Myklebust, linux-nfs On Thu, Nov 07, 2019 at 05:27:12PM -0500, J. Bruce Fields wrote: > On Tue, Oct 29, 2019 at 05:47:05PM -0400, J. Bruce Fields wrote: > > On Fri, Oct 25, 2019 at 11:33:36AM -0400, bfields wrote: > > > On Fri, Oct 25, 2019 at 11:21:19AM -0400, J. Bruce Fields wrote: > > > > I thought I was running v2, let me double-check.... > > > > > > Yes, with v2 I'm getting a hang on generic/013. > > > > > > I checked quickly and didn't see anything interesting in the logs, > > > otherwise I haven't done any digging. > > > > Reproduceable just with ./check -nfs generic/013. The last thing I see > > in wireshark is an asynchronous COPY call and reply. Which means it's > > probably trying to do a CB_OFFLOAD. Hm. > > Oh, I think it just needs the following. Applying as follows, with part of the change split out into a separate patch (since that's how I noticed the bug). --b. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() 2019-11-08 17:51 ` J. Bruce Fields @ 2019-11-08 17:52 ` J. Bruce Fields 2019-11-08 17:54 ` [PATCH] nfsd: document callback_wq serialization of callback code J. Bruce Fields 0 siblings, 1 reply; 12+ messages in thread From: J. Bruce Fields @ 2019-11-08 17:52 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Trond Myklebust, linux-nfs On Fri, Nov 08, 2019 at 12:51:09PM -0500, J. Bruce Fields wrote: > On Thu, Nov 07, 2019 at 05:27:12PM -0500, J. Bruce Fields wrote: > > On Tue, Oct 29, 2019 at 05:47:05PM -0400, J. Bruce Fields wrote: > > > On Fri, Oct 25, 2019 at 11:33:36AM -0400, bfields wrote: > > > > On Fri, Oct 25, 2019 at 11:21:19AM -0400, J. Bruce Fields wrote: > > > > > I thought I was running v2, let me double-check.... > > > > > > > > Yes, with v2 I'm getting a hang on generic/013. > > > > > > > > I checked quickly and didn't see anything interesting in the logs, > > > > otherwise I haven't done any digging. > > > > > > Reproduceable just with ./check -nfs generic/013. The last thing I see > > > in wireshark is an asynchronous COPY call and reply. Which means it's > > > probably trying to do a CB_OFFLOAD. Hm. > > > > Oh, I think it just needs the following. > > Applying as follows, with part of the change split out into a separate > patch (since that's how I noticed the bug). > > --b. Oops, remembered to append the patches this time.--b. commit 2bbfed98a4d8 Author: Trond Myklebust <trondmy@gmail.com> Date: Wed Oct 23 17:43:18 2019 -0400 nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() When we're destroying the client lease, and we call nfsd4_shutdown_callback(), we must ensure that we do not return before all outstanding callbacks have terminated and have released their payloads. Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> Signed-off-by: J. Bruce Fields <bfields@redhat.com> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index 1542e1d6dd1a..67d24a536082 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -826,6 +826,31 @@ static int max_cb_time(struct net *net) return max(nn->nfsd4_lease/10, (time_t)1) * HZ; } +static struct workqueue_struct *callback_wq; + +static bool nfsd4_queue_cb(struct nfsd4_callback *cb) +{ + return queue_work(callback_wq, &cb->cb_work); +} + +static void nfsd41_cb_inflight_begin(struct nfs4_client *clp) +{ + atomic_inc(&clp->cl_cb_inflight); +} + +static void nfsd41_cb_inflight_end(struct nfs4_client *clp) +{ + + if (atomic_dec_and_test(&clp->cl_cb_inflight)) + wake_up_var(&clp->cl_cb_inflight); +} + +static void nfsd41_cb_inflight_wait_complete(struct nfs4_client *clp) +{ + wait_var_event(&clp->cl_cb_inflight, + !atomic_read(&clp->cl_cb_inflight)); +} + static const struct cred *get_backchannel_cred(struct nfs4_client *clp, struct rpc_clnt *client, struct nfsd4_session *ses) { if (clp->cl_minorversion == 0) { @@ -937,14 +962,21 @@ static void nfsd4_cb_probe_done(struct rpc_task *task, void *calldata) clp->cl_cb_state = NFSD4_CB_UP; } +static void nfsd4_cb_probe_release(void *calldata) +{ + struct nfs4_client *clp = container_of(calldata, struct nfs4_client, cl_cb_null); + + nfsd41_cb_inflight_end(clp); + +} + static const struct rpc_call_ops nfsd4_cb_probe_ops = { /* XXX: release method to ensure we set the cb channel down if * necessary on early failure? */ .rpc_call_done = nfsd4_cb_probe_done, + .rpc_release = nfsd4_cb_probe_release, }; -static struct workqueue_struct *callback_wq; - /* * Poke the callback thread to process any updates to the callback * parameters, and send a null probe. @@ -1004,6 +1036,16 @@ static void nfsd41_cb_release_slot(struct nfsd4_callback *cb) } } +static void nfsd41_destroy_cb(struct nfsd4_callback *cb) +{ + struct nfs4_client *clp = cb->cb_clp; + + nfsd41_cb_release_slot(cb); + if (cb->cb_ops && cb->cb_ops->release) + cb->cb_ops->release(cb); + nfsd41_cb_inflight_end(clp); +} + /* * TODO: cb_sequence should support referring call lists, cachethis, multiple * slots, and mark callback channel down on communication errors. @@ -1101,8 +1143,10 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback ret = false; goto out; need_restart: - task->tk_status = 0; - cb->cb_need_restart = true; + if (!test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) { + task->tk_status = 0; + cb->cb_need_restart = true; + } return false; } @@ -1144,9 +1188,9 @@ static void nfsd4_cb_release(void *calldata) struct nfsd4_callback *cb = calldata; if (cb->cb_need_restart) - nfsd4_run_cb(cb); + nfsd4_queue_cb(cb); else - cb->cb_ops->release(cb); + nfsd41_destroy_cb(cb); } @@ -1180,6 +1224,7 @@ void nfsd4_shutdown_callback(struct nfs4_client *clp) */ nfsd4_run_cb(&clp->cl_cb_null); flush_workqueue(callback_wq); + nfsd41_cb_inflight_wait_complete(clp); } /* requires cl_lock: */ @@ -1265,8 +1310,7 @@ nfsd4_run_cb_work(struct work_struct *work) clnt = clp->cl_cb_client; if (!clnt) { /* Callback channel broken, or client killed; give up: */ - if (cb->cb_ops && cb->cb_ops->release) - cb->cb_ops->release(cb); + nfsd41_destroy_cb(cb); return; } @@ -1275,6 +1319,7 @@ nfsd4_run_cb_work(struct work_struct *work) */ if (!cb->cb_ops && clp->cl_minorversion) { clp->cl_cb_state = NFSD4_CB_UP; + nfsd41_destroy_cb(cb); return; } @@ -1300,5 +1345,9 @@ void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp, void nfsd4_run_cb(struct nfsd4_callback *cb) { - queue_work(callback_wq, &cb->cb_work); + struct nfs4_client *clp = cb->cb_clp; + + nfsd41_cb_inflight_begin(clp); + if (!nfsd4_queue_cb(cb)) + nfsd41_cb_inflight_end(clp); } diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h index 46f56afb6cb8..d61b83b9654c 100644 --- a/fs/nfsd/state.h +++ b/fs/nfsd/state.h @@ -367,6 +367,7 @@ struct nfs4_client { struct net *net; struct list_head async_copies; /* list of async copies */ spinlock_t async_lock; /* lock for async copies */ + atomic_t cl_cb_inflight; /* Outstanding callbacks */ }; /* struct nfs4_client_reset commit 12357f1b2c8e Author: Trond Myklebust <trondmy@gmail.com> Date: Thu Nov 7 17:11:57 2019 -0500 nfsd: minor 4.1 callback cleanup Move all the cb_holds_slot management into helper functions. No change in behavior. Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> Signed-off-by: J. Bruce Fields <bfields@redhat.com> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index 524111420b48..1542e1d6dd1a 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -975,9 +975,12 @@ void nfsd4_change_callback(struct nfs4_client *clp, struct nfs4_cb_conn *conn) * If the slot is available, then mark it busy. Otherwise, set the * thread for sleeping on the callback RPC wait queue. */ -static bool nfsd41_cb_get_slot(struct nfs4_client *clp, struct rpc_task *task) +static bool nfsd41_cb_get_slot(struct nfsd4_callback *cb, struct rpc_task *task) { - if (test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { + struct nfs4_client *clp = cb->cb_clp; + + if (!cb->cb_holds_slot && + test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { rpc_sleep_on(&clp->cl_cb_waitq, task, NULL); /* Race breaker */ if (test_and_set_bit(0, &clp->cl_cb_slot_busy) != 0) { @@ -986,9 +989,21 @@ static bool nfsd41_cb_get_slot(struct nfs4_client *clp, struct rpc_task *task) } rpc_wake_up_queued_task(&clp->cl_cb_waitq, task); } + cb->cb_holds_slot = true; return true; } +static void nfsd41_cb_release_slot(struct nfsd4_callback *cb) +{ + struct nfs4_client *clp = cb->cb_clp; + + if (cb->cb_holds_slot) { + cb->cb_holds_slot = false; + clear_bit(0, &clp->cl_cb_slot_busy); + rpc_wake_up_next(&clp->cl_cb_waitq); + } +} + /* * TODO: cb_sequence should support referring call lists, cachethis, multiple * slots, and mark callback channel down on communication errors. @@ -1005,11 +1020,8 @@ static void nfsd4_cb_prepare(struct rpc_task *task, void *calldata) */ cb->cb_seq_status = 1; cb->cb_status = 0; - if (minorversion) { - if (!cb->cb_holds_slot && !nfsd41_cb_get_slot(clp, task)) - return; - cb->cb_holds_slot = true; - } + if (minorversion && !nfsd41_cb_get_slot(cb, task)) + return; rpc_call_start(task); } @@ -1076,9 +1088,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback cb->cb_seq_status); } - cb->cb_holds_slot = false; - clear_bit(0, &clp->cl_cb_slot_busy); - rpc_wake_up_next(&clp->cl_cb_waitq); + nfsd41_cb_release_slot(cb); dprintk("%s: freed slot, new seqid=%d\n", __func__, clp->cl_cb_session->se_cb_seq_nr); ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] nfsd: document callback_wq serialization of callback code 2019-11-08 17:52 ` J. Bruce Fields @ 2019-11-08 17:54 ` J. Bruce Fields 2019-11-08 17:55 ` [PATCH] nfsd: mark cb path down on unknown errors J. Bruce Fields 0 siblings, 1 reply; 12+ messages in thread From: J. Bruce Fields @ 2019-11-08 17:54 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Trond Myklebust, linux-nfs From: "J. Bruce Fields" <bfields@redhat.com> Date: Tue, 29 Oct 2019 16:02:18 -0400 The callback code relies on the fact that much of it is only ever called from the ordered workqueue callback_wq, and this is worth documenting. Reported-by: Trond Myklebust <trond.myklebust@hammerspace.com> Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4callback.c | 6 ++++++ 1 file changed, 6 insertions(+) Also adding a comment, since I know this was a source of confusion when investigating these races. diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index c94768b096a3..24534db87e86 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -1243,6 +1243,12 @@ static struct nfsd4_conn * __nfsd4_find_backchannel(struct nfs4_client *clp) return NULL; } +/* + * Note there isn't a lot of locking in this code; instead we depend on + * the fact that it is run from the callback_wq, which won't run two + * work items at once. So, for example, callback_wq handles all access + * of cl_cb_client and all calls to rpc_create or rpc_shutdown_client. + */ static void nfsd4_process_cb_update(struct nfsd4_callback *cb) { struct nfs4_cb_conn conn; -- 2.23.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] nfsd: mark cb path down on unknown errors 2019-11-08 17:54 ` [PATCH] nfsd: document callback_wq serialization of callback code J. Bruce Fields @ 2019-11-08 17:55 ` J. Bruce Fields 0 siblings, 0 replies; 12+ messages in thread From: J. Bruce Fields @ 2019-11-08 17:55 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Trond Myklebust, linux-nfs From: "J. Bruce Fields" <bfields@redhat.com> Date: Tue, 22 Oct 2019 12:29:37 -0400 An unexpected error is probably a sign that something is wrong with the callback path. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4callback.c | 1 + 1 file changed, 1 insertion(+) Also, while we're here....--b. diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index 67d24a536082..c94768b096a3 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -1126,6 +1126,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback } break; default: + nfsd4_mark_cb_fault(cb->cb_clp, cb->cb_seq_status); dprintk("%s: unprocessed error %d\n", __func__, cb->cb_seq_status); } -- 2.23.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() 2019-11-07 22:27 ` J. Bruce Fields 2019-11-08 17:51 ` J. Bruce Fields @ 2019-11-08 18:44 ` Trond Myklebust 1 sibling, 0 replies; 12+ messages in thread From: Trond Myklebust @ 2019-11-08 18:44 UTC (permalink / raw) To: bfields, bfields; +Cc: linux-nfs On Thu, 2019-11-07 at 17:27 -0500, J. Bruce Fields wrote: > On Tue, Oct 29, 2019 at 05:47:05PM -0400, J. Bruce Fields wrote: > > On Fri, Oct 25, 2019 at 11:33:36AM -0400, bfields wrote: > > > On Fri, Oct 25, 2019 at 11:21:19AM -0400, J. Bruce Fields wrote: > > > > I thought I was running v2, let me double-check.... > > > > > > Yes, with v2 I'm getting a hang on generic/013. > > > > > > I checked quickly and didn't see anything interesting in the > > > logs, > > > otherwise I haven't done any digging. > > > > Reproduceable just with ./check -nfs generic/013. The last thing I > > see > > in wireshark is an asynchronous COPY call and reply. Which means > > it's > > probably trying to do a CB_OFFLOAD. Hm. > > Oh, I think it just needs the following. > > --b. > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index fb71e7f9d0d9..e49604701a71 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -1026,8 +1026,8 @@ static bool nfsd41_cb_get_slot(struct > nfsd4_callback *cb, struct rpc_task *task) > return false; > } > rpc_wake_up_queued_task(&clp->cl_cb_waitq, task); > - cb->cb_holds_slot = true; > } > + cb->cb_holds_slot = true; > return true; > } > Doh! Thanks for spotting that. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-11-08 18:44 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-23 21:43 [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() Trond Myklebust 2019-10-25 14:51 ` J. Bruce Fields 2019-10-25 14:55 ` Trond Myklebust 2019-10-25 15:21 ` J. Bruce Fields 2019-10-25 15:33 ` J. Bruce Fields 2019-10-29 21:47 ` J. Bruce Fields 2019-11-07 22:27 ` J. Bruce Fields 2019-11-08 17:51 ` J. Bruce Fields 2019-11-08 17:52 ` J. Bruce Fields 2019-11-08 17:54 ` [PATCH] nfsd: document callback_wq serialization of callback code J. Bruce Fields 2019-11-08 17:55 ` [PATCH] nfsd: mark cb path down on unknown errors J. Bruce Fields 2019-11-08 18:44 ` [PATCH v2] nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback() Trond Myklebust
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).