LinuxLists.cc - [PATCH] splice support #2

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

Jens Axboe <[email protected]> wrote:
>
> Hi,
>
> This patch should resolve all issues mentioned so far. I'd still like to
> implement the page moving, but that should just be a separate patch.
> After this round of patching, I thought I'd toss a fresh version out
> there for all to look at.
>
> Signed-off-by: Jens Axboe <[email protected]>
>
> ...
>
> --- a/arch/ia64/kernel/entry.S
> +++ b/arch/ia64/kernel/entry.S
> @@ -1605,5 +1605,6 @@ sys_call_table:
> data8 sys_ni_syscall // reserved for pselect
> data8 sys_ni_syscall // 1295 reserved for ppoll
> data8 sys_unshare
> + date8 sys_splice

typo

> +static void *page_cache_pipe_buf_map(struct file *file,
> + struct pipe_inode_info *info,
> + struct pipe_buffer *buf)
> +{
> + struct page *page = buf->page;
> +
> + lock_page(page);
> +
> + if (!PageUptodate(page)) {

|| page->mapping == NULL

> + struct page *pages[PIPE_BUFFERS];
> + struct page *page;
> + pgoff_t index, pidx;
> + int i;
> +
> + index = in->f_pos >> PAGE_CACHE_SHIFT;
> + offset = in->f_pos & ~PAGE_CACHE_MASK;
> + nr_pages = (len + offset + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
> +
> + if (nr_pages > PIPE_BUFFERS)
> + nr_pages = PIPE_BUFFERS;
> +
> + /*
> + * initiate read-ahead on this page range
> + */
> + do_page_cache_readahead(mapping, in, index, nr_pages);
> +
> + /*
> + * Get as many pages from the page cache as possible..
> + * Start IO on the page cache entries we create (we
> + * can assume that any pre-existing ones we find have
> + * already had IO started on them).
> + */
> + i = find_get_pages(mapping, index, nr_pages, pages);
> +
> + /*
> + * common case - we found all pages and they are contiguous,
> + * kick them off
> + */
> + if (i && (pages[i - 1]->index == index + i - 1))
> + goto splice_them;
> +
> + memset(&pages[i], 0, (nr_pages - i) * sizeof(struct page *));
> +
> + /*
> + * find_get_pages() may not return consecutive pages, so loop
> + * over the array moving pages and filling the rest, if need be.
> + */
> + for (i = 0, pidx = index; i < nr_pages; pidx++, i++) {
> + if (!pages[i]) {
> + int error;
> +fill_page:
> + /*
> + * no page there, look one up / create it
> + */
> + page = find_or_create_page(mapping, pidx,
> + mapping_gfp_mask(mapping));
> + if (!page)
> + break;
> +
> + if (PageUptodate(page))
> + unlock_page(page);
> + else {
> + error = mapping->a_ops->readpage(in, page);
> +
> + if (unlikely(error)) {
> + page_cache_release(page);
> + break;
> + }
> + }
> + pages[i] = page;
> + } else if (pages[i]->index != pidx) {
> + page = pages[i];
> + /*
> + * page isn't in the right spot, move it and jump
> + * back to filling this one. we know that ->index
> + * is larger than pidx
> + */
> + pages[i + page->index - pidx] = page;
> + pages[i] = NULL;
> + goto fill_page;
> + }
> + }
> +
> + if (!i)
> + return 0;
> +
> + /*
> + * Now we splice them into the pipe..
> + */
> +splice_them:
> + return move_to_pipe(pipe, pages, i, offset, len);
> +}

OK, and this returns the number of bytes transferred all the way up to
userspace.

And, like read(), if we hit an IO error or ENOMEM partway through we'll
just return a short read which is indistinguishable from EOF.

But if we do get an IO error, we'll detect it in page_cache_pipe_buf_map().
Does the code still follow these read() return value semantics in that
case?

> ...

argh, am all reviewed out, and infiniband isn't compiling. Will look later.

2006-03-30 10:24:13

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, Mar 30 2006, Andrew Morton wrote:
> Jens Axboe <[email protected]> wrote:
> >
> > Hi,
> >
> > This patch should resolve all issues mentioned so far. I'd still like to
> > implement the page moving, but that should just be a separate patch.
> > After this round of patching, I thought I'd toss a fresh version out
> > there for all to look at.
> >
> > Signed-off-by: Jens Axboe <[email protected]>
> >
> > ...
> >
> > --- a/arch/ia64/kernel/entry.S
> > +++ b/arch/ia64/kernel/entry.S
> > @@ -1605,5 +1605,6 @@ sys_call_table:
> > data8 sys_ni_syscall // reserved for pselect
> > data8 sys_ni_syscall // 1295 reserved for ppoll
> > data8 sys_unshare
> > + date8 sys_splice
>
> typo

Oops, fixed.

> > +static void *page_cache_pipe_buf_map(struct file *file,
> > + struct pipe_inode_info *info,
> > + struct pipe_buffer *buf)
> > +{
> > + struct page *page = buf->page;
> > +
> > + lock_page(page);
> > +
> > + if (!PageUptodate(page)) {
>
> || page->mapping == NULL

Fixed

> > + struct page *pages[PIPE_BUFFERS];
> > + struct page *page;
> > + pgoff_t index, pidx;
> > + int i;
> > +
> > + index = in->f_pos >> PAGE_CACHE_SHIFT;
> > + offset = in->f_pos & ~PAGE_CACHE_MASK;
> > + nr_pages = (len + offset + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
> > +
> > + if (nr_pages > PIPE_BUFFERS)
> > + nr_pages = PIPE_BUFFERS;
> > +
> > + /*
> > + * initiate read-ahead on this page range
> > + */
> > + do_page_cache_readahead(mapping, in, index, nr_pages);
> > +
> > + /*
> > + * Get as many pages from the page cache as possible..
> > + * Start IO on the page cache entries we create (we
> > + * can assume that any pre-existing ones we find have
> > + * already had IO started on them).
> > + */
> > + i = find_get_pages(mapping, index, nr_pages, pages);
> > +
> > + /*
> > + * common case - we found all pages and they are contiguous,
> > + * kick them off
> > + */
> > + if (i && (pages[i - 1]->index == index + i - 1))
> > + goto splice_them;
> > +
> > + memset(&pages[i], 0, (nr_pages - i) * sizeof(struct page *));
> > +
> > + /*
> > + * find_get_pages() may not return consecutive pages, so loop
> > + * over the array moving pages and filling the rest, if need be.
> > + */
> > + for (i = 0, pidx = index; i < nr_pages; pidx++, i++) {
> > + if (!pages[i]) {
> > + int error;
> > +fill_page:
> > + /*
> > + * no page there, look one up / create it
> > + */
> > + page = find_or_create_page(mapping, pidx,
> > + mapping_gfp_mask(mapping));
> > + if (!page)
> > + break;
> > +
> > + if (PageUptodate(page))
> > + unlock_page(page);
> > + else {
> > + error = mapping->a_ops->readpage(in, page);
> > +
> > + if (unlikely(error)) {
> > + page_cache_release(page);
> > + break;
> > + }
> > + }
> > + pages[i] = page;
> > + } else if (pages[i]->index != pidx) {
> > + page = pages[i];
> > + /*
> > + * page isn't in the right spot, move it and jump
> > + * back to filling this one. we know that ->index
> > + * is larger than pidx
> > + */
> > + pages[i + page->index - pidx] = page;
> > + pages[i] = NULL;
> > + goto fill_page;
> > + }
> > + }
> > +
> > + if (!i)
> > + return 0;
> > +
> > + /*
> > + * Now we splice them into the pipe..
> > + */
> > +splice_them:
> > + return move_to_pipe(pipe, pages, i, offset, len);
> > +}
>
> OK, and this returns the number of bytes transferred all the way up to
> userspace.
>
> And, like read(), if we hit an IO error or ENOMEM partway through we'll
> just return a short read which is indistinguishable from EOF.
>
> But if we do get an IO error, we'll detect it in page_cache_pipe_buf_map().
> Does the code still follow these read() return value semantics in that
> case?

Yes, see:

err = actor(info, buf, &sd);
if (err) {
if (!ret)
ret = err;

break;
}

so if we get -EIO from the actor, then we return bytes transferred _or_
-EIO in 0.

> argh, am all reviewed out, and infiniband isn't compiling. Will look later.

I don't blame you, you've been (as always) a huge help! Thanks a lot.

--
Jens Axboe

2006-03-30 11:17:04

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

Jens Axboe <[email protected]> wrote:
>
> > > +static void *page_cache_pipe_buf_map(struct file *file,
> > > + struct pipe_inode_info *info,
> > > + struct pipe_buffer *buf)
> > > +{
> > > + struct page *page = buf->page;
> > > +
> > > + lock_page(page);
> > > +
> > > + if (!PageUptodate(page)) {
> >
> > || page->mapping == NULL
>
> Fixed

Actually that wasn't right. If the page was truncated then we shouldn't
return -EIO to userspace. We should return zero, as this is the first
page.

Which means either returning an ERR_PTR here for -EIO or re-checking i_size
in the caller. Maybe the latter?

2006-03-30 11:55:18

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, Mar 30 2006, Andrew Morton wrote:
> Jens Axboe <[email protected]> wrote:
> >
> > > > +static void *page_cache_pipe_buf_map(struct file *file,
> > > > + struct pipe_inode_info *info,
> > > > + struct pipe_buffer *buf)
> > > > +{
> > > > + struct page *page = buf->page;
> > > > +
> > > > + lock_page(page);
> > > > +
> > > > + if (!PageUptodate(page)) {
> > >
> > > || page->mapping == NULL
> >
> > Fixed
>
> Actually that wasn't right. If the page was truncated then we shouldn't
> return -EIO to userspace. We should return zero, as this is the first
> page.

_if_ this is the first page, then agree.

> Which means either returning an ERR_PTR here for -EIO or re-checking i_size
> in the caller. Maybe the latter?

The latter sounds better, I'll add that.

--
Jens Axboe

2006-03-30 12:03:29

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

* Jens Axboe <[email protected]> wrote:

> Hi,
>
> This patch should resolve all issues mentioned so far. I'd still like
> to implement the page moving, but that should just be a separate
> patch.

neat stuff. One question: why do we require fdin or fdout to be a pipe?
Is there any fundamental problem with implementing what Larry's original
paper described too: straight pagecache -> socket transfers? Without a
pipe intermediary forced inbetween. It only adds unnecessary overhead.

Ingo

2006-03-30 12:05:09

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, Mar 30 2006, Ingo Molnar wrote:
>
> * Jens Axboe <[email protected]> wrote:
>
> > Hi,
> >
> > This patch should resolve all issues mentioned so far. I'd still like
> > to implement the page moving, but that should just be a separate
> > patch.
>
> neat stuff. One question: why do we require fdin or fdout to be a pipe?
> Is there any fundamental problem with implementing what Larry's original
> paper described too: straight pagecache -> socket transfers? Without a
> pipe intermediary forced inbetween. It only adds unnecessary overhead.

No, not a fundamental problem. I think I even hid that in some comment
in there, at least if it's decipharable by someone else than myself...
Basically I think it would be nice in the future to tidy this a little
bit and separate the actual container from the pipe itself - and have
the pipe just fill/use the same container.

--
Jens Axboe

2006-03-30 12:13:01

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

* Jens Axboe <[email protected]> wrote:

> On Thu, Mar 30 2006, Ingo Molnar wrote:
> >
> > * Jens Axboe <[email protected]> wrote:
> >
> > > Hi,
> > >
> > > This patch should resolve all issues mentioned so far. I'd still like
> > > to implement the page moving, but that should just be a separate
> > > patch.
> >
> > neat stuff. One question: why do we require fdin or fdout to be a pipe?
> > Is there any fundamental problem with implementing what Larry's original
> > paper described too: straight pagecache -> socket transfers? Without a
> > pipe intermediary forced inbetween. It only adds unnecessary overhead.
>
> No, not a fundamental problem. I think I even hid that in some comment
> in there, at least if it's decipharable by someone else than myself...
> Basically I think it would be nice in the future to tidy this a little
> bit and separate the actual container from the pipe itself - and have
> the pipe just fill/use the same container.

why is there a container needed at all? If i splice pagecache->socket,
we can use sendpage to send it off immediately. There is no need for any
container - both the pagecache and sendpage use struct page, and when we
iterate to create a container we might as well ->sendpage() those pages
off immediately instead.

I agree with the purpose of making sys_splice() generic and in
particular usable in scripts/shells where pipes are commonly used, but
we should also fulfill the original promise (outlined 15 years ago or
so) and not limit this to pipes. That way i could improve TUX to make
use of it for example ;)

Ingo

2006-03-30 12:16:32

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, Mar 30 2006, Ingo Molnar wrote:
>
> * Jens Axboe <[email protected]> wrote:
>
> > On Thu, Mar 30 2006, Ingo Molnar wrote:
> > >
> > > * Jens Axboe <[email protected]> wrote:
> > >
> > > > Hi,
> > > >
> > > > This patch should resolve all issues mentioned so far. I'd still like
> > > > to implement the page moving, but that should just be a separate
> > > > patch.
> > >
> > > neat stuff. One question: why do we require fdin or fdout to be a pipe?
> > > Is there any fundamental problem with implementing what Larry's original
> > > paper described too: straight pagecache -> socket transfers? Without a
> > > pipe intermediary forced inbetween. It only adds unnecessary overhead.
> >
> > No, not a fundamental problem. I think I even hid that in some comment
> > in there, at least if it's decipharable by someone else than myself...
> > Basically I think it would be nice in the future to tidy this a little
> > bit and separate the actual container from the pipe itself - and have
> > the pipe just fill/use the same container.
>
> why is there a container needed at all? If i splice pagecache->socket,
> we can use sendpage to send it off immediately. There is no need for any
> container - both the pagecache and sendpage use struct page, and when we
> iterate to create a container we might as well ->sendpage() those pages
> off immediately instead.
>
> I agree with the purpose of making sys_splice() generic and in
> particular usable in scripts/shells where pipes are commonly used, but
> we should also fulfill the original promise (outlined 15 years ago or
> so) and not limit this to pipes. That way i could improve TUX to make
> use of it for example ;)

There's absolutely no reason why we can't add fd -> fd splicing as well,
so no worries. Right now we just require a pipe transport. It's
extendable :-)

--
Jens Axboe

2006-03-30 12:30:05

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, Mar 30 2006, Jens Axboe wrote:
> On Thu, Mar 30 2006, Andrew Morton wrote:
> > Jens Axboe <[email protected]> wrote:
> > >
> > > > > +static void *page_cache_pipe_buf_map(struct file *file,
> > > > > + struct pipe_inode_info *info,
> > > > > + struct pipe_buffer *buf)
> > > > > +{
> > > > > + struct page *page = buf->page;
> > > > > +
> > > > > + lock_page(page);
> > > > > +
> > > > > + if (!PageUptodate(page)) {
> > > >
> > > > || page->mapping == NULL
> > >
> > > Fixed
> >
> > Actually that wasn't right. If the page was truncated then we shouldn't
> > return -EIO to userspace. We should return zero, as this is the first
> > page.
>
> _if_ this is the first page, then agree.
>
> > Which means either returning an ERR_PTR here for -EIO or re-checking i_size
> > in the caller. Maybe the latter?
>
> The latter sounds better, I'll add that.

Actually that's a little hard, since you don't know what state buf->page
is in - it might be page cache, it might not be. So I opted for the
ERR_PTR approach, just returning -ENODATA on !page->mapping.

--
Jens Axboe

2006-03-30 12:30:43

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, Mar 30 2006, Jens Axboe wrote:
> On Thu, Mar 30 2006, Jens Axboe wrote:
> > On Thu, Mar 30 2006, Andrew Morton wrote:
> > > Jens Axboe <[email protected]> wrote:
> > > >
> > > > > > +static void *page_cache_pipe_buf_map(struct file *file,
> > > > > > + struct pipe_inode_info *info,
> > > > > > + struct pipe_buffer *buf)
> > > > > > +{
> > > > > > + struct page *page = buf->page;
> > > > > > +
> > > > > > + lock_page(page);
> > > > > > +
> > > > > > + if (!PageUptodate(page)) {
> > > > >
> > > > > || page->mapping == NULL
> > > >
> > > > Fixed
> > >
> > > Actually that wasn't right. If the page was truncated then we shouldn't
> > > return -EIO to userspace. We should return zero, as this is the first
> > > page.
> >
> > _if_ this is the first page, then agree.
> >
> > > Which means either returning an ERR_PTR here for -EIO or re-checking i_size
> > > in the caller. Maybe the latter?
> >
> > The latter sounds better, I'll add that.
>
> Actually that's a little hard, since you don't know what state buf->page
> is in - it might be page cache, it might not be. So I opted for the
> ERR_PTR approach, just returning -ENODATA on !page->mapping.

Ala

diff --git a/fs/splice.c b/fs/splice.c
index 7927dff..68b98bb 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -47,9 +47,14 @@ static void *page_cache_pipe_buf_map(str

lock_page(page);

- if (!PageUptodate(page) || !page->mapping) {
+ if (!PageUptodate(page)) {
+ unlock_page(page);
+ return ERR_PTR(-EIO);
+ }
+
+ if (!page->mapping) {
unlock_page(page);
- return NULL;
+ return ERR_PTR(-ENODATA);
}

return kmap(buf->page);
@@ -279,6 +293,7 @@ static int pipe_to_sendpage(struct pipe_
loff_t pos = sd->pos;
unsigned int offset;
ssize_t ret;
+ void *ptr;

/*
* sub-optimal, but we are limited by the pipe ->map. we don't
@@ -286,8 +301,9 @@ static int pipe_to_sendpage(struct pipe_
* have the page pinned if the pipe page originates from the
* page cache
*/
- if (!buf->ops->map(file, info, buf))
- return -EIO;
+ ptr = buf->ops->map(file, info, buf);
+ if (IS_ERR(ptr))
+ return PTR_ERR(ptr);

offset = pos & ~PAGE_CACHE_MASK;

@@ -334,8 +350,8 @@ static int pipe_to_file(struct pipe_inod
* after this, page will be locked and unmapped
*/
src = buf->ops->map(file, info, buf);
- if (!src)
- return -EIO;
+ if (IS_ERR(src))
+ return PTR_ERR(src);

index = sd->pos >> PAGE_CACHE_SHIFT;
offset = sd->pos & ~PAGE_CACHE_MASK;
@@ -436,7 +452,7 @@ static ssize_t move_from_pipe(struct ino

err = actor(info, buf, &sd);
if (err) {
- if (!ret)
+ if (!ret && err != -ENODATA)
ret = err;

break;

--
Jens Axboe

2006-03-30 12:41:59

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, Mar 30 2006, Ingo Molnar wrote:
>
> * Jens Axboe <[email protected]> wrote:
>
> > > I agree with the purpose of making sys_splice() generic and in
> > > particular usable in scripts/shells where pipes are commonly used, but
> > > we should also fulfill the original promise (outlined 15 years ago or
> > > so) and not limit this to pipes. That way i could improve TUX to make
> > > use of it for example ;)
> >
> > There's absolutely no reason why we can't add fd -> fd splicing as
> > well, so no worries. Right now we just require a pipe transport. It's
> > extendable :-)
>
> ok :) I think this code should be merged into v2.6.17 - it's very clean
> and unintrusive.

I hope so, too. I'll post a hopefully finished patch real soon, and then
the move pages addon afterwards (which works now).

--
Jens Axboe

2006-03-30 12:40:36

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

* Jens Axboe <[email protected]> wrote:

> > I agree with the purpose of making sys_splice() generic and in
> > particular usable in scripts/shells where pipes are commonly used, but
> > we should also fulfill the original promise (outlined 15 years ago or
> > so) and not limit this to pipes. That way i could improve TUX to make
> > use of it for example ;)
>
> There's absolutely no reason why we can't add fd -> fd splicing as
> well, so no worries. Right now we just require a pipe transport. It's
> extendable :-)

ok :) I think this code should be merged into v2.6.17 - it's very clean
and unintrusive.

Ingo

2006-03-30 12:44:41

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

* Jens Axboe <[email protected]> wrote:

> On Thu, Mar 30 2006, Ingo Molnar wrote:
> >
> > * Jens Axboe <[email protected]> wrote:
> >
> > > > I agree with the purpose of making sys_splice() generic and in
> > > > particular usable in scripts/shells where pipes are commonly used, but
> > > > we should also fulfill the original promise (outlined 15 years ago or
> > > > so) and not limit this to pipes. That way i could improve TUX to make
> > > > use of it for example ;)
> > >
> > > There's absolutely no reason why we can't add fd -> fd splicing as
> > > well, so no worries. Right now we just require a pipe transport. It's
> > > extendable :-)
> >
> > ok :) I think this code should be merged into v2.6.17 - it's very clean
> > and unintrusive.
>
> I hope so, too. I'll post a hopefully finished patch real soon, and
> then the move pages addon afterwards (which works now).

what does the move pages addon do? pagecache->pagecache transfer?

Ingo

2006-03-30 13:02:56

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, Mar 30 2006, Ingo Molnar wrote:
>
> * Jens Axboe <[email protected]> wrote:
>
> > On Thu, Mar 30 2006, Ingo Molnar wrote:
> > >
> > > * Jens Axboe <[email protected]> wrote:
> > >
> > > > > I agree with the purpose of making sys_splice() generic and in
> > > > > particular usable in scripts/shells where pipes are commonly used, but
> > > > > we should also fulfill the original promise (outlined 15 years ago or
> > > > > so) and not limit this to pipes. That way i could improve TUX to make
> > > > > use of it for example ;)
> > > >
> > > > There's absolutely no reason why we can't add fd -> fd splicing as
> > > > well, so no worries. Right now we just require a pipe transport. It's
> > > > extendable :-)
> > >
> > > ok :) I think this code should be merged into v2.6.17 - it's very clean
> > > and unintrusive.
> >
> > I hope so, too. I'll post a hopefully finished patch real soon, and
> > then the move pages addon afterwards (which works now).
>
> what does the move pages addon do? pagecache->pagecache transfer?

Precisely, if you do file1 | file2 splicing, you can move the pages to
file2's address space. So say I do splice-cp file1 file2, I would only
have file2 contents in page cache aftwards. And I'd save a memcpy of all
the data.

--
Jens Axboe

2006-03-30 14:20:12

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, Mar 30, 2006 at 02:38:05PM +0200, Ingo Molnar wrote:
> ok :) I think this code should be merged into v2.6.17 - it's very clean
> and unintrusive.

no. there's a reason we have a staging area in -mm. Especially for things
introducing a new user ABI. Give it a few weeks exposure in which a few
things will change for sure instead of rushing it into a release.

2006-03-30 17:03:01

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, 30 Mar 2006, Jens Axboe wrote:
> On Thu, Mar 30 2006, Ingo Molnar wrote:
> >
> > neat stuff. One question: why do we require fdin or fdout to be a pipe?
> > Is there any fundamental problem with implementing what Larry's original
> > paper described too: straight pagecache -> socket transfers? Without a
> > pipe intermediary forced inbetween. It only adds unnecessary overhead.
>
> No, not a fundamental problem. I think I even hid that in some comment
> in there, at least if it's decipharable by someone else than myself...

Actually, there _is_ a fundamental problem. Two of them, in fact.

The reason it goes through a pipe is two-fold:

- the pipe _is_ the buffer. The reason sendfile() sucks is that sendfile
cannot work with <n> different buffer representations. sendfile() only
works with _one_ buffer representation, namely the "page cache of the
file".

By using the page cache directly, sendfile() doesn't need any extra
buffering, but that's also why sendfile() fundamentally _cannot_ work
with anything else. You cannot do "sendfile" between two sockets to
forward data from one place to another, for example. You cannot do
sendfile from a streaming device.

The pipe is just the standard in-kernel buffer between two arbitrary
points. Think of it as a scatter-gather list with a wait-queue. That's
what a pipe _is_. Trying to get rid of the pipe totally misses the
whole point of splice().

Now, we could have a splice call that has an _implicit_ pipe, ie if
neither side is a pipe, we could create a temporary pipe and thus
allow what looks like a direct splice. But the pipe should still be
there.

- The pipe is the buffer #2: it's what allows you to do _other_ things
with splice that are simply impossible to do with sendfile. Notably,
splice allows very naturally the "readv/writev" scatter-gather
behaviour of _mixing_ streams. If you're a web-server, with splice you
can do

write(pipefd, header, header_len);
splice(file, pipefd, file_len);
splice(pipefd, socket, total_len);

(this is all conceptual pseudo-code, of course), and this very
naturally has none of the issues that sendfile() has with plugging etc.
There's never any "send header separately and do extra work to make
sure it is in the same packet as the start of the data".

So having a separate buffer even when you _do_ have a buffer like the
page cache is still something you want to do.

So there.

Linus

2006-03-30 17:17:34

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, 30 Mar 2006, Linus Torvalds wrote:
>
> Actually, there _is_ a fundamental problem. Two of them, in fact.

Actually, four.

The third reason the pipe buffer is so useful is that it's literally a
stream with no position.

That may sound bad, but it's actually a huge deal. It's why standard unix
pipelines are so powerful. You don't pass around "this file, this offset,
this length" - you pass around a simple fd, and you can feed that fd data
without ever having to worry about what the reader of the data does. The
reader cannot seek around to places that you didn't want him to see, and
the reader cannot get confused about where the end is.

The 4th reason is "tee". Again, you _could_ perhaps do "tee" without the
pipe, but it would be a total nightmare. Now, tee isn't that common, but
it does happen, and in particular it happens a lot with certain streaming
content.

Doing a "tee" with regular pipes is not that common: you usually just use
it for debugging or logging (ie you have a pipeline you want to debug, and
inserting "tee" in the middle is a good way to keep the same pipeline, but
also being able to look at the intermediate data when something went
wrong).

However, one reason "tee" _isn't_ that common with regular pipe usage is
that normal programs never need to do that anyway: all the pipe data
always goes through user space, so you can trivially do a "tee" inside of
the application itself without any external support. You just log the data
as you receive it.

But with splice(), the whole _point_ of the system call is that at least a
portion of the data never hits a user space buffer at all. Which means
that suddenly "tee" becomes much more important, because it's the _only_
way to insert a point where you can do logging/debugging of the data.

Now, I didn't do the "tee()" system call in my initial example thing, and
Jens didn't add it either, but I described it back in Jan-2005 with the
original description. It really is very fundamental if you ever want to
have a "plugin" kind of model, where you plug in different users to the
same data stream.

The canonical example is getting video input from an mpeg encoder, and
_both_ saving it to a file and sending it on in real-time to the app that
shows it in a window. Again, having the pipe is what allows this.

Linus

2006-03-30 19:19:39

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

Jens Axboe <[email protected]> wrote:
>
> On Thu, Mar 30 2006, Jens Axboe wrote:
> > On Thu, Mar 30 2006, Andrew Morton wrote:
> > > Jens Axboe <[email protected]> wrote:
> > > >
> > > > > > +static void *page_cache_pipe_buf_map(struct file *file,
> > > > > > + struct pipe_inode_info *info,
> > > > > > + struct pipe_buffer *buf)
> > > > > > +{
> > > > > > + struct page *page = buf->page;
> > > > > > +
> > > > > > + lock_page(page);
> > > > > > +
> > > > > > + if (!PageUptodate(page)) {
> > > > >
> > > > > || page->mapping == NULL
> > > >
> > > > Fixed
> > >
> > > Actually that wasn't right. If the page was truncated then we shouldn't
> > > return -EIO to userspace. We should return zero, as this is the first
> > > page.
> >
> > _if_ this is the first page, then agree.
> >
> > > Which means either returning an ERR_PTR here for -EIO or re-checking i_size
> > > in the caller. Maybe the latter?
> >
> > The latter sounds better, I'll add that.
>
> Actually that's a little hard, since you don't know what state buf->page
> is in - it might be page cache, it might not be. So I opted for the
> ERR_PTR approach, just returning -ENODATA on !page->mapping.
>

That's good, because the recheck-i_size idea was racy too. If another
thread were to do a quick truncate-then-extend then there's a window where
we'd see a truncated page, return NULL from page_cache_pipe_buf_map() and
then by the time i_size is re-checked, i_size has grown again so we end up
deciding it must have been an IO error.

2006-03-30 20:48:20

by Jeff Garzik

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

Let me pull back from the details a bit, to note a code pattern I'm
beginning to see:

if (src_fd is a file &&
dest_fd is a socket)
sendfile()
else
hand code fd->fd data move

with splice this becomes

if (special case fd combination #1)
sendfile()
else (special case fd combination #2)
splice()
else
hand code fd->fd data move

Creating a syscall for each fd->fd data move case seems wasteful. I
would rather that the kernel Does The Right Thing so the app doesn't
have to support all these special cases. Handling the implicit buffer
case in the kernel, when needed, means that the app is future-proofed:
when another fd->fd optimization is implemented, the app automatically
takes advantage of it.

Jeff

2006-03-30 21:17:38

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, 30 Mar 2006, Jeff Garzik wrote:
>
> with splice this becomes
>
> if (special case fd combination #1)
> sendfile()
> else (special case fd combination #2)
> splice()
> else
> hand code fd->fd data move

No, it really should be splice for all combinations (possibly with a
manual read/write fallback for stuff where it just hasn't been done).

The fact that we don't have splice for certain fd combinations is really
just a result of it not being done yet. One of the reasons I wanted to
merge asap was that the original example patch was done over a year ago,
and not a lot happened, so I'm hoping that the fact that the core code is
now in the base tree is going to make people do the necessary splice input
functions for different file types.

For filesystems, splice support tends to be really easy (both read and
write). For other things, it depends a bit. But unlike sendfile(), it
really is quite possible to splice _from_ a socket too, not just _to_ a
socket. But no, that case hasn't been written yet.

(In contrast, with "sendfile()", you just fundamentally couldn't do it).

> Creating a syscall for each fd->fd data move case seems wasteful. I would
> rather that the kernel Does The Right Thing so the app doesn't have to support
> all these special cases. Handling the implicit buffer case in the kernel,
> when needed, means that the app is future-proofed: when another fd->fd
> optimization is implemented, the app automatically takes advantage of it.

splice() really can handle any fd->fd move.

The reason you want to have a pipe in the middle is that you have to
handle partial moves _some_ way. And the pipe being the buffer really does
allow that, and also handles the case of "what happens when we received
more data than we could write".

So the way to do copies is

int pipefd[2];
unsigned long copied = 0;

if (pipe(pipefd) < 0)
error

for (;;) {
int nr = splice(in, pipefd[1], MAX_INT, 0);
if (nr <= 0)
break;
do {
int ret = splice(pipefd[0], out, nr, 0);
if (ret <= 0) {
error: couldn't write 'nr' bytes
break;
}

nr -= ret;
} while (nr);
}
close(pipefd[0]);
close(pipefd[1]);

which may _seem_ very complicated and the extra pipe seems "useless", but
it's (a) actually pretty efficient and (b) allows error recovery.

That (b) is the really important part. I can pretty much guarantee that
without the "useless" pipe, you simply couldn't do it.

In particular, what happens when you try to connect two streaming devices,
but the destination stops accepting data? You cannot put the received data
"back" into the streaming source any way - so if you actually want to be
able to handle error recovery, you _have_ to get access to the source
buffers.

Also, for signal handling, you need to have some way to keep the pipe
around for several iterations on the sender side, while still returning to
user space to do the signal handler.

And guess what? That's exactly what you get with that "useless" pipe. For
error handling, you can decide to throw the extra data that the recipient
didn't want away (just close the pipe), and maybe that's going to be what
most people do. But you could also decide to just do a "read()" on the
pipefd, to just read the data into user space for debugging and/or error
recovery..

Similarly, for signals, the pipe _is_ that buffer that you need that is
consistent across multiple invocations.

So that pipe is not at all unnecessary, and in fact, it's critical. It may
look more complex than sendfile(), but it's more complex exactly because
it can handle cases that sendfile never could, and just punted on (for
regular files, you never have the issue of half-way buffers, since you
just re-read them. Which is why sendfile() could do with it's simple
interface, but is also why sendfile() was never good for anything else).

Linus

2006-03-31 00:03:52

by George Spelvin

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

The Lord High Penguin spake:
> In particular, what happens when you try to connect two streaming devices,
> but the destination stops accepting data? You cannot put the received
> data "back" into the streaming source any way - so if you actually want
> to be able to handle error recovery, you _have_ to get access to the
> source buffers.

Ding! Ding! Ding! The light dawns.

Oh my, that *is* clever. And I remember wrestling with the problem before.
If you're reading from a seekable fd (a la sendfile()), you can always
back up the read pointer, but if you're reading from a non-seekable
fd, what *do* you do if there's a write problem? The pipe gives you
a solution.

However, that gets me thinking... what about a pipe lets it be used as
a splice() source? The answer is that, like seekable files, it's peekable.
You can "peek at" an efficiently-sized chunk of data before deciding
to remove it from the source. You don't have to read(buf)/write(buf),
you can buf=peek()/len=write(buf)/accept(len).

Network sockets have too many features (what with urgent data pointers
and crap) to make that easy, and some character devices have
side effects from read() that can't be hidden at all. So you need
an explicit buffer.

Next, what about a pipe makes it an acceptable splice() destination?
The answer is that space is reservable. You can know that a write()
won't fail before read()ing the source. But a great many devices with
non-synchronous write semantics behave the same way. Certainly it's
not hard to make a non-O_SYNC file system behave that way.

Give all that, it it necessary to use the fallback of a pipe all
the time? It seems that doing without the pipe would be
feasible a large fraction of the time And would a

bytes_written = splice2(src, pipefd[1], pipefd[0], dst, &bytes_buffered);

system call be worth it to reduce overhead? (I'm assuming that
bytes_read = bytes_written + bytes_buffered, but you can use any two
of the three in the interface.) Note that it could bypass the pipe
entirely for those cases where it's possible.

Finally, has anyone thought about the semantics of tee(2)?
It seems that it could be used on block files to get a second
handle that does NOT share the file pointer. But if used on
a pipe, does the write block if EITHER reader is behind?
So one reader could block the other? I can't see any other way
to implement it, but I would like to be sure those semantics are
expected.

2006-03-31 02:31:42

by Nick Piggin

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

Linus Torvalds wrote:

> splice() really can handle any fd->fd move.
>
> The reason you want to have a pipe in the middle is that you have to
> handle partial moves _some_ way. And the pipe being the buffer really does
> allow that, and also handles the case of "what happens when we received
> more data than we could write".
>
> So the way to do copies is
>
> int pipefd[2];
> unsigned long copied = 0;
>
> if (pipe(pipefd) < 0)
> error
>
> for (;;) {
> int nr = splice(in, pipefd[1], MAX_INT, 0);
> if (nr <= 0)
> break;
> do {
> int ret = splice(pipefd[0], out, nr, 0);
> if (ret <= 0) {
> error: couldn't write 'nr' bytes
> break;
> }
>
> nr -= ret;
> } while (nr);
> }
> close(pipefd[0]);
> close(pipefd[1]);
>

I think it makes sense to have a 64-bit length. It just seems
cleaner because it is in userspace units of file offset. Also,
might you be able to do a single file-sized file<->file splice,
and have it do a remote copy on a suitable network fs, or a
whole-file COW on some local fs (maybe not, as splice doesn't
deal with metadata... I don't know the tricky details of fses).

But your argument against a 64-bit length seemed to involve
limiting the usage that sys_splice would see. Make it 64-bit
instead and someone might come up with something that you
hadn't thought of. Is there any downside?

No offsets :(
Don't they only increase flexibility? Or would you prefer to
add a new sys_psplice for that?

--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com

2006-03-31 02:44:08

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

Nick Piggin <[email protected]> wrote:
>
> Also,
> might you be able to do a single file-sized file<->file splice,
> and have it do a remote copy on a suitable network fs

splice() may not be suitable for such filesystems. Quoting Robert Peterson
from earlier this week:

> The problem is that
> loop.c circumvents proper locking by going directly to prepare_write
> rather than following the normal process. If I added some kind of
> library callbacks to allow cluster locking, it would break the
> normal locking sequence of an ordinary write.
>
> The normal sequence of an ordinary write typically looks like:
>
> 1. write
> 2. (Take care of cluster locking if necessary)
> 3. generic_file_write_nolock
> 4. generic_file_aio_write_nolock
> 5. generic_file_buffered_write
> 6. a_ops->prepare_write
> etc.
>
> Right now, loop.c is circumventing step 2 (optional cluster locking
> if needed by the underlying fs) by bypassing the "write" op. If I
> added callbacks to prepare_write at (7) as you suggest, it would either
> (a) introduce deadlocks conflicting with step 2 above or
> (b) require the underlying fs to use its own versions of 3,4,5, and 6,
> thus bypassing a great deal of vfs, which is not good for anyone.
>
> Some could argue that loop.c should always use write rather than
> prepare_write/commit_write, but most fs's don't have special
> locking needs and therefore using them is a performance boost.
>

2006-03-31 02:52:19

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

Andrew Morton <[email protected]> wrote:
>
> splice() may not be suitable for such filesystems.

OK, splice() cuts in at the file_operations level, so sych a clustered
filesystem _could_ implement it, but none of the code we have there will be
usable by it. If the operations in splice.c were to operate at the
file_operations level (->read, ->write) then probably they could be used
thusly.

2006-03-31 06:06:19

by tridge

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

Linus and Jens,

Stephen Rothwell just pointed me at the new splice() interface. Looks
really nice!

One comment though. Could we add a off_t to splice to make it more
like pwrite() ? Otherwise threads could get painful with race
conditions between the seek and the splice() call.

Either that or add psplice() like this:

ssize_t psplice(int fdin, int fdout, size_t len, off_t ofs, unsigned flags);

where ofs sets the offset to read from fdin.

Cheers, Tridge

2006-03-31 06:35:54

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, Mar 30, 2006 at 06:51:33PM -0800, Andrew Morton wrote:
> Andrew Morton <[email protected]> wrote:
> >
> > splice() may not be suitable for such filesystems.
>
> OK, splice() cuts in at the file_operations level, so sych a clustered
> filesystem _could_ implement it, but none of the code we have there will be
> usable by it. If the operations in splice.c were to operate at the
> file_operations level (->read, ->write) then probably they could be used
> thusly.

Of course the code would be useable, same as current generic_file_* code
is useable with small wrappers.

2006-03-31 06:59:23

by Antonio Vargas

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On 3/31/06, [email protected] <[email protected]> wrote:
> Linus and Jens,
>
> Stephen Rothwell just pointed me at the new splice() interface. Looks
> really nice!
>
> One comment though. Could we add a off_t to splice to make it more
> like pwrite() ? Otherwise threads could get painful with race
> conditions between the seek and the splice() call.
>
> Either that or add psplice() like this:
>
> ssize_t psplice(int fdin, int fdout, size_t len, off_t ofs, unsigned flags);
>
> where ofs sets the offset to read from fdin.
>
> Cheers, Tridge

Maybe "ssize_t psplice(int fdin, int fdout, size_t len, off_t
fdin_ofs, unsigned flags);"
would be better at documenting that the offset is for the input? Or
maybe adding also a fdout_ofs for when fdout is also a file...

--
Greetz, Antonio Vargas aka winden of network

http://wind.codepixel.com/
[email protected]
[email protected]

Every day, every year
you have to work
you have to study
you have to scene.

2006-03-31 07:12:08

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

* Linus Torvalds <[email protected]> wrote:

> In particular, what happens when you try to connect two streaming
> devices, but the destination stops accepting data? You cannot put the
> received data "back" into the streaming source any way - so if you
> actually want to be able to handle error recovery, you _have_ to get
> access to the source buffers.

i'd rather implement this error case as an exception mechanism, instead
of a forced intermediary buffer mechanism.

We should extend the userspace API so that it is prepared to receive
'excess data' via a separate 'flush excess data to' file descriptor:

sys_splice(fd_in, fd_out, fd_flush, size,
max_flush_size, *bytes_flushed)

Note1: fd_flush can be a pipe too! This would avoid copies in the
exception case - if the exception case is expected to be common.

Note2: max_flush_size serves as hint and as a natural 'buffering limit'
for the kernel-internal loops. I believe it's more natural than
the implicit 'pipe buffering limit' we currently have.
max_flush_size == 0 would say to the kernel: 'use whatever
buffering is natural or necessary'. E.g. if fd_flush is a pipe,
it would automatically set the buffering size to the flush-pipe's
internal buffering limit.

Note3: we could even eliminate the "*bytes_flushed" parameter from
the syscall: as fd_flush's seek offset gives userspace an idea
about how much data was written to it.

Note4: if the user messes up fd_flush so that the kernel's "excess data"
transfer into fd_flush failes then that's 'tough luck' and flush
data may be lost. Users can use pipes [if the exception case is
common and they want to optimize that codepath] or can pre-write
their files if they need a 100% guarantee.
In fact, the kernel doesnt even have to _look up_ fd_flush in the
common case. It's the application's responsibility to make sure
the exception case will work. This means that the _only_ overhead
from this exception mechanism are the 2-3 extra parameters to
sys_splice(). That's _much_ faster.

Just look at the beauty of this generalization. fd_flush can be
_anything_. It could be a pipe. It could be a temporary file in /tmp. It
could be a file over the network. fd_flush could be mmap()-ed to
user-space! Or it could even be -1 if the user is not interested in the
error case for the streaming data. (For example a good portion of video
and audio playback applications are not interested in the fd_out error
case at all: such data can easily lose 'value' if it gets delayed by
more than a few milliseconds and the right answer is to skip the frame
or display an error message, ignoring the lost data.)

But for heaven's sake: do not slow down the 99.9999999999% fastpath by
forcing a pipe inbetween on the ABI level! I really have nothing against
making sys_splice() generic and i agree that a very good first step to
achieve that is to include pipes in the implementation, but i dont think
pipes are (or should be) all that critical and fundamental to the splice
data-streaming concept itself, as you are suggesting.

> Also, for signal handling, you need to have some way to keep the pipe
> around for several iterations on the sender side, while still
> returning to user space to do the signal handler.

i believe the signal case is naturally handled by the fd_flush approach
too - in fact it can also acts as a nice tester for the exception
handling mechanism.

If the application in question expects to get many signals then it can
use a pipe as fd_flush. (But signal-heavy apps are quite rare: most
performance-critical apps avoid them for the fastpath like the plague,
on modern CPUs it's more expensive to receive and handle a single signal
than to create and tear down a completely new thread (!))

Ingo

2006-03-31 07:36:56

by tridge

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

Antonio,

> Maybe "ssize_t psplice(int fdin, int fdout, size_t len, off_t
> fdin_ofs, unsigned flags);"

yep, thats clearer

I think ideally splice() would just be changed to always take a off_t
as the interface is so new hopefully no apps depend on it yet. Then we
don't need psplice().

A while ago we got bitten with tdb in Samba and systems that only have
write(), and no pwrite(). If a process forks then accesses the db in
both parent and child the shared seek pointer semantics of inherited
file descriptors causes races with seek/write. It would be nice if
splice() was immune from this from the start.

If people want it both ways, then an alternative would be to have a
SPLICE_FLAG_OFS_CURRENT flag to splice(), which would tell splice() to
ignore the off_t and use the current seek ptr. I guess that would save
some book keeping for some apps that know they don't have any seek
races and want to always write at the current position. I wouldn't
need this, but some people might find it convenient.

> would be better at documenting that the offset is for the input? Or
> maybe adding also a fdout_ofs for when fdout is also a file...

I thought about that, but Stephen convinced me it was a bad idea, as
the semantics with whats in the pipe buffer at any one time gets too
messy if the output is a socket. I guess you could return -EINVAL if
you used the fdout_ofs and its not a seekable file, but that seems to
be overloading the system call too much for my liking.

Cheers, Tridge

2006-03-31 07:42:18

by Nick Piggin

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

Andrew Morton wrote:
> Andrew Morton <[email protected]> wrote:
>
>>splice() may not be suitable for such filesystems.
>

Well OK, but my point about making the syscall API use 64-bit
size is not completely invalid. Unless there is some downside
to it.

--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com

2006-03-31 09:56:28

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, Mar 30 2006, Linus Torvalds wrote:
>
>
> On Thu, 30 Mar 2006, Jens Axboe wrote:
> > On Thu, Mar 30 2006, Ingo Molnar wrote:
> > >
> > > neat stuff. One question: why do we require fdin or fdout to be a pipe?
> > > Is there any fundamental problem with implementing what Larry's original
> > > paper described too: straight pagecache -> socket transfers? Without a
> > > pipe intermediary forced inbetween. It only adds unnecessary overhead.
> >
> > No, not a fundamental problem. I think I even hid that in some comment
> > in there, at least if it's decipharable by someone else than myself...
>
> Actually, there _is_ a fundamental problem. Two of them, in fact.
>
> The reason it goes through a pipe is two-fold:
>
> - the pipe _is_ the buffer. The reason sendfile() sucks is that sendfile
> cannot work with <n> different buffer representations. sendfile() only
> works with _one_ buffer representation, namely the "page cache of the
> file".
>
> By using the page cache directly, sendfile() doesn't need any extra
> buffering, but that's also why sendfile() fundamentally _cannot_ work
> with anything else. You cannot do "sendfile" between two sockets to
> forward data from one place to another, for example. You cannot do
> sendfile from a streaming device.
>
> The pipe is just the standard in-kernel buffer between two arbitrary
> points. Think of it as a scatter-gather list with a wait-queue. That's
> what a pipe _is_. Trying to get rid of the pipe totally misses the
> whole point of splice().
>
> Now, we could have a splice call that has an _implicit_ pipe, ie if
> neither side is a pipe, we could create a temporary pipe and thus
> allow what looks like a direct splice. But the pipe should still be
> there.
>
> - The pipe is the buffer #2: it's what allows you to do _other_ things
> with splice that are simply impossible to do with sendfile. Notably,
> splice allows very naturally the "readv/writev" scatter-gather
> behaviour of _mixing_ streams. If you're a web-server, with splice you
> can do
>
> write(pipefd, header, header_len);
> splice(file, pipefd, file_len);
> splice(pipefd, socket, total_len);
>
> (this is all conceptual pseudo-code, of course), and this very
> naturally has none of the issues that sendfile() has with plugging etc.
> There's never any "send header separately and do extra work to make
> sure it is in the same packet as the start of the data".
>
> So having a separate buffer even when you _do_ have a buffer like the
> page cache is still something you want to do.
>
> So there.

My point was mainly that the buffer itself need not necessarily be a
pipe, it could be implemented with a pipe just using the same buffer
type. But I guess it doesn't make much sense, the pipe has nice
advantages in itself.

--
Jens Axboe

2006-03-31 09:57:15

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Fri, Mar 31 2006, [email protected] wrote:
> Linus and Jens,
>
> Stephen Rothwell just pointed me at the new splice() interface. Looks
> really nice!
>
> One comment though. Could we add a off_t to splice to make it more
> like pwrite() ? Otherwise threads could get painful with race
> conditions between the seek and the splice() call.
>
> Either that or add psplice() like this:
>
> ssize_t psplice(int fdin, int fdout, size_t len, off_t ofs, unsigned flags);
>
> where ofs sets the offset to read from fdin.

I definitely see some valid reasons for adding a file offset instead of
using ->f_pos, I'll leave that decision up to Linus though. Linus?

--
Jens Axboe

2006-03-31 12:20:48

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

* Linus Torvalds <[email protected]> wrote:

> - The pipe is the buffer #2: it's what allows you to do _other_ things
> with splice that are simply impossible to do with sendfile. Notably,
> splice allows very naturally the "readv/writev" scatter-gather
> behaviour of _mixing_ streams. If you're a web-server, with splice you
> can do
>
> write(pipefd, header, header_len);
> splice(file, pipefd, file_len);
> splice(pipefd, socket, total_len);
>
> (this is all conceptual pseudo-code, of course), and this very
> naturally has none of the issues that sendfile() has with plugging etc.
> There's never any "send header separately and do extra work to make
> sure it is in the same packet as the start of the data".

with pipe-based buffering this approach has still the very same problems
that sendfile() has with packet boundaries, because it's not enough to
have "large enough" buffering (like a pipe has), the pipe also has to be
drained, and the networking layer has to know the precise boundary of
data.

the right solution to the packet boundary problem is to pass in a proper
"does userspace expect more data right now" flag, or to let userspace
'flush' the socket independently - which is independent of the
pipe-in-slice issue. This solution already exists: the MSG_MORE flag.

Ingo

2006-03-31 12:23:31

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Fri, Mar 31 2006, Ingo Molnar wrote:
>
> * Linus Torvalds <[email protected]> wrote:
>
> > - The pipe is the buffer #2: it's what allows you to do _other_ things
> > with splice that are simply impossible to do with sendfile. Notably,
> > splice allows very naturally the "readv/writev" scatter-gather
> > behaviour of _mixing_ streams. If you're a web-server, with splice you
> > can do
> >
> > write(pipefd, header, header_len);
> > splice(file, pipefd, file_len);
> > splice(pipefd, socket, total_len);
> >
> > (this is all conceptual pseudo-code, of course), and this very
> > naturally has none of the issues that sendfile() has with plugging etc.
> > There's never any "send header separately and do extra work to make
> > sure it is in the same packet as the start of the data".
>
> with pipe-based buffering this approach has still the very same problems
> that sendfile() has with packet boundaries, because it's not enough to
> have "large enough" buffering (like a pipe has), the pipe also has to be
> drained, and the networking layer has to know the precise boundary of
> data.
>
> the right solution to the packet boundary problem is to pass in a proper
> "does userspace expect more data right now" flag, or to let userspace
> 'flush' the socket independently - which is independent of the
> pipe-in-slice issue. This solution already exists: the MSG_MORE flag.

We can add a SPLICE_F_MORE flag for this, right now splice doesn't set
the MSG_MORE flag for the end of the pipe.

--
Jens Axboe

2006-03-31 12:26:19

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Fri, Mar 31 2006, Jens Axboe wrote:
> On Fri, Mar 31 2006, Ingo Molnar wrote:
> >
> > * Linus Torvalds <[email protected]> wrote:
> >
> > > - The pipe is the buffer #2: it's what allows you to do _other_ things
> > > with splice that are simply impossible to do with sendfile. Notably,
> > > splice allows very naturally the "readv/writev" scatter-gather
> > > behaviour of _mixing_ streams. If you're a web-server, with splice you
> > > can do
> > >
> > > write(pipefd, header, header_len);
> > > splice(file, pipefd, file_len);
> > > splice(pipefd, socket, total_len);
> > >
> > > (this is all conceptual pseudo-code, of course), and this very
> > > naturally has none of the issues that sendfile() has with plugging etc.
> > > There's never any "send header separately and do extra work to make
> > > sure it is in the same packet as the start of the data".
> >
> > with pipe-based buffering this approach has still the very same problems
> > that sendfile() has with packet boundaries, because it's not enough to
> > have "large enough" buffering (like a pipe has), the pipe also has to be
> > drained, and the networking layer has to know the precise boundary of
> > data.
> >
> > the right solution to the packet boundary problem is to pass in a proper
> > "does userspace expect more data right now" flag, or to let userspace
> > 'flush' the socket independently - which is independent of the
> > pipe-in-slice issue. This solution already exists: the MSG_MORE flag.
>
> We can add a SPLICE_F_MORE flag for this, right now splice doesn't set
> the MSG_MORE flag for the end of the pipe.

Ala

diff --git a/fs/splice.c b/fs/splice.c
index d8787c1..8a9ef67 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -343,15 +343,16 @@ static int pipe_to_sendpage(struct pipe_
loff_t pos = sd->pos;
unsigned int offset;
ssize_t ret;
+ int more;

ret = buf->ops->map(file, info, buf);
if (unlikely(ret))
return ret;

offset = pos & ~PAGE_CACHE_MASK;
+ more = (sd->flags & SPLICE_F_MORE) || sd->len < sd->total_len;

- ret = file->f_op->sendpage(file, buf->page, offset, sd->len, &pos,
- sd->len < sd->total_len);
+ ret = file->f_op->sendpage(file, buf->page, offset, sd->len, &pos,more);

buf->ops->unmap(info, buf);
if (ret == sd->len)
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index bdf9d57..b003e3d 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -59,5 +59,6 @@ void free_pipe_info(struct inode* inode)
* add the splice flags here.
*/
#define SPLICE_F_MOVE (0x01) /* move pages instead of copying */
+#define SPLICE_F_MORE (0x02) /* expect more data */

#endif

--
Jens Axboe

2006-03-31 12:29:59

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

* Linus Torvalds <[email protected]> wrote:

> The reason it goes through a pipe is two-fold:
>
> - the pipe _is_ the buffer. The reason sendfile() sucks is that sendfile
> cannot work with <n> different buffer representations. sendfile() only
> works with _one_ buffer representation, namely the "page cache of the
> file".

The main problem of sendfile() is that it exposes an incoming-fd
'offset', which pretty much hardcodes the assumption that the incoming
file is a pagecache-backed object. I believe the flush_fd solution to
sys_splice() would solve that range of problems, and thus slice could be
used for offset-less file objects too.

all the remaining problems of sendfile() just derive from this basic
pagecache assumption that is hardcoded in the ABI. Once this one is
overcome (and i believe fd_flush overcomes it), there's no reason why we
couldnt implement the complete range of sys_splice() transfers.

Ingo

2006-03-31 12:47:19

by Bernd Petrovitsch

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Thu, 2006-03-30 at 13:16 -0800, Linus Torvalds wrote:
[...]
> splice() really can handle any fd->fd move.
>
> The reason you want to have a pipe in the middle is that you have to
> handle partial moves _some_ way. And the pipe being the buffer really does
> allow that, and also handles the case of "what happens when we received
> more data than we could write".
>
> So the way to do copies is
>
> int pipefd[2];
> unsigned long copied = 0;
>
> if (pipe(pipefd) < 0)
> error
>
> for (;;) {
> int nr = splice(in, pipefd[1], MAX_INT, 0);
> if (nr <= 0)
> break;
> do {
> int ret = splice(pipefd[0], out, nr, 0);
> if (ret <= 0) {
> error: couldn't write 'nr' bytes
> break;
> }
>
> nr -= ret;
> } while (nr);
> }
> close(pipefd[0]);
> close(pipefd[1]);
>
> which may _seem_ very complicated and the extra pipe seems "useless", but
> it's (a) actually pretty efficient and (b) allows error recovery.
>
> That (b) is the really important part. I can pretty much guarantee that
> without the "useless" pipe, you simply couldn't do it.
>
> In particular, what happens when you try to connect two streaming devices,
> but the destination stops accepting data? You cannot put the received data
> "back" into the streaming source any way - so if you actually want to be
> able to handle error recovery, you _have_ to get access to the source
> buffers.
>
> Also, for signal handling, you need to have some way to keep the pipe
> around for several iterations on the sender side, while still returning to
> user space to do the signal handler.
>
> And guess what? That's exactly what you get with that "useless" pipe. For
> error handling, you can decide to throw the extra data that the recipient
> didn't want away (just close the pipe), and maybe that's going to be what
> most people do. But you could also decide to just do a "read()" on the
> pipefd, to just read the data into user space for debugging and/or error
> recovery..
>
> Similarly, for signals, the pipe _is_ that buffer that you need that is
> consistent across multiple invocations.
>
> So that pipe is not at all unnecessary, and in fact, it's critical. It may

IOW the "pipe" above is in fact just a "handle" of a kernel-internal
buffer (of one page) and the two fd's can be used to tell sys-calls to
read from or write into the buffer. But it is much faster because it
avoids real copying of the data into a user-space buffer and out of the
user-space buffer since the kernel can manipulate the underlying data
directly as a typical implementation of the above code does (i.e. use
read(2) and write(2) instead of the two splice(2) calls).
Uuuuuh, real zero-copy seems possible.

Bernd
--
Firmix Software GmbH http://www.firmix.at/
mobil: +43 664 4416156 fax: +43 1 7890849-55
Embedded Linux Development and Services

2006-03-31 12:50:23

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

* Jens Axboe <[email protected]> wrote:

> > > with pipe-based buffering this approach has still the very same problems
> > > that sendfile() has with packet boundaries, because it's not enough to
> > > have "large enough" buffering (like a pipe has), the pipe also has to be
> > > drained, and the networking layer has to know the precise boundary of
> > > data.
> > >
> > > the right solution to the packet boundary problem is to pass in a proper
> > > "does userspace expect more data right now" flag, or to let userspace
> > > 'flush' the socket independently - which is independent of the
> > > pipe-in-slice issue. This solution already exists: the MSG_MORE flag.
> >
> > We can add a SPLICE_F_MORE flag for this, right now splice doesn't set
> > the MSG_MORE flag for the end of the pipe.
>
> Ala

> #define SPLICE_F_MOVE (0x01) /* move pages instead of copying */
> +#define SPLICE_F_MORE (0x02) /* expect more data */

ok, nice - something like this should work. The direction of the flag is
a philosophical question i guess: i believe in Linux we prefer to
default to "buffering enabled", i.e. the default flag should be "expect
more data". So maybe it would be better to pass in PLICE_F_END, to
indicate end-of-data. [it doesnt mean 'permanent end', so all the files
still remain open: this could be something like a HTTP 1.1 pipelined
request.]

furthermore, the internal implementation should also get smarter and do
a flush-socket if it would e.g. block on a pagecache page. [we often
prefer a partial packet in such cases instead of having a half-built
packet hang around.]

btw., that 'data boundary' detail is likely lost with the pipe
intermediary solution: there is no direct connection between 'input
file' and 'output socket', so a 'flush now' event doesnt get propagated
in a natural way. (unless we extend pipes with 'data boundary' markers,
or force their flushing, which looks a whole lot of complexity for such
a simple thing.)

straight pagecache->socket splicing on the other hand preserves 'data
boundary' markers in a natural way for two reasons: 1) the input and
output objects are known to the kernel at once 2) because there is no
internal buffering to begin with.

Ingo

2006-03-31 18:18:21

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Fri, Mar 31 2006, Ingo Molnar wrote:
>
> * Jens Axboe <[email protected]> wrote:
>
> > > > with pipe-based buffering this approach has still the very same problems
> > > > that sendfile() has with packet boundaries, because it's not enough to
> > > > have "large enough" buffering (like a pipe has), the pipe also has to be
> > > > drained, and the networking layer has to know the precise boundary of
> > > > data.
> > > >
> > > > the right solution to the packet boundary problem is to pass in a proper
> > > > "does userspace expect more data right now" flag, or to let userspace
> > > > 'flush' the socket independently - which is independent of the
> > > > pipe-in-slice issue. This solution already exists: the MSG_MORE flag.
> > >
> > > We can add a SPLICE_F_MORE flag for this, right now splice doesn't set
> > > the MSG_MORE flag for the end of the pipe.
> >
> > Ala
>
> > #define SPLICE_F_MOVE (0x01) /* move pages instead of copying */
> > +#define SPLICE_F_MORE (0x02) /* expect more data */
>
> ok, nice - something like this should work. The direction of the flag is
> a philosophical question i guess: i believe in Linux we prefer to
> default to "buffering enabled", i.e. the default flag should be "expect
> more data". So maybe it would be better to pass in PLICE_F_END, to
> indicate end-of-data. [it doesnt mean 'permanent end', so all the files
> still remain open: this could be something like a HTTP 1.1 pipelined
> request.]

Hmm I do prefer a MORE as to an END, as it will always give predictable
results. The MORE is a performance hit, where as a missing END wont
necessarily be detected until you start looking at latencies.

> furthermore, the internal implementation should also get smarter and do
> a flush-socket if it would e.g. block on a pagecache page. [we often
> prefer a partial packet in such cases instead of having a half-built
> packet hang around.]

That's a really good idea! Wont be too much trouble to add, probably the
easiest is to add a 'wait' variable to the ->map() function or something
along those lines.

> btw., that 'data boundary' detail is likely lost with the pipe
> intermediary solution: there is no direct connection between 'input
> file' and 'output socket', so a 'flush now' event doesnt get propagated
> in a natural way. (unless we extend pipes with 'data boundary' markers,
> or force their flushing, which looks a whole lot of complexity for such
> a simple thing.)

Probably not.

--
Jens Axboe

2006-03-31 19:11:59

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Fri, 31 Mar 2006, Jens Axboe wrote:
> >
> > ssize_t psplice(int fdin, int fdout, size_t len, off_t ofs, unsigned flags);
>
> I definitely see some valid reasons for adding a file offset instead of
> using ->f_pos, I'll leave that decision up to Linus though. Linus?

I think a file offset is fine, the one thing holding me back was just the
interface. One file offset per fd? Or just have the rule that the file
offset is for the "non-pipe" device?

Linus

2006-03-31 19:40:10

[permalink] [raw]

Subject: Re: [PATCH] splice support #2

On Fri, Mar 31 2006, Linus Torvalds wrote:
>
>
> On Fri, 31 Mar 2006, Jens Axboe wrote:
> > >
> > > ssize_t psplice(int fdin, int fdout, size_t len, off_t ofs, unsigned flags);
> >
> > I definitely see some valid reasons for adding a file offset instead of
> > using ->f_pos, I'll leave that decision up to Linus though. Linus?
>
> I think a file offset is fine, the one thing holding me back was just the
> interface. One file offset per fd? Or just have the rule that the file
> offset is for the "non-pipe" device?

Intuitively, I'd expect the offset to be tied to the non-pipe if I were
to eg see this for the first time. So my vote would go for that.

--
Jens Axboe

2006-03-31 20:39:12

by Hua Zhong

[permalink] [raw]

Subject: RE: [PATCH] splice support #2

Hi Linus,

> The 4th reason is "tee". Again, you _could_ perhaps do "tee"
> without the pipe, but it would be a total nightmare. Now, tee
> isn't that common, but it does happen, and in particular it
> happens a lot with certain streaming content.

If I understand correctly:

splice is one fd in, one fd out
tee is one fd in, two fd out (and I'd assume the "one fd in" would always be
a pipe)

How about one fd in, N fd out? Do you then stack the tee calls using
temporary pipes?

i.e., if N=3, then we'd have:

pipe(fd_tmp_pipe);
tee(fd_in, fd_out1, fd_tmp_pipe[0];
tee(fd_tmp_pipe[1], fd_out2, fd_out3);

Basically, N-2 temporary pipes would be required.

Is this the intention?

Hua

2006-03-31 20:49:53