| Message ID | 668e25098cb97187d084d5fa2916ddd4d2a68e00.1742941932.git.trond.myklebust@hammerspace.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Ensure that ENETUNREACH terminates state recovery | expand |
On Tue, 2025-03-25 at 18:35 -0400, trondmy@kernel.org wrote: > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > Replace the tests for the RPC client being shut down with tests for > whether the nfs_client is in an error state. > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfs/nfs4proc.c | 2 +- > fs/nfs/nfs4state.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 889511650ceb..50be54e0f578 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -9580,7 +9580,7 @@ static void nfs41_sequence_call_done(struct rpc_task *task, void *data) > return; > > trace_nfs4_sequence(clp, task->tk_status); > - if (task->tk_status < 0 && !task->tk_client->cl_shutdown) { > + if (task->tk_status < 0 && clp->cl_cons_state >= 0) { > dprintk("%s ERROR %d\n", __func__, task->tk_status); > if (refcount_read(&clp->cl_count) == 1) > return; > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c > index 542cdf71229f..f1f7eaa97973 100644 > --- a/fs/nfs/nfs4state.c > +++ b/fs/nfs/nfs4state.c > @@ -1198,7 +1198,7 @@ void nfs4_schedule_state_manager(struct nfs_client *clp) > struct rpc_clnt *clnt = clp->cl_rpcclient; > bool swapon = false; > > - if (clnt->cl_shutdown) > + if (clp->cl_cons_state < 0) > return; > > set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state); This should probably also have: Fixes: 6ad477a69ad8 ("NFSv4: Clean up some shutdown loops") ...but otherwise this looks good to me. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Tue, 2025-03-25 at 18:35 -0400, trondmy@kernel.org wrote: > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > Replace the tests for the RPC client being shut down with tests for > whether the nfs_client is in an error state. > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfs/nfs4proc.c | 2 +- > fs/nfs/nfs4state.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 889511650ceb..50be54e0f578 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -9580,7 +9580,7 @@ static void nfs41_sequence_call_done(struct rpc_task *task, void *data) > return; > > trace_nfs4_sequence(clp, task->tk_status); > - if (task->tk_status < 0 && !task->tk_client->cl_shutdown) { > + if (task->tk_status < 0 && clp->cl_cons_state >= 0) { > dprintk("%s ERROR %d\n", __func__, task->tk_status); > if (refcount_read(&clp->cl_count) == 1) > return; > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c > index 542cdf71229f..f1f7eaa97973 100644 > --- a/fs/nfs/nfs4state.c > +++ b/fs/nfs/nfs4state.c > @@ -1198,7 +1198,7 @@ void nfs4_schedule_state_manager(struct nfs_client *clp) > struct rpc_clnt *clnt = clp->cl_rpcclient; > bool swapon = false; > > - if (clnt->cl_shutdown) > + if (clp->cl_cons_state < 0) > return; > > set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state); One more thing: Do we need cl_shutdown at all? If we can replace these checks here with a check for cl_cons_state < 0, why not do the same in call_start()?
On 26 Mar 2025, at 6:13, Jeff Layton wrote: > On Tue, 2025-03-25 at 18:35 -0400, trondmy@kernel.org wrote: >> From: Trond Myklebust <trond.myklebust@hammerspace.com> >> >> Replace the tests for the RPC client being shut down with tests for >> whether the nfs_client is in an error state. >> >> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> >> --- >> fs/nfs/nfs4proc.c | 2 +- >> fs/nfs/nfs4state.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index 889511650ceb..50be54e0f578 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -9580,7 +9580,7 @@ static void nfs41_sequence_call_done(struct rpc_task *task, void *data) >> return; >> >> trace_nfs4_sequence(clp, task->tk_status); >> - if (task->tk_status < 0 && !task->tk_client->cl_shutdown) { >> + if (task->tk_status < 0 && clp->cl_cons_state >= 0) { >> dprintk("%s ERROR %d\n", __func__, task->tk_status); >> if (refcount_read(&clp->cl_count) == 1) >> return; >> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c >> index 542cdf71229f..f1f7eaa97973 100644 >> --- a/fs/nfs/nfs4state.c >> +++ b/fs/nfs/nfs4state.c >> @@ -1198,7 +1198,7 @@ void nfs4_schedule_state_manager(struct nfs_client *clp) >> struct rpc_clnt *clnt = clp->cl_rpcclient; >> bool swapon = false; >> >> - if (clnt->cl_shutdown) >> + if (clp->cl_cons_state < 0) >> return; >> >> set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state); > > One more thing: > > Do we need cl_shutdown at all? If we can replace these checks here with > a check for cl_cons_state < 0, why not do the same in call_start()? > -- > Jeff Layton <jlayton@kernel.org> Agree - I don't see why not. Reviewed-by: Benjamin Coddington <bcodding@redhat.com> Ben
On Wed, 2025-03-26 at 06:13 -0400, Jeff Layton wrote: > On Tue, 2025-03-25 at 18:35 -0400, trondmy@kernel.org wrote: > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > Replace the tests for the RPC client being shut down with tests for > > whether the nfs_client is in an error state. > > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > fs/nfs/nfs4proc.c | 2 +- > > fs/nfs/nfs4state.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > index 889511650ceb..50be54e0f578 100644 > > --- a/fs/nfs/nfs4proc.c > > +++ b/fs/nfs/nfs4proc.c > > @@ -9580,7 +9580,7 @@ static void nfs41_sequence_call_done(struct > > rpc_task *task, void *data) > > return; > > > > trace_nfs4_sequence(clp, task->tk_status); > > - if (task->tk_status < 0 && !task->tk_client->cl_shutdown) > > { > > + if (task->tk_status < 0 && clp->cl_cons_state >= 0) { > > dprintk("%s ERROR %d\n", __func__, task- > > >tk_status); > > if (refcount_read(&clp->cl_count) == 1) > > return; > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c > > index 542cdf71229f..f1f7eaa97973 100644 > > --- a/fs/nfs/nfs4state.c > > +++ b/fs/nfs/nfs4state.c > > @@ -1198,7 +1198,7 @@ void nfs4_schedule_state_manager(struct > > nfs_client *clp) > > struct rpc_clnt *clnt = clp->cl_rpcclient; > > bool swapon = false; > > > > - if (clnt->cl_shutdown) > > + if (clp->cl_cons_state < 0) > > return; > > > > set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state); > > One more thing: > > Do we need cl_shutdown at all? If we can replace these checks here > with > a check for cl_cons_state < 0, why not do the same in call_start()? The struct nfs_client is a NFS level object. It can't be moved to the RPC layer.
On Wed, 2025-03-26 at 18:11 +0000, Trond Myklebust wrote: > On Wed, 2025-03-26 at 06:13 -0400, Jeff Layton wrote: > > On Tue, 2025-03-25 at 18:35 -0400, trondmy@kernel.org wrote: > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > > > Replace the tests for the RPC client being shut down with tests for > > > whether the nfs_client is in an error state. > > > > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > > --- > > > fs/nfs/nfs4proc.c | 2 +- > > > fs/nfs/nfs4state.c | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > index 889511650ceb..50be54e0f578 100644 > > > --- a/fs/nfs/nfs4proc.c > > > +++ b/fs/nfs/nfs4proc.c > > > @@ -9580,7 +9580,7 @@ static void nfs41_sequence_call_done(struct > > > rpc_task *task, void *data) > > > return; > > > > > > trace_nfs4_sequence(clp, task->tk_status); > > > - if (task->tk_status < 0 && !task->tk_client->cl_shutdown) > > > { > > > + if (task->tk_status < 0 && clp->cl_cons_state >= 0) { > > > dprintk("%s ERROR %d\n", __func__, task- > > > > tk_status); > > > if (refcount_read(&clp->cl_count) == 1) > > > return; > > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c > > > index 542cdf71229f..f1f7eaa97973 100644 > > > --- a/fs/nfs/nfs4state.c > > > +++ b/fs/nfs/nfs4state.c > > > @@ -1198,7 +1198,7 @@ void nfs4_schedule_state_manager(struct > > > nfs_client *clp) > > > struct rpc_clnt *clnt = clp->cl_rpcclient; > > > bool swapon = false; > > > > > > - if (clnt->cl_shutdown) > > > + if (clp->cl_cons_state < 0) > > > return; > > > > > > set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state); > > > > One more thing: > > > > Do we need cl_shutdown at all? If we can replace these checks here > > with > > a check for cl_cons_state < 0, why not do the same in call_start()? > > The struct nfs_client is a NFS level object. It can't be moved to the > RPC layer. > But...cl_shutdown is an rpc_clnt field.
On Wed, 2025-03-26 at 14:21 -0400, Jeff Layton wrote: > On Wed, 2025-03-26 at 18:11 +0000, Trond Myklebust wrote: > > On Wed, 2025-03-26 at 06:13 -0400, Jeff Layton wrote: > > > On Tue, 2025-03-25 at 18:35 -0400, trondmy@kernel.org wrote: > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > > > > > Replace the tests for the RPC client being shut down with tests > > > > for > > > > whether the nfs_client is in an error state. > > > > > > > > Signed-off-by: Trond Myklebust > > > > <trond.myklebust@hammerspace.com> > > > > --- > > > > fs/nfs/nfs4proc.c | 2 +- > > > > fs/nfs/nfs4state.c | 2 +- > > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > > index 889511650ceb..50be54e0f578 100644 > > > > --- a/fs/nfs/nfs4proc.c > > > > +++ b/fs/nfs/nfs4proc.c > > > > @@ -9580,7 +9580,7 @@ static void > > > > nfs41_sequence_call_done(struct > > > > rpc_task *task, void *data) > > > > return; > > > > > > > > trace_nfs4_sequence(clp, task->tk_status); > > > > - if (task->tk_status < 0 && !task->tk_client- > > > > >cl_shutdown) > > > > { > > > > + if (task->tk_status < 0 && clp->cl_cons_state >= 0) { > > > > dprintk("%s ERROR %d\n", __func__, task- > > > > > tk_status); > > > > if (refcount_read(&clp->cl_count) == 1) > > > > return; > > > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c > > > > index 542cdf71229f..f1f7eaa97973 100644 > > > > --- a/fs/nfs/nfs4state.c > > > > +++ b/fs/nfs/nfs4state.c > > > > @@ -1198,7 +1198,7 @@ void nfs4_schedule_state_manager(struct > > > > nfs_client *clp) > > > > struct rpc_clnt *clnt = clp->cl_rpcclient; > > > > bool swapon = false; > > > > > > > > - if (clnt->cl_shutdown) > > > > + if (clp->cl_cons_state < 0) > > > > return; > > > > > > > > set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state); > > > > > > One more thing: > > > > > > Do we need cl_shutdown at all? If we can replace these checks > > > here > > > with > > > a check for cl_cons_state < 0, why not do the same in > > > call_start()? > > > > The struct nfs_client is a NFS level object. It can't be moved to > > the > > RPC layer. > > > > But...cl_shutdown is an rpc_clnt field. Right, but cl_cons_state is a field in struct nfs_client, hence that check cannot be moved into call_start()
On Wed, 2025-03-26 at 18:24 +0000, Trond Myklebust wrote: > On Wed, 2025-03-26 at 14:21 -0400, Jeff Layton wrote: > > On Wed, 2025-03-26 at 18:11 +0000, Trond Myklebust wrote: > > > On Wed, 2025-03-26 at 06:13 -0400, Jeff Layton wrote: > > > > On Tue, 2025-03-25 at 18:35 -0400, trondmy@kernel.org wrote: > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > > > > > > > Replace the tests for the RPC client being shut down with tests > > > > > for > > > > > whether the nfs_client is in an error state. > > > > > > > > > > Signed-off-by: Trond Myklebust > > > > > <trond.myklebust@hammerspace.com> > > > > > --- > > > > > fs/nfs/nfs4proc.c | 2 +- > > > > > fs/nfs/nfs4state.c | 2 +- > > > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > > > index 889511650ceb..50be54e0f578 100644 > > > > > --- a/fs/nfs/nfs4proc.c > > > > > +++ b/fs/nfs/nfs4proc.c > > > > > @@ -9580,7 +9580,7 @@ static void > > > > > nfs41_sequence_call_done(struct > > > > > rpc_task *task, void *data) > > > > > return; > > > > > > > > > > trace_nfs4_sequence(clp, task->tk_status); > > > > > - if (task->tk_status < 0 && !task->tk_client- > > > > > > cl_shutdown) > > > > > { > > > > > + if (task->tk_status < 0 && clp->cl_cons_state >= 0) { > > > > > dprintk("%s ERROR %d\n", __func__, task- > > > > > > tk_status); > > > > > if (refcount_read(&clp->cl_count) == 1) > > > > > return; > > > > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c > > > > > index 542cdf71229f..f1f7eaa97973 100644 > > > > > --- a/fs/nfs/nfs4state.c > > > > > +++ b/fs/nfs/nfs4state.c > > > > > @@ -1198,7 +1198,7 @@ void nfs4_schedule_state_manager(struct > > > > > nfs_client *clp) > > > > > struct rpc_clnt *clnt = clp->cl_rpcclient; > > > > > bool swapon = false; > > > > > > > > > > - if (clnt->cl_shutdown) > > > > > + if (clp->cl_cons_state < 0) > > > > > return; > > > > > > > > > > set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state); > > > > > > > > One more thing: > > > > > > > > Do we need cl_shutdown at all? If we can replace these checks > > > > here > > > > with > > > > a check for cl_cons_state < 0, why not do the same in > > > > call_start()? > > > > > > The struct nfs_client is a NFS level object. It can't be moved to > > > the > > > RPC layer. > > > > > > > But...cl_shutdown is an rpc_clnt field. > > Right, but cl_cons_state is a field in struct nfs_client, hence that > check cannot be moved into call_start() Doh! Good point. Nevermind!
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 889511650ceb..50be54e0f578 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -9580,7 +9580,7 @@ static void nfs41_sequence_call_done(struct rpc_task *task, void *data) return; trace_nfs4_sequence(clp, task->tk_status); - if (task->tk_status < 0 && !task->tk_client->cl_shutdown) { + if (task->tk_status < 0 && clp->cl_cons_state >= 0) { dprintk("%s ERROR %d\n", __func__, task->tk_status); if (refcount_read(&clp->cl_count) == 1) return; diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c index 542cdf71229f..f1f7eaa97973 100644 --- a/fs/nfs/nfs4state.c +++ b/fs/nfs/nfs4state.c @@ -1198,7 +1198,7 @@ void nfs4_schedule_state_manager(struct nfs_client *clp) struct rpc_clnt *clnt = clp->cl_rpcclient; bool swapon = false; - if (clnt->cl_shutdown) + if (clp->cl_cons_state < 0) return; set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state);