[PATCH] NFSD: Keep existing listners on portlist error

[PATCH] NFSD: Keep existing listners on portlist error

[Benjamin Coddington]

If nfsd has existing listening sockets without any processes, then an error
returned from svc_create_xprt() for an additional transport will remove
those existing listeners.  We're seeing this in practice when userspace
attempts to create rpcrdma transports without having the rpcrdma modules
present before creating nfsd kernel processes.  Fix this by checking for
existing sockets before callingn nfsd_destroy().

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfsd/nfsctl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index c2c3d9077dc5..df4613a4924c 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -793,7 +793,10 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
 		svc_xprt_put(xprt);
 	}
 out_err:
-	nfsd_destroy(net);
+	if (list_empty(&nn->nfsd_serv->sv_permsocks))
+		nfsd_destroy(net);
+	else
+		nn->nfsd_serv->sv_nrthreads--;
 	return err;
 }
 
-- 
2.30.2

Re: [PATCH] NFSD: Keep existing listners on portlist error

[Benjamin Coddington]

On 6 Oct 2021, at 10:18, Benjamin Coddington wrote:

> If nfsd has existing listening sockets without any processes, then an 
> error
> returned from svc_create_xprt() for an additional transport will 
> remove
> those existing listeners.  We're seeing this in practice when 
> userspace
> attempts to create rpcrdma transports without having the rpcrdma 
> modules
> present before creating nfsd kernel processes.  Fix this by checking 
> for
> existing sockets before callingn nfsd_destroy().
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfsd/nfsctl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index c2c3d9077dc5..df4613a4924c 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -793,7 +793,10 @@ static ssize_t __write_ports_addxprt(char *buf, 
> struct net *net, const struct cr
>  		svc_xprt_put(xprt);
>  	}
>  out_err:
> -	nfsd_destroy(net);
> +	if (list_empty(&nn->nfsd_serv->sv_permsocks))
> +		nfsd_destroy(net);
> +	else
> +		nn->nfsd_serv->sv_nrthreads--;

Eh -- ignore this one, needs a v2 since we might decrement sv_nrthreads 
twice for the INET6 case..

Ben

Re: [PATCH] NFSD: Keep existing listners on portlist error

[Chuck Lever III]

Bruce, if this looks OK to you I can push it via 5.15-rc


> On Oct 6, 2021, at 10:18 AM, Benjamin Coddington <bcodding@redhat.com> wrote:
> 
> If nfsd has existing listening sockets without any processes, then an error
> returned from svc_create_xprt() for an additional transport will remove
> those existing listeners.  We're seeing this in practice when userspace
> attempts to create rpcrdma transports without having the rpcrdma modules
> present before creating nfsd kernel processes.  Fix this by checking for
> existing sockets before callingn nfsd_destroy().
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
> fs/nfsd/nfsctl.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index c2c3d9077dc5..df4613a4924c 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -793,7 +793,10 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
> 		svc_xprt_put(xprt);
> 	}
> out_err:
> -	nfsd_destroy(net);
> +	if (list_empty(&nn->nfsd_serv->sv_permsocks))
> +		nfsd_destroy(net);
> +	else
> +		nn->nfsd_serv->sv_nrthreads--;
> 	return err;
> }
> 
> -- 
> 2.30.2
> 

--
Chuck Lever

Re: [PATCH] NFSD: Keep existing listners on portlist error

[J. Bruce Fields]

On Wed, Oct 06, 2021 at 10:18:05AM -0400, Benjamin Coddington wrote:
> If nfsd has existing listening sockets without any processes, then an error
> returned from svc_create_xprt() for an additional transport will remove
> those existing listeners.  We're seeing this in practice when userspace
> attempts to create rpcrdma transports without having the rpcrdma modules
> present before creating nfsd kernel processes.  Fix this by checking for
> existing sockets before callingn nfsd_destroy().

That seems like an improvement.

I'm curious, though, what the rpc.nfsd behavior is on partial failure.
And what do we want it to be?

If a user runs rpc.nfsd expecting it to start up tcp and rdma, but rdma
fails, do we want rpc.nfsd to succeed or fail?  Should it exit with nfsd
running or not?

--b.

> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfsd/nfsctl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index c2c3d9077dc5..df4613a4924c 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -793,7 +793,10 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
>  		svc_xprt_put(xprt);
>  	}
>  out_err:
> -	nfsd_destroy(net);
> +	if (list_empty(&nn->nfsd_serv->sv_permsocks))
> +		nfsd_destroy(net);
> +	else
> +		nn->nfsd_serv->sv_nrthreads--;
>  	return err;
>  }
>  
> -- 
> 2.30.2

Re: [PATCH] NFSD: Keep existing listners on portlist error

[Benjamin Coddington]

On 6 Oct 2021, at 10:33, J. Bruce Fields wrote:

> On Wed, Oct 06, 2021 at 10:18:05AM -0400, Benjamin Coddington wrote:
>> If nfsd has existing listening sockets without any processes, then an 
>> error
>> returned from svc_create_xprt() for an additional transport will 
>> remove
>> those existing listeners.  We're seeing this in practice when 
>> userspace
>> attempts to create rpcrdma transports without having the rpcrdma 
>> modules
>> present before creating nfsd kernel processes.  Fix this by checking 
>> for
>> existing sockets before callingn nfsd_destroy().
>
> That seems like an improvement.
>
> I'm curious, though, what the rpc.nfsd behavior is on partial failure.
> And what do we want it to be?
>
> If a user runs rpc.nfsd expecting it to start up tcp and rdma, but 
> rdma
> fails, do we want rpc.nfsd to succeed or fail?  Should it exit with 
> nfsd
> running or not?

I lean toward having it fail - but I think that's a different patch for
rpc.nfsd.  Right now rpc.nfsd exists without error, but you end up 
without
any listeners at all.

Do you want a patch for rpc.nfsd instead?

Ben

Re: [PATCH] NFSD: Keep existing listners on portlist error

[J. Bruce Fields]

On Wed, Oct 06, 2021 at 10:43:14AM -0400, Benjamin Coddington wrote:
> On 6 Oct 2021, at 10:33, J. Bruce Fields wrote:
> 
> >On Wed, Oct 06, 2021 at 10:18:05AM -0400, Benjamin Coddington wrote:
> >>If nfsd has existing listening sockets without any processes,
> >>then an error
> >>returned from svc_create_xprt() for an additional transport will
> >>remove
> >>those existing listeners.  We're seeing this in practice when
> >>userspace
> >>attempts to create rpcrdma transports without having the rpcrdma
> >>modules
> >>present before creating nfsd kernel processes.  Fix this by
> >>checking for
> >>existing sockets before callingn nfsd_destroy().
> >
> >That seems like an improvement.
> >
> >I'm curious, though, what the rpc.nfsd behavior is on partial failure.
> >And what do we want it to be?
> >
> >If a user runs rpc.nfsd expecting it to start up tcp and rdma, but
> >rdma
> >fails, do we want rpc.nfsd to succeed or fail?  Should it exit
> >with nfsd
> >running or not?
> 
> I lean toward having it fail - but I think that's a different patch for
> rpc.nfsd.  Right now rpc.nfsd exists without error, but you end up
> without
> any listeners at all.
> 
> Do you want a patch for rpc.nfsd instead?

I think we need the kernel fix.  I agree that it's weird to shut down
the server on a failure to add a listener--better to leave usrespace to
decide if it wants to do that.

--b.

[PATCH] NFSD: Keep existing listners on portlist error

[Benjamin Coddington]

If nfsd has existing listening sockets without any processes, then an error
returned from svc_create_xprt() for an additional transport will remove
those existing listeners.  We're seeing this in practice when userspace
attempts to create rpcrdma transports without having the rpcrdma modules
present before creating nfsd kernel processes.  Fix this by checking for
existing sockets before callingn nfsd_destroy().

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfsd/nfsctl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index c2c3d9077dc5..696a217255fc 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -793,7 +793,10 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
 		svc_xprt_put(xprt);
 	}
 out_err:
-	nfsd_destroy(net);
+	if (!list_empty(&nn->nfsd_serv->sv_permsocks))
+		nn->nfsd_serv->sv_nrthreads--;
+	 else
+		nfsd_destroy(net);
 	return err;
 }
 
-- 
2.30.2

[PATCH v2] NFSD: Keep existing listeners on portlist error

[Benjamin Coddington]

Why V2: further testing to verify INET6 handling, fix spelling mistakes

8<------------------------------------------------------------------------

If nfsd has existing listening sockets without any processes, then an error
returned from svc_create_xprt() for an additional transport will remove
those existing listeners.  We're seeing this in practice when userspace
attempts to create rpcrdma transports without having the rpcrdma modules
present before creating nfsd kernel processes.  Fix this by checking for
existing sockets before calling nfsd_destroy().

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfsd/nfsctl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index c2c3d9077dc5..696a217255fc 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -793,7 +793,10 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
 		svc_xprt_put(xprt);
 	}
 out_err:
-	nfsd_destroy(net);
+	if (!list_empty(&nn->nfsd_serv->sv_permsocks))
+		nn->nfsd_serv->sv_nrthreads--;
+	 else
+		nfsd_destroy(net);
 	return err;
 }
 
-- 
2.30.2

Re: [PATCH v2] NFSD: Keep existing listeners on portlist error

[Chuck Lever III]

> On Oct 6, 2021, at 1:20 PM, Benjamin Coddington <bcodding@redhat.com> wrote:
> 
> Why V2: further testing to verify INET6 handling, fix spelling mistakes
> 
> 8<------------------------------------------------------------------------
> 
> If nfsd has existing listening sockets without any processes, then an error
> returned from svc_create_xprt() for an additional transport will remove
> those existing listeners.  We're seeing this in practice when userspace
> attempts to create rpcrdma transports without having the rpcrdma modules
> present before creating nfsd kernel processes.  Fix this by checking for
> existing sockets before calling nfsd_destroy().
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>

Thanks, applied to the for-next topic branch at

git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git


> ---
> fs/nfsd/nfsctl.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index c2c3d9077dc5..696a217255fc 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -793,7 +793,10 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
> 		svc_xprt_put(xprt);
> 	}
> out_err:
> -	nfsd_destroy(net);
> +	if (!list_empty(&nn->nfsd_serv->sv_permsocks))
> +		nn->nfsd_serv->sv_nrthreads--;
> +	 else
> +		nfsd_destroy(net);
> 	return err;
> }
> 
> -- 
> 2.30.2
> 

--
Chuck Lever