[PATCH v2 0/4] Ensure that ENETUNREACH terminates state recovery

[PATCH v2 0/4] Ensure that ENETUNREACH terminates state recovery

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

With the recent patch series that caused containerised mounts which
return ENETUNREACH or ENETDOWN errors to report fatal errors, we also
want to ensure that the state manager thread also triggers fatal errors
in the processes or threads that are waiting for recovery to complete.

---
v2:
 - Return EIO instead of ENETUNREACH in nfs4_wait_clnt_recover()

Trond Myklebust (4):
  SUNRPC: rpcbind should never reset the port to the value '0'
  SUNRPC: rpc_clnt_set_transport() must not change the autobind setting
  NFSv4: clp->cl_cons_state < 0 signifies an invalid nfs_client
  NFSv4: Treat ENETUNREACH errors as fatal for state recovery

 fs/nfs/nfs4state.c     | 14 +++++++++++---
 net/sunrpc/clnt.c      |  3 ---
 net/sunrpc/rpcb_clnt.c |  5 +++--
 3 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.49.0

[PATCH v2 1/4] SUNRPC: rpcbind should never reset the port to the value '0'

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

If we already had a valid port number for the RPC service, then we
should not allow the rpcbind client to set it to the invalid value '0'.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/rpcb_clnt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 102c3818bc54..53bcca365fb1 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -820,9 +820,10 @@ static void rpcb_getport_done(struct rpc_task *child, void *data)
 	}
 
 	trace_rpcb_setport(child, map->r_status, map->r_port);
-	xprt->ops->set_port(xprt, map->r_port);
-	if (map->r_port)
+	if (map->r_port) {
+		xprt->ops->set_port(xprt, map->r_port);
 		xprt_set_bound(xprt);
+	}
 }
 
 /*
-- 
2.49.0

Re: [PATCH v2 1/4] SUNRPC: rpcbind should never reset the port to the value '0'

[Jeff Layton]

On Tue, 2025-03-25 at 12:17 -0400, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> If we already had a valid port number for the RPC service, then we
> should not allow the rpcbind client to set it to the invalid value '0'.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  net/sunrpc/rpcb_clnt.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> index 102c3818bc54..53bcca365fb1 100644
> --- a/net/sunrpc/rpcb_clnt.c
> +++ b/net/sunrpc/rpcb_clnt.c
> @@ -820,9 +820,10 @@ static void rpcb_getport_done(struct rpc_task *child, void *data)
>  	}
>  
>  	trace_rpcb_setport(child, map->r_status, map->r_port);
> -	xprt->ops->set_port(xprt, map->r_port);
> -	if (map->r_port)
> +	if (map->r_port) {
> +		xprt->ops->set_port(xprt, map->r_port);
>  		xprt_set_bound(xprt);
> +	}
>  }
>  
>  /*

Reviewed-by: Jeff Layton <jlayton@kernel.org>

[PATCH v2 2/4] SUNRPC: rpc_clnt_set_transport() must not change the autobind setting

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

The autobind setting was supposed to be determined in rpc_create(),
since commit c2866763b402 ("SUNRPC: use sockaddr + size when creating
remote transport endpoints").

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/clnt.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 1cfaf93ceec1..6f75862d9782 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -270,9 +270,6 @@ static struct rpc_xprt *rpc_clnt_set_transport(struct rpc_clnt *clnt,
 	old = rcu_dereference_protected(clnt->cl_xprt,
 			lockdep_is_held(&clnt->cl_lock));
 
-	if (!xprt_bound(xprt))
-		clnt->cl_autobind = 1;
-
 	clnt->cl_timeout = timeout;
 	rcu_assign_pointer(clnt->cl_xprt, xprt);
 	spin_unlock(&clnt->cl_lock);
-- 
2.49.0

Re: [PATCH v2 2/4] SUNRPC: rpc_clnt_set_transport() must not change the autobind setting

[Jeff Layton]

On Tue, 2025-03-25 at 12:17 -0400, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> The autobind setting was supposed to be determined in rpc_create(),
> since commit c2866763b402 ("SUNRPC: use sockaddr + size when creating
> remote transport endpoints").
> 

Should this have:

    Fixes: c2866763b402 ("SUNRPC: use sockaddr + size when creating remote transport endpoints")

?

Though I guess that commit is old enough that maybe it doesn't matter.

> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  net/sunrpc/clnt.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 1cfaf93ceec1..6f75862d9782 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -270,9 +270,6 @@ static struct rpc_xprt *rpc_clnt_set_transport(struct rpc_clnt *clnt,
>  	old = rcu_dereference_protected(clnt->cl_xprt,
>  			lockdep_is_held(&clnt->cl_lock));
>  
> -	if (!xprt_bound(xprt))
> -		clnt->cl_autobind = 1;
> -
>  	clnt->cl_timeout = timeout;
>  	rcu_assign_pointer(clnt->cl_xprt, xprt);
>  	spin_unlock(&clnt->cl_lock);

Reviewed-by: Jeff Layton <jlayton@kernel.org>

[PATCH v2 3/4] NFSv4: clp->cl_cons_state < 0 signifies an invalid nfs_client

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

If someone calls nfs_mark_client_ready(clp, status) with a negative
value for status, then that should signal that the nfs_client is no
longer valid.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs4state.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 542cdf71229f..738eb2789266 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 (clnt->cl_shutdown || clp->cl_cons_state < 0)
 		return;
 
 	set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state);
@@ -1403,7 +1403,7 @@ int nfs4_schedule_stateid_recovery(const struct nfs_server *server, struct nfs4_
 	dprintk("%s: scheduling stateid recovery for server %s\n", __func__,
 			clp->cl_hostname);
 	nfs4_schedule_state_manager(clp);
-	return 0;
+	return clp->cl_cons_state < 0 ? clp->cl_cons_state : 0;
 }
 EXPORT_SYMBOL_GPL(nfs4_schedule_stateid_recovery);
 
-- 
2.49.0

Re: [PATCH v2 3/4] NFSv4: clp->cl_cons_state < 0 signifies an invalid nfs_client

[Jeff Layton]

On Tue, 2025-03-25 at 12:17 -0400, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> If someone calls nfs_mark_client_ready(clp, status) with a negative
> value for status, then that should signal that the nfs_client is no
> longer valid.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/nfs4state.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 542cdf71229f..738eb2789266 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 (clnt->cl_shutdown || clp->cl_cons_state < 0)

Would it be simpler to just set cl_shutdown when this occurs instead of
having to check cl_cons_state as well?

>  		return;
>  
>  	set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state);
> @@ -1403,7 +1403,7 @@ int nfs4_schedule_stateid_recovery(const struct nfs_server *server, struct nfs4_
>  	dprintk("%s: scheduling stateid recovery for server %s\n", __func__,
>  			clp->cl_hostname);
>  	nfs4_schedule_state_manager(clp);
> -	return 0;
> +	return clp->cl_cons_state < 0 ? clp->cl_cons_state : 0;
>  }
>  EXPORT_SYMBOL_GPL(nfs4_schedule_stateid_recovery);
>  

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 3/4] NFSv4: clp->cl_cons_state < 0 signifies an invalid nfs_client

[Trond Myklebust]

On Tue, 2025-03-25 at 13:59 -0400, Jeff Layton wrote:
> On Tue, 2025-03-25 at 12:17 -0400, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > If someone calls nfs_mark_client_ready(clp, status) with a negative
> > value for status, then that should signal that the nfs_client is no
> > longer valid.
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/nfs4state.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > index 542cdf71229f..738eb2789266 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 (clnt->cl_shutdown || clp->cl_cons_state < 0)
> 
> Would it be simpler to just set cl_shutdown when this occurs instead
> of
> having to check cl_cons_state as well?

Do we need the check for clnt->cl_shutdown at all here? I'd expect any
caller of this function to already hold a reference to the client,
which means that the RPC client should still be up.

I'm a little suspicious of the check in nfs41_sequence_call_done() too.

> 
> >  		return;
> >  
> >  	set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state);
> > @@ -1403,7 +1403,7 @@ int nfs4_schedule_stateid_recovery(const
> > struct nfs_server *server, struct nfs4_
> >  	dprintk("%s: scheduling stateid recovery for server %s\n",
> > __func__,
> >  			clp->cl_hostname);
> >  	nfs4_schedule_state_manager(clp);
> > -	return 0;
> > +	return clp->cl_cons_state < 0 ? clp->cl_cons_state : 0;
> >  }
> >  EXPORT_SYMBOL_GPL(nfs4_schedule_stateid_recovery);
> >  
> 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v2 3/4] NFSv4: clp->cl_cons_state < 0 signifies an invalid nfs_client

[Jeff Layton]

On Tue, 2025-03-25 at 18:48 +0000, Trond Myklebust wrote:
> On Tue, 2025-03-25 at 13:59 -0400, Jeff Layton wrote:
> > On Tue, 2025-03-25 at 12:17 -0400, trondmy@kernel.org wrote:
> > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > 
> > > If someone calls nfs_mark_client_ready(clp, status) with a negative
> > > value for status, then that should signal that the nfs_client is no
> > > longer valid.
> > > 
> > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > ---
> > >  fs/nfs/nfs4state.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > > index 542cdf71229f..738eb2789266 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 (clnt->cl_shutdown || clp->cl_cons_state < 0)
> > 
> > Would it be simpler to just set cl_shutdown when this occurs instead
> > of
> > having to check cl_cons_state as well?
> 
> Do we need the check for clnt->cl_shutdown at all here? I'd expect any
> caller of this function to already hold a reference to the client,
> which means that the RPC client should still be up.

Not necessarily? Just because you hold a reference to the rpc_clnt
doesn't mean that it's still up, AFAIU.

For instance, if you end up using the "shutdown" file in sysfs, any RPC
still in flight will hold a reference to the client. Writing to
"shutdown" will set cl_shutdown to 1 and then cancel all the RPCs, but
there is at least a window of time where we have an elevated refcount
but the client is no longer valid.


> 
> I'm a little suspicious of the check in nfs41_sequence_call_done() too.
> 

Me too. I think this is probably an indicator that we need to carefully
audit how cl_shutdown is used and clarify what it means. Luckily there
are only a handful of places that reference it:

The call_start check is fine I thinkhhuhdljkfjltkuddjrig, though maybe
we should add cl_shutdown checks in later states? The other places that
check it come from this commit:

    6ad477a69ad8 NFSv4: Clean up some shutdown loops

Should we convert both of those checks to look at clp->cl_cons_state
instead?

> > 
> > >  		return;
> > >  
> > >  	set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state);
> > > @@ -1403,7 +1403,7 @@ int nfs4_schedule_stateid_recovery(const
> > > struct nfs_server *server, struct nfs4_
> > >  	dprintk("%s: scheduling stateid recovery for server %s\n",
> > > __func__,
> > >  			clp->cl_hostname);
> > >  	nfs4_schedule_state_manager(clp);
> > > -	return 0;
> > > +	return clp->cl_cons_state < 0 ? clp->cl_cons_state : 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(nfs4_schedule_stateid_recovery);
> > >  
> > 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 3/4] NFSv4: clp->cl_cons_state < 0 signifies an invalid nfs_client

[Trond Myklebust]

On Tue, 2025-03-25 at 15:44 -0400, Jeff Layton wrote:
> On Tue, 2025-03-25 at 18:48 +0000, Trond Myklebust wrote:
> > On Tue, 2025-03-25 at 13:59 -0400, Jeff Layton wrote:
> > > On Tue, 2025-03-25 at 12:17 -0400, trondmy@kernel.org wrote:
> > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > 
> > > > If someone calls nfs_mark_client_ready(clp, status) with a
> > > > negative
> > > > value for status, then that should signal that the nfs_client
> > > > is no
> > > > longer valid.
> > > > 
> > > > Signed-off-by: Trond Myklebust
> > > > <trond.myklebust@hammerspace.com>
> > > > ---
> > > >  fs/nfs/nfs4state.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > > > index 542cdf71229f..738eb2789266 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 (clnt->cl_shutdown || clp->cl_cons_state < 0)
> > > 
> > > Would it be simpler to just set cl_shutdown when this occurs
> > > instead
> > > of
> > > having to check cl_cons_state as well?
> > 
> > Do we need the check for clnt->cl_shutdown at all here? I'd expect
> > any
> > caller of this function to already hold a reference to the client,
> > which means that the RPC client should still be up.
> 
> Not necessarily? Just because you hold a reference to the rpc_clnt
> doesn't mean that it's still up, AFAIU.
> 
> For instance, if you end up using the "shutdown" file in sysfs, any
> RPC
> still in flight will hold a reference to the client. Writing to
> "shutdown" will set cl_shutdown to 1 and then cancel all the RPCs,
> but
> there is at least a window of time where we have an elevated refcount
> but the client is no longer valid.

The shutdown of the nfs_client RPC client happens in nfs_free_client().

Oh wait... Crap... Why is a per-nfs_server function like
shutdown_store() reaching into the nfs_client? That's borked and needs
to be fixed.

> 
> 
> > 
> > I'm a little suspicious of the check in nfs41_sequence_call_done()
> > too.
> > 
> 
> Me too. I think this is probably an indicator that we need to
> carefully
> audit how cl_shutdown is used and clarify what it means. Luckily
> there
> are only a handful of places that reference it:
> 
> The call_start check is fine I thinkhhuhdljkfjltkuddjrig, though
> maybe
> we should add cl_shutdown checks in later states? The other places
> that
> check it come from this commit:
> 
>     6ad477a69ad8 NFSv4: Clean up some shutdown loops
> 
> Should we convert both of those checks to look at clp->cl_cons_state
> instead?

Yes.

> 
> > > 
> > > >  		return;
> > > >  
> > > >  	set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state);
> > > > @@ -1403,7 +1403,7 @@ int nfs4_schedule_stateid_recovery(const
> > > > struct nfs_server *server, struct nfs4_
> > > >  	dprintk("%s: scheduling stateid recovery for server
> > > > %s\n",
> > > > __func__,
> > > >  			clp->cl_hostname);
> > > >  	nfs4_schedule_state_manager(clp);
> > > > -	return 0;
> > > > +	return clp->cl_cons_state < 0 ? clp->cl_cons_state :
> > > > 0;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(nfs4_schedule_stateid_recovery);
> > > >  
> > > 
> > 
> > -- 
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > trond.myklebust@hammerspace.com
> > 
> > 
> 

-- 
Trond Myklebust 
CTO, Hammerspace Inc 
1900 S Norfolk St, Suite 350 - #45 
San Mateo, CA 94403 
​
www.hammerspace.com

[PATCH v2 4/4] NFSv4: Treat ENETUNREACH errors as fatal for state recovery

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

If a containerised process is killed and causes an ENETUNREACH or
ENETDOWN error to be propagated to the state manager, then mark the
nfs_client as being dead so that we don't loop in functions that are
expecting recovery to succeed.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs4state.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 738eb2789266..14ba3f96e6fc 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -2739,7 +2739,15 @@ static void nfs4_state_manager(struct nfs_client *clp)
 	pr_warn_ratelimited("NFS: state manager%s%s failed on NFSv4 server %s"
 			" with error %d\n", section_sep, section,
 			clp->cl_hostname, -status);
-	ssleep(1);
+	switch (status) {
+	case -ENETDOWN:
+	case -ENETUNREACH:
+		nfs_mark_client_ready(clp, -EIO);
+		break;
+	default:
+		ssleep(1);
+		break;
+	}
 out_drain:
 	memalloc_nofs_restore(memflags);
 	nfs4_end_drain_session(clp);
-- 
2.49.0

Re: [PATCH v2 4/4] NFSv4: Treat ENETUNREACH errors as fatal for state recovery

[Jeff Layton]

On Tue, 2025-03-25 at 12:17 -0400, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> If a containerised process is killed and causes an ENETUNREACH or
> ENETDOWN error to be propagated to the state manager, then mark the
> nfs_client as being dead so that we don't loop in functions that are
> expecting recovery to succeed.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/nfs4state.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 738eb2789266..14ba3f96e6fc 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -2739,7 +2739,15 @@ static void nfs4_state_manager(struct nfs_client *clp)
>  	pr_warn_ratelimited("NFS: state manager%s%s failed on NFSv4 server %s"
>  			" with error %d\n", section_sep, section,
>  			clp->cl_hostname, -status);
> -	ssleep(1);
> +	switch (status) {
> +	case -ENETDOWN:
> +	case -ENETUNREACH:
> +		nfs_mark_client_ready(clp, -EIO);
> +		break;

Should this be conditional on clnt->cl_netunreach_fatal being true?
 
> +	default:
> +		ssleep(1);
> +		break;
> +	}
>  out_drain:
>  	memalloc_nofs_restore(memflags);
>  	nfs4_end_drain_session(clp);

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 4/4] NFSv4: Treat ENETUNREACH errors as fatal for state recovery

[Trond Myklebust]

On Tue, 2025-03-25 at 14:04 -0400, Jeff Layton wrote:
> On Tue, 2025-03-25 at 12:17 -0400, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > If a containerised process is killed and causes an ENETUNREACH or
> > ENETDOWN error to be propagated to the state manager, then mark the
> > nfs_client as being dead so that we don't loop in functions that
> > are
> > expecting recovery to succeed.
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/nfs4state.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > index 738eb2789266..14ba3f96e6fc 100644
> > --- a/fs/nfs/nfs4state.c
> > +++ b/fs/nfs/nfs4state.c
> > @@ -2739,7 +2739,15 @@ static void nfs4_state_manager(struct
> > nfs_client *clp)
> >  	pr_warn_ratelimited("NFS: state manager%s%s failed on
> > NFSv4 server %s"
> >  			" with error %d\n", section_sep, section,
> >  			clp->cl_hostname, -status);
> > -	ssleep(1);
> > +	switch (status) {
> > +	case -ENETDOWN:
> > +	case -ENETUNREACH:
> > +		nfs_mark_client_ready(clp, -EIO);
> > +		break;
> 
> Should this be conditional on clnt->cl_netunreach_fatal being true?

I should hope not. We shouldn't ever be seeing these errors here if it
is false.

>  
> > +	default:
> > +		ssleep(1);
> > +		break;
> > +	}
> >  out_drain:
> >  	memalloc_nofs_restore(memflags);
> >  	nfs4_end_drain_session(clp);
> 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v2 4/4] NFSv4: Treat ENETUNREACH errors as fatal for state recovery

[Jeff Layton]

On Tue, 2025-03-25 at 18:50 +0000, Trond Myklebust wrote:
> On Tue, 2025-03-25 at 14:04 -0400, Jeff Layton wrote:
> > On Tue, 2025-03-25 at 12:17 -0400, trondmy@kernel.org wrote:
> > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > 
> > > If a containerised process is killed and causes an ENETUNREACH or
> > > ENETDOWN error to be propagated to the state manager, then mark the
> > > nfs_client as being dead so that we don't loop in functions that
> > > are
> > > expecting recovery to succeed.
> > > 
> > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > ---
> > >  fs/nfs/nfs4state.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > > index 738eb2789266..14ba3f96e6fc 100644
> > > --- a/fs/nfs/nfs4state.c
> > > +++ b/fs/nfs/nfs4state.c
> > > @@ -2739,7 +2739,15 @@ static void nfs4_state_manager(struct
> > > nfs_client *clp)
> > >  	pr_warn_ratelimited("NFS: state manager%s%s failed on
> > > NFSv4 server %s"
> > >  			" with error %d\n", section_sep, section,
> > >  			clp->cl_hostname, -status);
> > > -	ssleep(1);
> > > +	switch (status) {
> > > +	case -ENETDOWN:
> > > +	case -ENETUNREACH:
> > > +		nfs_mark_client_ready(clp, -EIO);
> > > +		break;
> > 
> > Should this be conditional on clnt->cl_netunreach_fatal being true?
> 
> I should hope not. We shouldn't ever be seeing these errors here if it
> is false.
> 

Oh right, the only way that these errors bubble up from sunrpc layer is
if this option is enabled. You can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Concerns about ENETUNREACH patch series Re: [PATCH v2 0/4] Ensure that ENETUNREACH terminates state recovery

[Lionel Cons]

On Tue, 25 Mar 2025 at 17:19, <trondmy@kernel.org> wrote:
>
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> With the recent patch series that caused containerised mounts which
> return ENETUNREACH or ENETDOWN errors to report fatal errors, we also
> want to ensure that the state manager thread also triggers fatal errors
> in the processes or threads that are waiting for recovery to complete.
>
> ---
> v2:
>  - Return EIO instead of ENETUNREACH in nfs4_wait_clnt_recover()
>
> Trond Myklebust (4):
>   SUNRPC: rpcbind should never reset the port to the value '0'
>   SUNRPC: rpc_clnt_set_transport() must not change the autobind setting
>   NFSv4: clp->cl_cons_state < 0 signifies an invalid nfs_client
>   NFSv4: Treat ENETUNREACH errors as fatal for state recovery
>
>  fs/nfs/nfs4state.c     | 14 +++++++++++---
>  net/sunrpc/clnt.c      |  3 ---
>  net/sunrpc/rpcb_clnt.c |  5 +++--
>  3 files changed, 14 insertions(+), 8 deletions(-)

1. Can this "ENETUNREACH or ENETDOWN are fatal" feature be turned off?

2. We have concerns about this feature - what will happen if a switch
or router gets rebooted? What will happen if you unplug your laptop?
What will happen if you enable/disable your VPN software on a machine
or container? What will happen if you switch WIFIs on your laptop?

All these scenarios will trigger a temporary ENETUNREACH or ENETDOWN,
and should NOT be fatal for mounts or containers.

Lionel

Re: Concerns about ENETUNREACH patch series Re: [PATCH v2 0/4] Ensure that ENETUNREACH terminates state recovery

[Trond Myklebust]

On Tue, 2025-03-25 at 20:40 +0100, Lionel Cons wrote:
> On Tue, 25 Mar 2025 at 17:19, <trondmy@kernel.org> wrote:
> > 
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > With the recent patch series that caused containerised mounts which
> > return ENETUNREACH or ENETDOWN errors to report fatal errors, we
> > also
> > want to ensure that the state manager thread also triggers fatal
> > errors
> > in the processes or threads that are waiting for recovery to
> > complete.
> > 
> > ---
> > v2:
> >  - Return EIO instead of ENETUNREACH in nfs4_wait_clnt_recover()
> > 
> > Trond Myklebust (4):
> >   SUNRPC: rpcbind should never reset the port to the value '0'
> >   SUNRPC: rpc_clnt_set_transport() must not change the autobind
> > setting
> >   NFSv4: clp->cl_cons_state < 0 signifies an invalid nfs_client
> >   NFSv4: Treat ENETUNREACH errors as fatal for state recovery
> > 
> >  fs/nfs/nfs4state.c     | 14 +++++++++++---
> >  net/sunrpc/clnt.c      |  3 ---
> >  net/sunrpc/rpcb_clnt.c |  5 +++--
> >  3 files changed, 14 insertions(+), 8 deletions(-)
> 
> 1. Can this "ENETUNREACH or ENETDOWN are fatal" feature be turned
> off?
> 
> 2. We have concerns about this feature - what will happen if a switch
> or router gets rebooted? What will happen if you unplug your laptop?
> What will happen if you enable/disable your VPN software on a machine
> or container? What will happen if you switch WIFIs on your laptop?
> 
> All these scenarios will trigger a temporary ENETUNREACH or ENETDOWN,
> and should NOT be fatal for mounts or containers.

TL;DR: Yes, you can turn it off using a mount option, but we preserve
the existing hard mount behaviour unless you are actually mounting in a
container.

See the description in the first series of patches:
https://lore.kernel.org/linux-nfs/2a37ba5a-f212-4456-a7c1-3f96b1148b3b@oracle.com/T/#mf3df4516c5fcb0ef22c1c1c6f5433535f4d4805a



-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com