2021-10-06 14:18:47

by Benjamin Coddington

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

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 <[email protected]>
---
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

2021-10-06 14:23:05

by Benjamin Coddington

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

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 <[email protected]>
> ---
> 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

2021-10-06 14:25:58

by Chuck Lever III

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

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 <[email protected]> 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 <[email protected]>
> ---
> 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

2021-10-06 14:33:55

by J. Bruce Fields

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

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 <[email protected]>
> ---
> 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

2021-10-06 14:43:46

by Benjamin Coddington

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

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

2021-10-06 14:47:13

by J. Bruce Fields

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

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.

2021-10-06 17:17:28

by Benjamin Coddington

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

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 <[email protected]>
---
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

2021-10-06 17:21:33

by Benjamin Coddington

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

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 <[email protected]>
---
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

2021-10-06 17:28:38

by Chuck Lever III

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

> On Oct 6, 2021, at 1:20 PM, Benjamin Coddington <[email protected]> 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 <[email protected]>

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