Re: [PATCH 5/9] rpc: call release_pipe only on last close

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Nov 10, 2008 at 04:18:03PM -0500, Trond Myklebust wrote:
> On Mon, 2008-11-10 at 15:26 -0500, J. Bruce Fields wrote:
> > Yep.  And when we're in that situation, last_close is true, so
> > 
> > 	if (last_close && ops->release_pipe)
> > 		ops->release_pipe(inode)
> > 
> > does ensure that calls to the ->open_pipe() and ->release_pipe() methods
> > balance, as originally claimed.
> > 
> > Maybe it'd be clearer to call that variable "still_open", or something?
> > 
> > 	still_open = rpci->nreaders != 0 || rpci->nwriters != 0;
> > 	...
> > 	if (still_open && ops->release_pipe)
> > 		ops->release_pipe(inode)
> 
> Call it 'need_release'. It is definitely confusing right now...

Sure.

--b.

commit 572df09a77f3cfdca9d5fba4ca736919294eb177
Author: J. Bruce Fields <bfields@citi.umich.edu>
Date:   Fri Nov 7 18:53:24 2008 -0500

    rpc: call release_pipe only on last close
    
    I can't see any reason we need to call this until either the kernel or
    the last gssd closes the pipe.
    
    Also, this allows to guarantee that open_pipe and release_pipe are
    called strictly in pairs; open_pipe on gssd's first open, release_pipe
    on gssd's last close (or on the close of the kernel side of the pipe, if
    that comes first).
    
    That will make it very easy for the gss code to keep track of which
    pipes gssd is using.
    
    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 4171ab7..a68aaac 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -126,13 +126,14 @@ rpc_close_pipes(struct inode *inode)
 {
 	struct rpc_inode *rpci = RPC_I(inode);
 	struct rpc_pipe_ops *ops;
+	int need_release;
 
 	mutex_lock(&inode->i_mutex);
 	ops = rpci->ops;
 	if (ops != NULL) {
 		LIST_HEAD(free_list);
-
 		spin_lock(&inode->i_lock);
+		need_release = rpci->nreaders != 0 || rpci->nwriters != 0;
 		rpci->nreaders = 0;
 		list_splice_init(&rpci->in_upcall, &free_list);
 		list_splice_init(&rpci->pipe, &free_list);
@@ -141,7 +142,7 @@ rpc_close_pipes(struct inode *inode)
 		spin_unlock(&inode->i_lock);
 		rpc_purge_list(rpci, &free_list, ops->destroy_msg, -EPIPE);
 		rpci->nwriters = 0;
-		if (ops->release_pipe)
+		if (need_release && ops->release_pipe)
 			ops->release_pipe(inode);
 		cancel_delayed_work_sync(&rpci->queue_timeout);
 	}
@@ -169,12 +170,13 @@ static int
 rpc_pipe_open(struct inode *inode, struct file *filp)
 {
 	struct rpc_inode *rpci = RPC_I(inode);
-	int first_open = rpci->nreaders == 0 && rpci->nwriters == 0;
+	int first_open;
 	int res = -ENXIO;
 
 	mutex_lock(&inode->i_mutex);
 	if (rpci->ops == NULL)
 		goto out;
+	first_open = rpci->nreaders == 0 && rpci->nwriters == 0;
 	if (first_open && rpci->ops->open_pipe) {
 		res = rpci->ops->open_pipe(inode);
 		if (res)
@@ -195,6 +197,7 @@ rpc_pipe_release(struct inode *inode, struct file *filp)
 {
 	struct rpc_inode *rpci = RPC_I(inode);
 	struct rpc_pipe_msg *msg;
+	int last_close;
 
 	mutex_lock(&inode->i_mutex);
 	if (rpci->ops == NULL)
@@ -221,7 +224,8 @@ rpc_pipe_release(struct inode *inode, struct file *filp)
 					rpci->ops->destroy_msg, -EAGAIN);
 		}
 	}
-	if (rpci->ops->release_pipe)
+	last_close = rpci->nwriters == 0 && rpci->nreaders == 0;
+	if (last_close && rpci->ops->release_pipe)
 		rpci->ops->release_pipe(inode);
 out:
 	mutex_unlock(&inode->i_mutex);