Re: blktrace/relay/s390: Oops in subbuf_splice_actor

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

On Mon, Apr 07 2008, Tom Zanussi wrote:
> 
> On Fri, 2008-03-14 at 14:22 +0100, Christof Schmitt wrote:
> > plain text document attachment (Re: blktrace/relay/s390: Oops in
> > subbuf_splice_actor.eml)
> > On Fri, Mar 14, 2008 at 02:10:07PM +0100, Jens Axboe wrote:
> > > On Fri, Mar 14 2008, Christof Schmitt wrote:
> > > > On Fri, Mar 14, 2008 at 12:58:03PM +0100, Jens Axboe wrote:
> > > > > That is indeed a bug, does this work for you?
> > > > > 
> > > > > diff --git a/kernel/relay.c b/kernel/relay.c
> > > > > index d080b9d..39d1fa8 100644
> > > > > --- a/kernel/relay.c
> > > > > +++ b/kernel/relay.c
> > > > > @@ -1066,7 +1066,7 @@ static int subbuf_splice_actor(struct file *in,
> > > > >  			       unsigned int flags,
> > > > >  			       int *nonpad_ret)
> > > > >  {
> > > > > -	unsigned int pidx, poff, total_len, subbuf_pages, ret;
> > > > > +	unsigned int pidx, poff, total_len, subbuf_pages, nr_pages, ret;
> > > > >  	struct rchan_buf *rbuf = in->private_data;
> > > > >  	unsigned int subbuf_size = rbuf->chan->subbuf_size;
> > > > >  	uint64_t pos = (uint64_t) *ppos;
> > > > > @@ -1098,7 +1098,9 @@ static int subbuf_splice_actor(struct file *in,
> > > > >  	pidx = (read_start / PAGE_SIZE) % subbuf_pages;
> > > > >  	poff = read_start & ~PAGE_MASK;
> > > > > 
> > > > > -	for (total_len = 0; spd.nr_pages < subbuf_pages; spd.nr_pages++) {
> > > > > +	nr_pages = min_t(unsigned int, subbuf_pages, PIPE_BUFFERS);
> > > > > +
> > > > > +	for (total_len = 0; spd.nr_pages < nr_pages; spd.nr_pages++) {
> > > > >  		unsigned int this_len, this_end, private;
> > > > >  		unsigned int cur_pos = read_start + total_len;
> > > > 
> > > > With the patch, i can run dd and 'blktrace -h traceserver' without the
> > > > oops. But the output from blktrace only contains only zeros and no
> > > > usable data for blkparse. Using blktrace to write the data directly to
> > > > disk, without using the blktrace server works. Is there anything i
> > > > should look for to help debugging the problem?
> > > 
> > > We should probably get Tom in the loop, as he is the relay expert. I'll
> > > make sure the above patch gets into 2.6.25, as it is definitely a bug
> > > that needs fixing.
> > 
> > http://relayfs.sourceforge.net/contact.html mentions Tom Zanussi, but
> > his email address seems to be no longer valid. I copy Dave Wilder
> > here, since he is mentioned as relay maintainer on the web page.
> > 
> 
> Hi,
> 
> Yeah, I no longer work for IBM - I need to update the relayfs site to
> reflect that.
> 
> > Dave, can you have a look at this? I can easily reproduce the problem
> > on s390 Linux for testing and getting more debug information.
> > 
> 
> Not sure anyone's still looking into this, but it doesn't look like an
> s390 problem specifically.  Apparently what happened was that some
> internal changes to splice seem to have broken the relay splice_read
> implementation.  The above patch fixes part of the problem; the patch
> below should fix the rest, and AFAICT it doesn't break anything else.
> 
> Signed-off-by: Tom Zanussi <zanussi@comcast.net>
> 
> diff --git a/fs/splice.c b/fs/splice.c
> index a861bb3..068b210 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1094,7 +1094,7 @@ long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
>  
>  	ret = splice_direct_to_actor(in, &sd, direct_splice_actor);
>  	if (ret > 0)
> -		*ppos += ret;
> +		*ppos = sd.pos;

Tom, did you verify that sd.pos is always updated?

-- 
Jens Axboe