Re: [PATCH][RFC] network splice receive v2

[Jens Axboe <jens.axboe@oracle.com>]

On Fri, Jun 15 2007, Evgeniy Polyakov wrote:
> On Fri, Jun 15, 2007 at 10:43:18AM +0200, Jens Axboe (jens.axboe@oracle.com) wrote:
> > > So, things turned down to be not in the __splice_from_pipe(), but
> > > splice_to_pipe(). Attached patch fixes a leak for me.
> > > It was tested with different data files and all were received correctly.
> > > 
> > > Signed-off-by: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
> > > 
> > > diff --git a/fs/splice.c b/fs/splice.c
> > > index bc481f1..365bfd9 100644
> > > --- a/fs/splice.c
> > > +++ b/fs/splice.c
> > > @@ -211,8 +211,6 @@ ssize_t splice_to_pipe(struct pipe_inode_info *pipe,
> > >  				break;
> > >  			if (pipe->nrbufs < PIPE_BUFFERS)
> > >  				continue;
> > > -
> > > -			break;
> > >  		}
> > >  
> > >  		if (spd->flags & SPLICE_F_NONBLOCK) {
> > > 
> > 
> > Hmm, curious. If we hit that location, then two conditions are true:
> > 
> > - Pipe is full
> > - We transferred some data
> 
> Yep.
> 
> > if you remove the break, then you'll end up blocking in pipe_wait()
> > (unless you have SPLICE_F_NONBLOCK also set). And we don't want to block
> > waiting for more room, if we already transferred some data. In that case
> > we just want to return a short splice. Looking at pipe_write(), it'll
> > block as well though. Just doesn't seem optimal to me, but...
> > 
> > So the question is why would doing the break there cause a leak? I just
> > don't yet see how it can happen, I'd love to fix that condition instead.
> > For the case you describe, we should have page_nr == 1 and spd->nr_pages
> > == 2. Is the:
> > 
> >         while (page_nr < spd->nr_pages)
> >                 spd->spd_release(spd, page_nr++);
> > 
> > not dropping the right reference?
> 
> Both spd->nr_pages and page_nr are equal to 1. When spd->nr_pages
> was 2 there was only 1 free slot in pipe_buffer.

Ah duh, indeed, it is a dumb bug indeed. This should work.

diff --git a/fs/splice.c b/fs/splice.c
index 89871c6..5327cbf 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -172,6 +172,7 @@ static const struct pipe_buf_operations user_page_pipe_buf_ops = {
 ssize_t splice_to_pipe(struct pipe_inode_info *pipe,
 		       struct splice_pipe_desc *spd)
 {
+	unsigned int spd_pages = spd->nr_pages;
 	int ret, do_wakeup, page_nr;
 
 	ret = 0;
@@ -252,7 +253,7 @@ ssize_t splice_to_pipe(struct pipe_inode_info *pipe,
 		}
 	}
 
-	while (page_nr < spd->nr_pages)
+	while (page_nr < spd_pages)
 		spd->spd_release(spd, page_nr++);
 
 	return ret;

-- 
Jens Axboe