Re: [PATCH 1/4] svcrpc: never clear XPT_BUSY on dead xprt

[Neil Brown <neilb@suse.de>]

On Mon, 25 Oct 2010 20:30:08 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Oct 25, 2010 at 08:28:58PM -0400, J. Bruce Fields wrote:
> > On Mon, Oct 25, 2010 at 08:11:45PM -0400, J. Bruce Fields wrote:
> > > On Tue, Oct 26, 2010 at 10:54:47AM +1100, Neil Brown wrote:
> > > > Note that once we always set XPT_BUSY and it stays set.  So we call
> > > > svc_delete_xprt instread of svc_close_xprt.
> > > > 
> > > > Maybe we don't actually need to list_del_init - both the pool and the xprt
> > > > will soon be freed and if there is linkage between them, who cares??
> > > > In that case we wouldn't need to for_each_pool after all ???
> > 
> > So, untested:
> 
> Ditto.  But, hey, it deletes more code, it must be good.
> 
> --b.
> 
> commit 99401f6f6c3d0782ef79bb37749ace2accd4c79a
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Mon Oct 25 20:24:48 2010 -0400
> 
>     svcrpc: simplify svc_close_xprt, fix minor race
>     
>     We're assuming that in the XPT_BUSY case, somebody else will take care
>     of the close.  But that's not necessarily the case (e.g. if
>     svc_xprt_enqueue() exits due to a lack of write space).  So the close
>     could in theory be delayed indefinitely.  We should be calling
>     svc_xprt_enqueue() after every set of XPT_CLOSE.
>     
>     Also with svc_close_all() no longer relying on svc_close, it no longer
>     needs to be able to immediately do the svc_delete_xprt() itself.
>     
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 8c018df..dfee123 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -928,11 +928,7 @@ void svc_delete_xprt(struct svc_xprt *xprt)
>  void svc_close_xprt(struct svc_xprt *xprt)
>  {
>  	set_bit(XPT_CLOSE, &xprt->xpt_flags);
> -	if (test_and_set_bit(XPT_BUSY, &xprt->xpt_flags))
> -		/* someone else will have to effect the close */
> -		return;
> -
> -	svc_delete_xprt(xprt);
> +	svc_xprt_enqueue(xprt);
>  }
>  EXPORT_SYMBOL_GPL(svc_close_xprt);
>  

I'm not quite so sure about this one.

svc_close_xprt gets called when user-space writes some magic string to some
magic file.  This could be done when there are no active nfsd threads.
It is a bit far fetched, but you could tell nfsd to open a socket, then tell
it to close it, before telling it to start any threads.

In that case the socket would stay open until the first thread was started.

So ... not quite convinced (much as the code reduction is nice!).

NeilBrown