Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 6 Oct 2021 10:33:35 -0400
From:   "J. Bruce Fields" <bfields@fieldses.org>
To:     Benjamin Coddington <bcodding@redhat.com>
Cc:     chuck.lever@oracle.com, linux-nfs@vger.kernel.org
Subject: Re: [PATCH] NFSD: Keep existing listners on portlist error
Message-ID: <20211006143335.GA15343@fieldses.org>
References: <45b916f1aa3fb7c059a574f61188a8f2f615410e.1633529847.git.bcodding@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <45b916f1aa3fb7c059a574f61188a8f2f615410e.1633529847.git.bcodding@redhat.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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