LinuxLists.cc - [PATCH] sendfile removal

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, May 31 2007, Jens Axboe wrote:
> Hi,
>
> This patch removes the ->sendfile() hook from the file_operations
> structure, and replaces the sys_sendfile() mechanism to be based on
> ->splice_read() instead. There should be no functional changes.

Forgot to mention, that the modified kernel of course passes ltp
sendfile tests. FWIW.

--
Jens Axboe

2007-05-31 10:54:30

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, May 31 2007, Eric Dumazet wrote:
> On Thu, 31 May 2007 12:33:16 +0200
> Jens Axboe <[email protected]> wrote:
>
> > Hi,
> >
> > This patch removes the ->sendfile() hook from the file_operations
> > structure, and replaces the sys_sendfile() mechanism to be based on
> > ->splice_read() instead. There should be no functional changes.
> >
>
> > - retval = in_file->f_op->sendfile(in_file, ppos, count, file_send_actor, out_file);
> > + fl = 0;
> > + if (in_file->f_flags & O_NONBLOCK)
> > + fl = SPLICE_F_NONBLOCK;
> > +
> > + retval = do_splice_direct(in_file, ppos, out_file, count, fl);
>
> I like this, but are you sure it wont break user land ?
>
> Some applications might react badly if sendfile() returns EAGAIN ?

Yeah, I didn't actually intend for that to sneak in. I'd think that
userspace should handle it if they opened the file O_NONBLOCK (or used
fcntl()), but it's a change in behaviour none the less and probably not
a good idea.

Encourage those people to use splice() instead :-)

--
Jens Axboe

2007-05-31 10:55:57

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, May 31, 2007 at 12:33:16PM +0200, Jens Axboe wrote:
> - nfds: The ->rq_sendfile_ok optimization is gone for now. I can't
> determine the value of it, but I'm assuming it's there for a reason.
> Any chance this can be converted to splice, or use something else than
> ->sendfile()? CC'ed Neil.

sendfile useage in nfsd avoids a data copy and allows to use checksum
offloading. it's quite important for nfs server workloads.

> Apart from that, it was mostly straight forward. Almost everybody uses
> generic_file_sendfile(), which makes the conversion easy. I changed loop
> to use do_generic_file_read() instead of sendfile, it works for me...

> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 5526ead..92bac14 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -435,16 +435,24 @@ do_lo_receive(struct loop_device *lo,
> {
> struct lo_read_data cookie;
> struct file *file;
> - int retval;
> + read_descriptor_t desc;
> +
> + desc.written = 0;
> + desc.count = bvec->bv_len;
> + desc.arg.data = &cookie;
> + desc.error = 0;
>
> cookie.lo = lo;
> cookie.page = bvec->bv_page;
> cookie.offset = bvec->bv_offset;
> cookie.bsize = bsize;
> file = lo->lo_backing_file;
> - retval = file->f_op->sendfile(file, &pos, bvec->bv_len,
> - lo_read_actor, &cookie);
> - return (retval < 0)? retval: 0;
> +
> + do_generic_file_read(file, &pos, &desc, lo_read_actor);

This change is wrong. loop or any existing user of ->sendfile absolutely
needs to go through a file operations vector so that file-system specific
actions such as locking are performed. This is required at least for the
clustered filesystems and XFS. The right way to implement this is
via do_splice_direct or something similar.

do_generic_file_read is only a library function for filesystem use
and should never be called directly.

2007-05-31 11:05:05

by Carsten Otte

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

Jens Axboe wrote:
> Work to be done:
>
> - The ext2 xip support needs a splice_read() implementation, currently I
> just if 0'ed out the send xip_file_sendfile(). CC'ed Carsten, who
> seems to be the author of this code.
Yup. Will do the splice_read implementation for xip. Do you want to
see it in splice.c or should it go to filemap_xip?

Acked-by: Carsten Otte <[email protected]>

2007-05-31 11:06:58

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, May 31 2007, Christoph Hellwig wrote:
> On Thu, May 31, 2007 at 12:33:16PM +0200, Jens Axboe wrote:
> > - nfds: The ->rq_sendfile_ok optimization is gone for now. I can't
> > determine the value of it, but I'm assuming it's there for a reason.
> > Any chance this can be converted to splice, or use something else than
> > ->sendfile()? CC'ed Neil.
>
> sendfile useage in nfsd avoids a data copy and allows to use checksum
> offloading. it's quite important for nfs server workloads.

OK, I hope Neil can provide some input on how to convert it. Of course
I'm just fishing for Neil to actually do that work :-)

> > Apart from that, it was mostly straight forward. Almost everybody uses
> > generic_file_sendfile(), which makes the conversion easy. I changed loop
> > to use do_generic_file_read() instead of sendfile, it works for me...
>
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 5526ead..92bac14 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -435,16 +435,24 @@ do_lo_receive(struct loop_device *lo,
> > {
> > struct lo_read_data cookie;
> > struct file *file;
> > - int retval;
> > + read_descriptor_t desc;
> > +
> > + desc.written = 0;
> > + desc.count = bvec->bv_len;
> > + desc.arg.data = &cookie;
> > + desc.error = 0;
> >
> > cookie.lo = lo;
> > cookie.page = bvec->bv_page;
> > cookie.offset = bvec->bv_offset;
> > cookie.bsize = bsize;
> > file = lo->lo_backing_file;
> > - retval = file->f_op->sendfile(file, &pos, bvec->bv_len,
> > - lo_read_actor, &cookie);
> > - return (retval < 0)? retval: 0;
> > +
> > + do_generic_file_read(file, &pos, &desc, lo_read_actor);
>
> This change is wrong. loop or any existing user of ->sendfile absolutely
> needs to go through a file operations vector so that file-system specific
> actions such as locking are performed. This is required at least for the
> clustered filesystems and XFS. The right way to implement this is
> via do_splice_direct or something similar.
>
> do_generic_file_read is only a library function for filesystem use
> and should never be called directly.

I'll convert it to do_splice_direct(), thanks.

--
Jens Axboe

2007-05-31 11:07:32

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, May 31 2007, Carsten Otte wrote:
> Jens Axboe wrote:
> >Work to be done:
> >
> >- The ext2 xip support needs a splice_read() implementation, currently I
> > just if 0'ed out the send xip_file_sendfile(). CC'ed Carsten, who
> > seems to be the author of this code.
> Yup. Will do the splice_read implementation for xip. Do you want to
> see it in splice.c or should it go to filemap_xip?

Just put it in filemap_xip.c, I think that would be the best place.
Thanks!

> Acked-by: Carsten Otte <[email protected]>

--
Jens Axboe

2007-05-31 12:26:54

by NeilBrown

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thursday May 31, [email protected] wrote:
> On Thu, May 31 2007, Christoph Hellwig wrote:
> > On Thu, May 31, 2007 at 12:33:16PM +0200, Jens Axboe wrote:
> > > - nfds: The ->rq_sendfile_ok optimization is gone for now. I can't
> > > determine the value of it, but I'm assuming it's there for a reason.
> > > Any chance this can be converted to splice, or use something else than
> > > ->sendfile()? CC'ed Neil.
> >
> > sendfile useage in nfsd avoids a data copy and allows to use checksum
> > offloading. it's quite important for nfs server workloads.
>
> OK, I hope Neil can provide some input on how to convert it. Of course
> I'm just fishing for Neil to actually do that work :-)

Well, seeing you did that zero-length-barrier thing for me, how can I
complain :-)

I'll have a look in the morning.

NeilBrown

2007-05-31 12:28:33

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, May 31 2007, Neil Brown wrote:
> On Thursday May 31, [email protected] wrote:
> > On Thu, May 31 2007, Christoph Hellwig wrote:
> > > On Thu, May 31, 2007 at 12:33:16PM +0200, Jens Axboe wrote:
> > > > - nfds: The ->rq_sendfile_ok optimization is gone for now. I can't
> > > > determine the value of it, but I'm assuming it's there for a reason.
> > > > Any chance this can be converted to splice, or use something else than
> > > > ->sendfile()? CC'ed Neil.
> > >
> > > sendfile useage in nfsd avoids a data copy and allows to use checksum
> > > offloading. it's quite important for nfs server workloads.
> >
> > OK, I hope Neil can provide some input on how to convert it. Of course
> > I'm just fishing for Neil to actually do that work :-)
>
> Well, seeing you did that zero-length-barrier thing for me, how can I
> complain :-)
>
> I'll have a look in the morning.

Woo, thanks Neil :-)

--
Jens Axboe

2007-05-31 15:40:21

by Tom Zanussi

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, 2007-05-31 at 12:33 +0200, Jens Axboe wrote:
> Hi,
>
> This patch removes the ->sendfile() hook from the file_operations
> structure, and replaces the sys_sendfile() mechanism to be based on
> ->splice_read() instead. There should be no functional changes.
>
> Work to be done:
>
> - The ext2 xip support needs a splice_read() implementation, currently I
> just if 0'ed out the send xip_file_sendfile(). CC'ed Carsten, who
> seems to be the author of this code.
>
> - shmem needs a splice_read() implementation. Optimistically CC'ed Hugh.
>
> - nfds: The ->rq_sendfile_ok optimization is gone for now. I can't
> determine the value of it, but I'm assuming it's there for a reason.
> Any chance this can be converted to splice, or use something else than
> ->sendfile()? CC'ed Neil.
>
> - relay: needs a splice_read() implementation. I think Tom already has
> one, CC'ed him.
>

Hi Jens,

Yeah, I have it lying around somewhere - the patch I sent you awhile
back of an initial version of splice_read() is the last work I did on
this. Basically, it worked OK but IIRC there were a couple things in
the code that needed fixing. I also modified blktrace to use splice so
I could use it for testing; I'll send a patch for that too once I get it
working again.

Anyway, that was ages ago (sometime last year ;-) so I'm going to have
to dig up the code and probably rework it a bit.

Tom

2007-05-31 17:06:46

by Hugh Dickins

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, 31 May 2007, Jens Axboe wrote:
>
> This patch removes the ->sendfile() hook from the file_operations
> structure, and replaces the sys_sendfile() mechanism to be based on
> ->splice_read() instead. There should be no functional changes.
>
> Work to be done:
>
> - The ext2 xip support needs a splice_read() implementation, currently I
> just if 0'ed out the send xip_file_sendfile(). CC'ed Carsten, who
> seems to be the author of this code.
>
> - shmem needs a splice_read() implementation. Optimistically CC'ed Hugh.

I'll take that "Optimistically" as a special compliment,
rather than as a particular insult ;)

Yes, thanks, please leave shmem_file_splice_read() to me, I'll give
it priority now. Not deep enough in yet, but I'll probably aim for
something simple-minded (correct but slow once it hits swap).

>
> - nfds: The ->rq_sendfile_ok optimization is gone for now. I can't
> determine the value of it, but I'm assuming it's there for a reason.
> Any chance this can be converted to splice, or use something else than
> ->sendfile()? CC'ed Neil.
>
> - relay: needs a splice_read() implementation. I think Tom already has
> one, CC'ed him.
>
> Apart from that, it was mostly straight forward. Almost everybody uses
> generic_file_sendfile(), which makes the conversion easy. I changed loop
> to use do_generic_file_read() instead of sendfile, it works for me...

Christoph already picked up on that, and it's of interest to shmem too:
loop over tmpfs in 2.6 was relying on shmem_file_sendfile, for which
the generic route is not good enough.

If we're giving a .splice_read to everything which used to have a
.sendfile, then I think you just need to make do_lo_read() use
->splice_read now?

Hugh

2007-05-31 17:32:00

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, May 31, 2007 at 06:06:19PM +0100, Hugh Dickins wrote:
> Christoph already picked up on that, and it's of interest to shmem too:
> loop over tmpfs in 2.6 was relying on shmem_file_sendfile, for which
> the generic route is not good enough.
>
> If we're giving a .splice_read to everything which used to have a
> .sendfile, then I think you just need to make do_lo_read() use
> ->splice_read now?

I'd be perfectly happy with killing support for using ->read. Any
simple filesystem can use the generic methods, and any more complex
filesystem should better have splice aswell. Btw, someone should
look into using splice for the loop write path aswell, the current
useage of ->prepare_write and ->commit_write has similar locking
problems aswell.

2007-05-31 19:03:09

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, May 31 2007, Tom Zanussi wrote:
> On Thu, 2007-05-31 at 12:33 +0200, Jens Axboe wrote:
> > Hi,
> >
> > This patch removes the ->sendfile() hook from the file_operations
> > structure, and replaces the sys_sendfile() mechanism to be based on
> > ->splice_read() instead. There should be no functional changes.
> >
> > Work to be done:
> >
> > - The ext2 xip support needs a splice_read() implementation, currently I
> > just if 0'ed out the send xip_file_sendfile(). CC'ed Carsten, who
> > seems to be the author of this code.
> >
> > - shmem needs a splice_read() implementation. Optimistically CC'ed Hugh.
> >
> > - nfds: The ->rq_sendfile_ok optimization is gone for now. I can't
> > determine the value of it, but I'm assuming it's there for a reason.
> > Any chance this can be converted to splice, or use something else than
> > ->sendfile()? CC'ed Neil.
> >
> > - relay: needs a splice_read() implementation. I think Tom already has
> > one, CC'ed him.
> >
>
> Hi Jens,
>
> Yeah, I have it lying around somewhere - the patch I sent you awhile
> back of an initial version of splice_read() is the last work I did on
> this. Basically, it worked OK but IIRC there were a couple things in
> the code that needed fixing. I also modified blktrace to use splice so
> I could use it for testing; I'll send a patch for that too once I get it
> working again.
>
> Anyway, that was ages ago (sometime last year ;-) so I'm going to have
> to dig up the code and probably rework it a bit.

It'd be great if you can polish it a bit (basically just make it apply
to the current kernel). I don't think it's hard, aside from moving the
code from fs/relayfs/ to kernel/relay.c, there hasn't been many changes.

--
Jens Axboe

2007-05-31 19:04:54

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, May 31 2007, Hugh Dickins wrote:
> On Thu, 31 May 2007, Jens Axboe wrote:
> >
> > This patch removes the ->sendfile() hook from the file_operations
> > structure, and replaces the sys_sendfile() mechanism to be based on
> > ->splice_read() instead. There should be no functional changes.
> >
> > Work to be done:
> >
> > - The ext2 xip support needs a splice_read() implementation, currently I
> > just if 0'ed out the send xip_file_sendfile(). CC'ed Carsten, who
> > seems to be the author of this code.
> >
> > - shmem needs a splice_read() implementation. Optimistically CC'ed Hugh.
>
> I'll take that "Optimistically" as a special compliment,
> rather than as a particular insult ;)

It's a compliment for sure, the "optimistically" was just mentioned
because there are lots of people mentioned in that file, but I mostly
remember you doing some heavy lifting there :-)

> Yes, thanks, please leave shmem_file_splice_read() to me, I'll give
> it priority now. Not deep enough in yet, but I'll probably aim for
> something simple-minded (correct but slow once it hits swap).

Super, thanks!!

> > - nfds: The ->rq_sendfile_ok optimization is gone for now. I can't
> > determine the value of it, but I'm assuming it's there for a reason.
> > Any chance this can be converted to splice, or use something else than
> > ->sendfile()? CC'ed Neil.
> >
> > - relay: needs a splice_read() implementation. I think Tom already has
> > one, CC'ed him.
> >
> > Apart from that, it was mostly straight forward. Almost everybody uses
> > generic_file_sendfile(), which makes the conversion easy. I changed loop
> > to use do_generic_file_read() instead of sendfile, it works for me...
>
> Christoph already picked up on that, and it's of interest to shmem too:
> loop over tmpfs in 2.6 was relying on shmem_file_sendfile, for which
> the generic route is not good enough.
>
> If we're giving a .splice_read to everything which used to have a
> .sendfile, then I think you just need to make do_lo_read() use
> ->splice_read now?

Yes, I will make that change myself. I can do that, as I seem to have
sucessfully pushed out the remaining work to others :-)

--
Jens Axboe

2007-06-01 04:14:25

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

Jens Axboe wrote:
>>
>>> - retval = in_file->f_op->sendfile(in_file, ppos, count, file_send_actor, out_file);
>>> + fl = 0;
>>> + if (in_file->f_flags & O_NONBLOCK)
>>> + fl = SPLICE_F_NONBLOCK;
>>> +
>>> + retval = do_splice_direct(in_file, ppos, out_file, count, fl);
>> I like this, but are you sure it wont break user land ?
>>
>> Some applications might react badly if sendfile() returns EAGAIN ?
>
> Yeah, I didn't actually intend for that to sneak in. I'd think that
> userspace should handle it if they opened the file O_NONBLOCK (or used
> fcntl()), but it's a change in behaviour none the less and probably not
> a good idea.
>

I would personally argue that sendfile() blocking on an O_NONBLOCK
desriptor, as opposed to returning EAGAIN, is a bug, and a fairly
serious such.

-hpa

2007-06-01 05:42:38

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, May 31 2007, H. Peter Anvin wrote:
> Jens Axboe wrote:
> >>
> >>> - retval = in_file->f_op->sendfile(in_file, ppos, count, file_send_actor, out_file);
> >>> + fl = 0;
> >>> + if (in_file->f_flags & O_NONBLOCK)
> >>> + fl = SPLICE_F_NONBLOCK;
> >>> +
> >>> + retval = do_splice_direct(in_file, ppos, out_file, count, fl);
> >> I like this, but are you sure it wont break user land ?
> >>
> >> Some applications might react badly if sendfile() returns EAGAIN ?
> >
> > Yeah, I didn't actually intend for that to sneak in. I'd think that
> > userspace should handle it if they opened the file O_NONBLOCK (or used
> > fcntl()), but it's a change in behaviour none the less and probably not
> > a good idea.
> >
>
> I would personally argue that sendfile() blocking on an O_NONBLOCK
> desriptor, as opposed to returning EAGAIN, is a bug, and a fairly
> serious such.

I agree, but it's still a change in behaviour. Even if we consider the
app buggy (it is), can we potentially break it?

--
Jens Axboe

2007-06-01 05:54:36

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

Jens Axboe wrote:
>>>
>> I would personally argue that sendfile() blocking on an O_NONBLOCK
>> desriptor, as opposed to returning EAGAIN, is a bug, and a fairly
>> serious such.
>
> I agree, but it's still a change in behaviour. Even if we consider the
> app buggy (it is), can we potentially break it?
>

It depends on which app it is, of course. However, I think we have to
smoke that out the hard way. I don't think we should retain a bug in
the kernel just because some unknown app might depend on that bug --
taking that to the extreme we could never fix bugs at all...

-hpa

2007-06-01 07:24:32

by Eric Dumazet

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

H. Peter Anvin a ?crit :
> Jens Axboe wrote:
>>> I would personally argue that sendfile() blocking on an O_NONBLOCK
>>> desriptor, as opposed to returning EAGAIN, is a bug, and a fairly
>>> serious such.
>> I agree, but it's still a change in behaviour. Even if we consider the
>> app buggy (it is), can we potentially break it?
>>
>
> It depends on which app it is, of course. However, I think we have to
> smoke that out the hard way. I don't think we should retain a bug in
> the kernel just because some unknown app might depend on that bug --
> taking that to the extreme we could never fix bugs at all...
>

As I said, this new non blocking feature on the input side (disk), is nice and
usefull. (For people scared by splice() syscall :) )

Just have to mention it is a change of behavior, and documentation probably
needs to reflect this change. "Since linux 2.6.23, sendfile() repects
O_NONBLOCK on in_fd as well"

My man page here says :

ERRORS
EAGAIN Non-blocking I/O has been selected using O_NONBLOCK and the
write would block.

EBADF The input file was not opened for reading or the output file
was not opened for writing.

EFAULT Bad address.

EINVAL Descriptor is not valid or locked, or an mmap()-like operation
is not available for in_fd.

EIO Unspecified error while reading from in_fd.

ENOMEM Insufficient memory to read from in_fd.

This implies O_NONBLOCK on the out filedesc, not the input one :)

2007-06-01 08:17:04

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Thu, May 31 2007, Jens Axboe wrote:
> > This change is wrong. loop or any existing user of ->sendfile absolutely
> > needs to go through a file operations vector so that file-system specific
> > actions such as locking are performed. This is required at least for the
> > clustered filesystems and XFS. The right way to implement this is
> > via do_splice_direct or something similar.
> >
> > do_generic_file_read is only a library function for filesystem use
> > and should never be called directly.
>
> I'll convert it to do_splice_direct(), thanks.

So how does this look? Totally untested, will do that now. It only
covers the read side, the write conversion to splice will happen later.

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 92bac14..3714e60 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -74,6 +74,7 @@
#include <linux/highmem.h>
#include <linux/gfp.h>
#include <linux/kthread.h>
+#include <linux/pipe_fs_i.h>

#include <asm/uaccess.h>

@@ -401,58 +402,70 @@ struct lo_read_data {
};

static int
-lo_read_actor(read_descriptor_t *desc, struct page *page,
- unsigned long offset, unsigned long size)
+lo_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
+ struct splice_desc *sd)
{
- unsigned long count = desc->count;
- struct lo_read_data *p = desc->arg.data;
+ struct lo_read_data *p = sd->data;
struct loop_device *lo = p->lo;
+ struct page *page = buf->page;
sector_t IV;
+ size_t size;
+ int ret;

- IV = ((sector_t) page->index << (PAGE_CACHE_SHIFT - 9))+(offset >> 9);
+ ret = buf->ops->pin(pipe, buf);
+ if (unlikely(ret))
+ return ret;

- if (size > count)
- size = count;
+ IV = ((sector_t) page->index << (PAGE_CACHE_SHIFT - 9))+(buf->offset >> 9);
+ size = sd->len;

- if (lo_do_transfer(lo, READ, page, offset, p->page, p->offset, size, IV)) {
+ if (lo_do_transfer(lo, READ, page, buf->offset, p->page, p->offset, size, IV)) {
size = 0;
printk(KERN_ERR "loop: transfer error block %ld\n",
page->index);
- desc->error = -EINVAL;
+ size = -EINVAL;
}

flush_dcache_page(p->page);

- desc->count = count - size;
- desc->written += size;
- p->offset += size;
+ if (size > 0)
+ p->offset += size;
return size;
}

static int
+lo_direct_splice_actor(struct pipe_inode_info *pipe, struct splice_desc *sd)
+{
+ return __splice_from_pipe(pipe, sd, lo_splice_actor);
+}
+
+static int
do_lo_receive(struct loop_device *lo,
struct bio_vec *bvec, int bsize, loff_t pos)
{
struct lo_read_data cookie;
+ struct splice_desc sd;
struct file *file;
- read_descriptor_t desc;
-
- desc.written = 0;
- desc.count = bvec->bv_len;
- desc.arg.data = &cookie;
- desc.error = 0;
+ int retval;

cookie.lo = lo;
cookie.page = bvec->bv_page;
cookie.offset = bvec->bv_offset;
cookie.bsize = bsize;
+
+ sd.len = 0;
+ sd.total_len = bsize;
+ sd.flags = 0;
+ sd.pos = pos;
+ sd.data = &cookie;
+
file = lo->lo_backing_file;
+ retval = splice_direct_to_actor(file, &sd, lo_direct_splice_actor);

- do_generic_file_read(file, &pos, &desc, lo_read_actor);
- if (desc.written)
- return 0;
+ if (retval < 0)
+ return retval;

- return desc.error;
+ return 0;
}

static int
@@ -687,8 +700,8 @@ static int loop_change_fd(struct loop_device *lo, struct file *lo_file,
if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
goto out_putf;

- /* new backing store needs to support loop (eg readpage) */
- if (!file->f_mapping->a_ops->readpage)
+ /* new backing store needs to support loop (eg splice_read) */
+ if (!inode->i_fop->splice_read)
goto out_putf;

/* size of the new backing store needs to be the same */
@@ -768,7 +781,7 @@ static int loop_set_fd(struct loop_device *lo, struct file *lo_file,
* If we can't read - sorry. If we only can't write - well,
* it's going to be read-only.
*/
- if (!aops->readpage)
+ if (!file->f_op->splice_read)
goto out_putf;
if (aops->prepare_write && aops->commit_write)
lo_flags |= LO_FLAGS_USE_AOPS;

--
Jens Axboe

2007-06-01 15:57:48

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

Eric Dumazet wrote:
>
> As I said, this new non blocking feature on the input side (disk), is
> nice and usefull. (For people scared by splice() syscall :) )
>
> Just have to mention it is a change of behavior, and documentation
> probably needs to reflect this change. "Since linux 2.6.23, sendfile()
> repects O_NONBLOCK on in_fd as well"
>

Fair enough. Unix has traditionally not acknowledged the possibility of
nonblocking I/O on conventional files, for some odd reason.

-hpa

2007-06-01 16:20:26

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Fri, 1 Jun 2007, H. Peter Anvin wrote:
>
> Fair enough. Unix has traditionally not acknowledged the possibility of
> nonblocking I/O on conventional files, for some odd reason.

It's not odd at all.

If you return EAGAIN, you had better have a way to _wait_ for that EAGAIN
to go away, otherwise the EAGAIN is just a total waste of time.

So the rule about EAGAIN is very simple:
(a) the file descriptor must be O_NONBLOCK
(b) the access must otherwise block
AND
(c) the condition must be something we can wait for with poll/select

I don't know why people continually ignore that (c) point, even though
it's obvious and very very important!

If you cannot wait for it, tell me why the kernel should _ever_ return
EAGAIN? The only option for the user is to just do the operation again
immediately.

And the thing is, neither poll nor select work on regular files. And no,
that is _not_ just an implementation issue. It's very fundamental: neither
poll nor select get the file offset to wait for!

And that file offset is _critical_ for a regular file, in a way it
obviously is _not_ for a socket, pipe, or other special file. Because
without knowing the file offset, you cannot know which page you should be
waiting for!

And no, the file offset is not "f_pos". sendfile(), along with
pread/pwrite, uses a totally separate file offset, so if select/poll were
to base their decision on f_pos, they'd be _wrong_.

This really is very fundamental.

Now, you can argue that you can always just return -EAGAIN anyway, but
then the calling process will basically be busy-looping, calling
sendfile() (or splice()) over and over again. That's _horrible_. It's much
better to just not return EAGAIN, and sleep like a good process should!

So there's a few things to take away from this:

- regular file access MUST NOT return EAGAIN just because a page isn't
in the cache. Doing so is simply a bug. No ifs, buts or maybe's about
it!

Busy-looping is NOT ACCEPTABLE!

- you *could* make some alternative conventions:

(a) you could make O_NONBLOCK mean that you'll at least
guarantee that you *start* the IO, and while you never return
EAGAIN, you migth validly return a _partial_ result!

(b) variation on (a): it's ok to return EAGAIN if _you_ were the
one who started the IO during this particular time aroudn the
loop. But if you find a page that isn't up-to-date yet, and
you didn't start the IO, you *must* wait for it, so that you
end up returning EAGAIN atmost once! Exactly because
busy-looping is simply not acceptable behaviour!

I have to admit that I didn't look at what raw splice() itself does these
days. I would not be surprised if Jens also didn't realize this very
fundamental issue. It seems too easy to miss, because people think
that EAGAIN stands on its own, and don't realize that EAGAIN must be
paired with select/poll to make sense.

Jens?

Linus

2007-06-01 16:40:03

by Pádraig Brady

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

H. Peter Anvin wrote:
> Eric Dumazet wrote:
>> As I said, this new non blocking feature on the input side (disk), is
>> nice and usefull. (For people scared by splice() syscall :) )
>>
>> Just have to mention it is a change of behavior, and documentation
>> probably needs to reflect this change. "Since linux 2.6.23, sendfile()
>> repects O_NONBLOCK on in_fd as well"
>>
>
> Fair enough. Unix has traditionally not acknowledged the possibility of
> nonblocking I/O on conventional files, for some odd reason.

That reminds me of this patch:
http://lkml.org/lkml/2004/10/1/217
which went in for a while but was reverted:
http://lkml.org/lkml/2004/10/17/17

P?draig

2007-06-01 16:49:43

by Eric Dumazet

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

Linus Torvalds a ?crit :
>
> On Fri, 1 Jun 2007, H. Peter Anvin wrote:
>> Fair enough. Unix has traditionally not acknowledged the possibility of
>> nonblocking I/O on conventional files, for some odd reason.
>
> It's not odd at all.
>
> If you return EAGAIN, you had better have a way to _wait_ for that EAGAIN
> to go away, otherwise the EAGAIN is just a total waste of time.
>
> So the rule about EAGAIN is very simple:
> (a) the file descriptor must be O_NONBLOCK
> (b) the access must otherwise block
> AND
> (c) the condition must be something we can wait for with poll/select
>
> I don't know why people continually ignore that (c) point, even though
> it's obvious and very very important!
>
> If you cannot wait for it, tell me why the kernel should _ever_ return
> EAGAIN? The only option for the user is to just do the operation again
> immediately.
>
> And the thing is, neither poll nor select work on regular files. And no,
> that is _not_ just an implementation issue. It's very fundamental: neither
> poll nor select get the file offset to wait for!
>
> And that file offset is _critical_ for a regular file, in a way it
> obviously is _not_ for a socket, pipe, or other special file. Because
> without knowing the file offset, you cannot know which page you should be
> waiting for!
>
> And no, the file offset is not "f_pos". sendfile(), along with
> pread/pwrite, uses a totally separate file offset, so if select/poll were
> to base their decision on f_pos, they'd be _wrong_.
>
> This really is very fundamental.
>
> Now, you can argue that you can always just return -EAGAIN anyway, but
> then the calling process will basically be busy-looping, calling
> sendfile() (or splice()) over and over again. That's _horrible_. It's much
> better to just not return EAGAIN, and sleep like a good process should!
>
> So there's a few things to take away from this:
>
> - regular file access MUST NOT return EAGAIN just because a page isn't
> in the cache. Doing so is simply a bug. No ifs, buts or maybe's about
> it!
>
> Busy-looping is NOT ACCEPTABLE!

yes, very true, but then some apps do this (and sometimes depends on yield())

>
> - you *could* make some alternative conventions:
>
> (a) you could make O_NONBLOCK mean that you'll at least
> guarantee that you *start* the IO, and while you never return
> EAGAIN, you migth validly return a _partial_ result!
>
> (b) variation on (a): it's ok to return EAGAIN if _you_ were the
> one who started the IO during this particular time aroudn the
> loop. But if you find a page that isn't up-to-date yet, and
> you didn't start the IO, you *must* wait for it, so that you
> end up returning EAGAIN atmost once! Exactly because
> busy-looping is simply not acceptable behaviour!
>
> I have to admit that I didn't look at what raw splice() itself does these
> days. I would not be surprised if Jens also didn't realize this very
> fundamental issue. It seems too easy to miss, because people think
> that EAGAIN stands on its own, and don't realize that EAGAIN must be
> paired with select/poll to make sense.
>

Right now, splice() has one SPLICE_F_NONBLOCK flag, and this flag is applied
on both sides (in & out)

So either :

1) We separate the flag into two flags NONBLOCK_IN & NONBLOCK_OUT, so that the
application is free to chose to busy-loop/yield if it wants.

2) We ignore NONBLOCK flag for regular files in splice() (and sendfile()),
just following current facto

3) We consider select()/poll()/splice() can be extended to regular files on
[f_pos] (select() and related functions have a meaning on non-seekable files,
so consider it can be extended on files only on current file pos)

2007-06-01 16:59:09

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

Linus Torvalds wrote:
> And the thing is, neither poll nor select work on regular files. And no,
> that is _not_ just an implementation issue. It's very fundamental: neither
> poll nor select get the file offset to wait for!
>
> And that file offset is _critical_ for a regular file, in a way it
> obviously is _not_ for a socket, pipe, or other special file. Because
> without knowing the file offset, you cannot know which page you should be
> waiting for!
>
> And no, the file offset is not "f_pos". sendfile(), along with
> pread/pwrite, uses a totally separate file offset, so if select/poll were
> to base their decision on f_pos, they'd be _wrong_.

This is obviously correct, although at the time those interfaces were
designed, I don't believe either pread/pwrite nor sendfile() existed,
and they still couldn't wait on real files. That there isn't a suitable
way to wait for a file at an offset is probably a result of that past
history.

Waiting at f_pos is still a possible interface, of course; it would mean
that pread/pwrite/sendfile users would have to seek before waiting.
However, implementing waiting on files in select/poll is prohibited by
POSIX, so it would at least need some sort of Linux-specific flag anyway.

It seems that being able to do nonblocking I/O on files would be a
useful thing. This really *does* require proper nonblocking I/O and not
just the ability to wait, since you can never know when the kernel
decides to recycle the page you are just about to want from the cache.

> So there's a few things to take away from this:
>
> - regular file access MUST NOT return EAGAIN just because a page isn't
> in the cache. Doing so is simply a bug. No ifs, buts or maybe's about
> it!
>
> Busy-looping is NOT ACCEPTABLE!
>
> - you *could* make some alternative conventions:
>
> (a) you could make O_NONBLOCK mean that you'll at least
> guarantee that you *start* the IO, and while you never return
> EAGAIN, you migth validly return a _partial_ result!
>
> (b) variation on (a): it's ok to return EAGAIN if _you_ were the
> one who started the IO during this particular time aroudn the
> loop. But if you find a page that isn't up-to-date yet, and
> you didn't start the IO, you *must* wait for it, so that you
> end up returning EAGAIN atmost once! Exactly because
> busy-looping is simply not acceptable behaviour!

(b) seems really ugly. (a) is at least well-defined. Either seems
wrong, though.

-hpa

2007-06-02 15:02:36

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Fri, Jun 01 2007, Linus Torvalds wrote:
>
>
> On Fri, 1 Jun 2007, H. Peter Anvin wrote:
> >
> > Fair enough. Unix has traditionally not acknowledged the possibility of
> > nonblocking I/O on conventional files, for some odd reason.
>
> It's not odd at all.
>
> If you return EAGAIN, you had better have a way to _wait_ for that EAGAIN
> to go away, otherwise the EAGAIN is just a total waste of time.
>
> So the rule about EAGAIN is very simple:
> (a) the file descriptor must be O_NONBLOCK
> (b) the access must otherwise block
> AND
> (c) the condition must be something we can wait for with poll/select
>
> I don't know why people continually ignore that (c) point, even though
> it's obvious and very very important!
>
> If you cannot wait for it, tell me why the kernel should _ever_ return
> EAGAIN? The only option for the user is to just do the operation again
> immediately.
>
> And the thing is, neither poll nor select work on regular files. And no,
> that is _not_ just an implementation issue. It's very fundamental: neither
> poll nor select get the file offset to wait for!
>
> And that file offset is _critical_ for a regular file, in a way it
> obviously is _not_ for a socket, pipe, or other special file. Because
> without knowing the file offset, you cannot know which page you should be
> waiting for!
>
> And no, the file offset is not "f_pos". sendfile(), along with
> pread/pwrite, uses a totally separate file offset, so if select/poll were
> to base their decision on f_pos, they'd be _wrong_.
>
> This really is very fundamental.
>
> Now, you can argue that you can always just return -EAGAIN anyway, but
> then the calling process will basically be busy-looping, calling
> sendfile() (or splice()) over and over again. That's _horrible_. It's much
> better to just not return EAGAIN, and sleep like a good process should!
>
> So there's a few things to take away from this:
>
> - regular file access MUST NOT return EAGAIN just because a page isn't
> in the cache. Doing so is simply a bug. No ifs, buts or maybe's about
> it!
>
> Busy-looping is NOT ACCEPTABLE!
>
> - you *could* make some alternative conventions:
>
> (a) you could make O_NONBLOCK mean that you'll at least
> guarantee that you *start* the IO, and while you never return
> EAGAIN, you migth validly return a _partial_ result!
>
> (b) variation on (a): it's ok to return EAGAIN if _you_ were the
> one who started the IO during this particular time aroudn the
> loop. But if you find a page that isn't up-to-date yet, and
> you didn't start the IO, you *must* wait for it, so that you
> end up returning EAGAIN atmost once! Exactly because
> busy-looping is simply not acceptable behaviour!
>
> I have to admit that I didn't look at what raw splice() itself does these
> days. I would not be surprised if Jens also didn't realize this very
> fundamental issue. It seems too easy to miss, because people think
> that EAGAIN stands on its own, and don't realize that EAGAIN must be
> paired with select/poll to make sense.

splice() WILL return EAGAIN, but in that case it should have triggered
the read-ahead and thus started some IO. At least that was/is the
intention, and it worked like that originally. Since I'm deep on the
splice updating for sendfile etc right now anyway, I'll doubly verify
that it still works that way as expected!

For the from-file case, see __generic_file_splice_read(). splice does:

if (!PageUptodate(page)) {
/*
* If in nonblock mode then dont block on
* waiting
* for an in-flight io page
*/
if (flags & SPLICE_F_NONBLOCK) {
if (TestSetPageLocked(page))
break;
} else
lock_page(page);

but it doesn't know whether this call into __generic_file_splice_read()
initiated IO to that page or if someone else did. Naturally this will
most often be the case that __generic_file_splice_read() was the one
that initiated IO to that page, but it could be someone else of course.

The code above doesn't actually return EAGAIN, but
generic_file_splice_read() turns a 0 return for SPLICE_F_NONBLOCK into
EAGAIN.

So we need a bit of tweaking. I don't see how we can make b) work well
enough without getting hackish and breaking into the page cache and
read-ahead code, so I'd be inclined to suggest we make it conform to
your a) case. Now I didn't test yet, but it seems that just doing:

if ((flags & SPLICE_F_NONBLOCK) && spd.nr_pages) {
if (TestSetPageLocked(page))
break;
} else
lock_page(page);

should do that - always block for the first page and potentially return
a partial results for the remaining pages that read-ahead kicked into
gear.

--
Jens Axboe

2007-06-02 15:03:58

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Fri, Jun 01 2007, H. Peter Anvin wrote:
> > So there's a few things to take away from this:
> >
> > - regular file access MUST NOT return EAGAIN just because a page isn't
> > in the cache. Doing so is simply a bug. No ifs, buts or maybe's about
> > it!
> >
> > Busy-looping is NOT ACCEPTABLE!
> >
> > - you *could* make some alternative conventions:
> >
> > (a) you could make O_NONBLOCK mean that you'll at least
> > guarantee that you *start* the IO, and while you never return
> > EAGAIN, you migth validly return a _partial_ result!
> >
> > (b) variation on (a): it's ok to return EAGAIN if _you_ were the
> > one who started the IO during this particular time aroudn the
> > loop. But if you find a page that isn't up-to-date yet, and
> > you didn't start the IO, you *must* wait for it, so that you
> > end up returning EAGAIN atmost once! Exactly because
> > busy-looping is simply not acceptable behaviour!
>
> (b) seems really ugly. (a) is at least well-defined. Either seems
> wrong, though.

I totally agree, b) would get nasty. And while a) isn't perfect by any
means, I do follow Linus' logic and agree it's probably the best (only?)
way to handle it.

--
Jens Axboe

2007-06-02 15:41:36

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Sat, 2 Jun 2007, Jens Axboe wrote:
>
> splice() WILL return EAGAIN, but in that case it should have triggered
> the read-ahead and thus started some IO.

That's not enough.

If the IO has already been started, splice needs to wait.

> For the from-file case, see __generic_file_splice_read(). splice does:
>
> if (!PageUptodate(page)) {
> /*
> * If in nonblock mode then dont block on
> * waiting
> * for an in-flight io page
> */
> if (flags & SPLICE_F_NONBLOCK) {
> if (TestSetPageLocked(page))
> break;
> } else
> lock_page(page);

Yeah, that's just wrong.

Your suggested:

> if ((flags & SPLICE_F_NONBLOCK) && spd.nr_pages) {
> if (TestSetPageLocked(page))
> break;
> } else
> lock_page(page);
>
> should do that - always block for the first page and potentially return
> a partial results for the remaining pages that read-ahead kicked into
> gear.

would work, but I suspect that for a server, returning EAGAIN once is
actually the best option - if it has a select() loop, and something else
is running, the "return EAGAIN once" actually makes tons of sense (it
would basically boil down to the kernel effectively saying "ok, try
anything else you might have pending in your queues first, if you get back
to me, I'll block then").

Linus

2007-06-02 16:37:27

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Sat, Jun 02 2007, Linus Torvalds wrote:
>
>
> On Sat, 2 Jun 2007, Jens Axboe wrote:
> >
> > splice() WILL return EAGAIN, but in that case it should have triggered
> > the read-ahead and thus started some IO.
>
> That's not enough.
>
> If the IO has already been started, splice needs to wait.

But splice doesn't know, page_cache_readahead() may not have started
anything.

> > For the from-file case, see __generic_file_splice_read(). splice does:
> >
> > if (!PageUptodate(page)) {
> > /*
> > * If in nonblock mode then dont block on
> > * waiting
> > * for an in-flight io page
> > */
> > if (flags & SPLICE_F_NONBLOCK) {
> > if (TestSetPageLocked(page))
> > break;
> > } else
> > lock_page(page);
>
> Yeah, that's just wrong.
>
> Your suggested:
>
> > if ((flags & SPLICE_F_NONBLOCK) && spd.nr_pages) {
> > if (TestSetPageLocked(page))
> > break;
> > } else
> > lock_page(page);
> >
> > should do that - always block for the first page and potentially return
> > a partial results for the remaining pages that read-ahead kicked into
> > gear.
>
> would work, but I suspect that for a server, returning EAGAIN once is
> actually the best option - if it has a select() loop, and something else
> is running, the "return EAGAIN once" actually makes tons of sense (it
> would basically boil down to the kernel effectively saying "ok, try
> anything else you might have pending in your queues first, if you get back
> to me, I'll block then").

Well then the current code should work, _provided_ that we know
read-ahead kicked off the IO. Well almost, still needs a bit of
tweaking, with some knowledge of whether page_cache_readahead() actually
called into read_pages() or not.

--
Jens Axboe

2007-06-03 13:05:25

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Sat, Jun 02, 2007 at 08:40:04AM -0700, Linus Torvalds wrote:
> On Sat, 2 Jun 2007, Jens Axboe wrote:
> Your suggested:
>
> > if ((flags & SPLICE_F_NONBLOCK) && spd.nr_pages) {
> > if (TestSetPageLocked(page))
> > break;
> > } else
> > lock_page(page);
> >
> > should do that - always block for the first page and potentially return
> > a partial results for the remaining pages that read-ahead kicked into
> > gear.
>
> would work, but I suspect that for a server, returning EAGAIN once is
> actually the best option - if it has a select() loop, and something else
> is running, the "return EAGAIN once" actually makes tons of sense (it
> would basically boil down to the kernel effectively saying "ok, try
> anything else you might have pending in your queues first, if you get back
> to me, I'll block then").

May be we can settle with "return EAGAIN once" for *all* possible
waits, including I/O submitted by others:

if ((flags & SPLICE_F_NONBLOCK) &&
TestSetPageLocked(page) &&
in->f_ra.prev_index != index)
break;
else
lock_page(page);

However, the upper layer generic_file_splice_read() may keep
on calling __generic_file_splice_read(), so we need more code to
stop it with flag SPLICE_INTERNAL_WILLBLOCK:

---
(not tested yet; still not sure if it's the best solution.)

In non-block splicing, return EAGAIN once for all possible I/O waits.

It works by checking (ra.prev_index != index) in
__generic_file_splice_read(). Another reader on the same fd will at
worst cause a few more splice syscalls.

If keep calling generic_file_splice_read() in non-block mode, it will
return partial data before the first hole; then return EAGAIN at
second call; at last it will block on the I/O page.

Signed-off-by: Fengguang Wu <[email protected]>
---
fs/splice.c | 38 ++++++++++++++++++++++--------------
include/linux/pipe_fs_i.h | 2 +
2 files changed, 26 insertions(+), 14 deletions(-)

--- linux-2.6.22-rc3-mm1.orig/fs/splice.c
+++ linux-2.6.22-rc3-mm1/fs/splice.c
@@ -264,10 +264,11 @@ static ssize_t splice_to_pipe(struct pip
static int
__generic_file_splice_read(struct file *in, loff_t *ppos,
struct pipe_inode_info *pipe, size_t len,
- unsigned int flags)
+ unsigned int *flags)
{
struct address_space *mapping = in->f_mapping;
unsigned int loff, nr_pages;
+ unsigned int first_hole;
struct page *pages[PIPE_BUFFERS];
struct partial_page partial[PIPE_BUFFERS];
struct page *page;
@@ -278,7 +279,7 @@ __generic_file_splice_read(struct file *
struct splice_pipe_desc spd = {
.pages = pages,
.partial = partial,
- .flags = flags,
+ .flags = *flags,
.ops = &page_cache_pipe_buf_ops,
};

@@ -299,12 +300,17 @@ __generic_file_splice_read(struct file *
* Lookup the (hopefully) full range of pages we need.
*/
spd.nr_pages = find_get_pages_contig(mapping, index, nr_pages, pages);
+ first_hole = spd.nr_pages;
+ index += spd.nr_pages;

/*
* If find_get_pages_contig() returned fewer pages than we needed,
- * allocate the rest.
+ * readahead/allocate the rest.
*/
- index += spd.nr_pages;
+ if (spd.nr_pages < nr_pages)
+ page_cache_readahead_ondemand(mapping, &in->f_ra, in,
+ NULL, index, nr_pages - spd.nr_pages);
+
while (spd.nr_pages < nr_pages) {
/*
* Page could be there, find_get_pages_contig() breaks on
@@ -312,9 +318,6 @@ __generic_file_splice_read(struct file *
*/
page = find_get_page(mapping, index);
if (!page) {
- page_cache_readahead_ondemand(mapping, &in->f_ra, in,
- NULL, index, nr_pages - spd.nr_pages);
-
/*
* page didn't exist, allocate one.
*/
@@ -369,13 +372,13 @@ __generic_file_splice_read(struct file *
*/
if (!PageUptodate(page)) {
/*
- * If in nonblock mode then dont block on waiting
- * for an in-flight io page
+ * In non-block mode, only block at the second try.
*/
- if (flags & SPLICE_F_NONBLOCK) {
- if (TestSetPageLocked(page))
- break;
- } else
+ if ((*flags & SPLICE_F_NONBLOCK) &&
+ TestSetPageLocked(page) &&
+ in->f_ra.prev_index != index)
+ break;
+ else
lock_page(page);

/*
@@ -454,6 +457,10 @@ fill_it:
page_cache_release(pages[page_nr++]);
in->f_ra.prev_index = index;

+ if ((*flags & SPLICE_F_NONBLOCK) && (spd.nr_pages > first_hole)) {
+ *flags |= SPLICE_INTERNAL_WILLBLOCK;
+ spd.nr_pages = first_hole;
+ }
if (spd.nr_pages)
return splice_to_pipe(pipe, &spd);

@@ -480,7 +487,7 @@ ssize_t generic_file_splice_read(struct
spliced = 0;

while (len) {
- ret = __generic_file_splice_read(in, ppos, pipe, len, flags);
+ ret = __generic_file_splice_read(in, ppos, pipe, len, &flags);

if (ret < 0)
break;
@@ -496,6 +503,9 @@ ssize_t generic_file_splice_read(struct
*ppos += ret;
len -= ret;
spliced += ret;
+
+ if (flags & SPLICE_INTERNAL_WILLBLOCK)
+ break;
}

if (spliced)
--- linux-2.6.22-rc3-mm1.orig/include/linux/pipe_fs_i.h
+++ linux-2.6.22-rc3-mm1/include/linux/pipe_fs_i.h
@@ -82,6 +82,8 @@ int generic_pipe_buf_steal(struct pipe_i
#define SPLICE_F_MORE (0x04) /* expect more data */
#define SPLICE_F_GIFT (0x08) /* pages passed in are a gift */

+#define SPLICE_INTERNAL_WILLBLOCK (0x100) /* read on next page will block */
+
/*
* Passed to the actors
*/

2007-06-03 14:29:39

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Sun, Jun 03, 2007 at 09:05:08PM +0800, Fengguang Wu wrote:
> In non-block splicing, return EAGAIN once for all possible I/O waits.
>
> It works by checking (ra.prev_index != index) in
> __generic_file_splice_read(). Another reader on the same fd will at
> worst cause a few more splice syscalls.
>
> If keep calling generic_file_splice_read() in non-block mode, it will
> return partial data before the first hole; then return EAGAIN at
> second call; at last it will block on the I/O page.

Here is a better one, comments are welcome.
---

In non-block splicing read, return EAGAIN once for all possible I/O waits.

It avoids busy waiting by checking (ra.prev_index != index) in
__generic_file_splice_read(). Possible other readers on the same fd
may temporarily cheat the logic, but will at worst cause a few more
splice syscalls.

If keep calling generic_file_splice_read() in non-blocking mode, it
will return partial data before the first wait point; then return
EAGAIN at second call; at last it will block on the I/O page. This
behavior should be compatible with old apps that are unaware of
EAGAIN.

Signed-off-by: Fengguang Wu <[email protected]>
---
fs/splice.c | 33 ++++++++++++++++++---------------
include/linux/pipe_fs_i.h | 2 ++
2 files changed, 20 insertions(+), 15 deletions(-)

--- linux-2.6.22-rc3-mm1.orig/fs/splice.c
+++ linux-2.6.22-rc3-mm1/fs/splice.c
@@ -264,7 +264,7 @@ static ssize_t splice_to_pipe(struct pip
static int
__generic_file_splice_read(struct file *in, loff_t *ppos,
struct pipe_inode_info *pipe, size_t len,
- unsigned int flags)
+ unsigned int *flags)
{
struct address_space *mapping = in->f_mapping;
unsigned int loff, nr_pages;
@@ -278,7 +278,7 @@ __generic_file_splice_read(struct file *
struct splice_pipe_desc spd = {
.pages = pages,
.partial = partial,
- .flags = flags,
+ .flags = *flags,
.ops = &page_cache_pipe_buf_ops,
};

@@ -299,12 +299,16 @@ __generic_file_splice_read(struct file *
* Lookup the (hopefully) full range of pages we need.
*/
spd.nr_pages = find_get_pages_contig(mapping, index, nr_pages, pages);
+ index += spd.nr_pages;

/*
* If find_get_pages_contig() returned fewer pages than we needed,
- * allocate the rest.
+ * readahead/allocate the rest.
*/
- index += spd.nr_pages;
+ if (spd.nr_pages < nr_pages)
+ page_cache_readahead_ondemand(mapping, &in->f_ra, in,
+ NULL, index, nr_pages - spd.nr_pages);
+
while (spd.nr_pages < nr_pages) {
/*
* Page could be there, find_get_pages_contig() breaks on
@@ -312,9 +316,6 @@ __generic_file_splice_read(struct file *
*/
page = find_get_page(mapping, index);
if (!page) {
- page_cache_readahead_ondemand(mapping, &in->f_ra, in,
- NULL, index, nr_pages - spd.nr_pages);
-
/*
* page didn't exist, allocate one.
*/
@@ -326,8 +327,6 @@ __generic_file_splice_read(struct file *
GFP_KERNEL);
if (unlikely(error)) {
page_cache_release(page);
- if (error == -EEXIST)
- continue;
break;
}
/*
@@ -369,12 +368,13 @@ __generic_file_splice_read(struct file *
*/
if (!PageUptodate(page)) {
/*
- * If in nonblock mode then dont block on waiting
- * for an in-flight io page
+ * In non-block mode, only block at the second try.
*/
- if (flags & SPLICE_F_NONBLOCK) {
- if (TestSetPageLocked(page))
- break;
+ if ((*flags & SPLICE_F_NONBLOCK) &&
+ TestSetPageLocked(page) &&
+ in->f_ra.prev_index != index) {
+ *flags |= SPLICE_INTERNAL_WILLBLOCK;
+ break;
} else
lock_page(page);

@@ -480,7 +480,7 @@ ssize_t generic_file_splice_read(struct
spliced = 0;

while (len) {
- ret = __generic_file_splice_read(in, ppos, pipe, len, flags);
+ ret = __generic_file_splice_read(in, ppos, pipe, len, &flags);

if (ret < 0)
break;
@@ -496,6 +496,9 @@ ssize_t generic_file_splice_read(struct
*ppos += ret;
len -= ret;
spliced += ret;
+
+ if (flags & SPLICE_INTERNAL_WILLBLOCK)
+ break;
}

if (spliced)
--- linux-2.6.22-rc3-mm1.orig/include/linux/pipe_fs_i.h
+++ linux-2.6.22-rc3-mm1/include/linux/pipe_fs_i.h
@@ -82,6 +82,8 @@ int generic_pipe_buf_steal(struct pipe_i
#define SPLICE_F_MORE (0x04) /* expect more data */
#define SPLICE_F_GIFT (0x08) /* pages passed in are a gift */

+#define SPLICE_INTERNAL_WILLBLOCK (0x100) /* read on next page will block */
+
/*
* Passed to the actors
*/

2007-06-04 00:46:57

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

Hi Jens,

This is another try, still not in a comfortable state though.
//Busy waiting is possible for interleaved reads.

Fengguang
---

In non-block splicing read, return EAGAIN once for possible I/O waits.

It avoids busy waiting by checking (ra.prev_index != index) in
__generic_file_splice_read(). If there are other readers on the same
fd, busy waiting is still possible.

If keep calling generic_file_splice_read() in non-blocking mode, it
will return partial data before the first wait point; then return
EAGAIN at second call; at last it will block on the I/O page.

Signed-off-by: Fengguang Wu <[email protected]>
---
fs/splice.c | 41 +++++++++++++++++++++---------------
include/linux/pipe_fs_i.h | 2 +
2 files changed, 27 insertions(+), 16 deletions(-)

--- linux-2.6.22-rc3-mm1.orig/fs/splice.c
+++ linux-2.6.22-rc3-mm1/fs/splice.c
@@ -264,7 +264,7 @@ static ssize_t splice_to_pipe(struct pip
static int
__generic_file_splice_read(struct file *in, loff_t *ppos,
struct pipe_inode_info *pipe, size_t len,
- unsigned int flags)
+ unsigned int *flags)
{
struct address_space *mapping = in->f_mapping;
unsigned int loff, nr_pages;
@@ -278,7 +278,7 @@ __generic_file_splice_read(struct file *
struct splice_pipe_desc spd = {
.pages = pages,
.partial = partial,
- .flags = flags,
+ .flags = *flags,
.ops = &page_cache_pipe_buf_ops,
};

@@ -299,12 +299,16 @@ __generic_file_splice_read(struct file *
* Lookup the (hopefully) full range of pages we need.
*/
spd.nr_pages = find_get_pages_contig(mapping, index, nr_pages, pages);
+ index += spd.nr_pages;

/*
* If find_get_pages_contig() returned fewer pages than we needed,
- * allocate the rest.
+ * readahead/allocate the rest.
*/
- index += spd.nr_pages;
+ if (spd.nr_pages < nr_pages)
+ page_cache_readahead_ondemand(mapping, &in->f_ra, in,
+ NULL, index, nr_pages - spd.nr_pages);
+
while (spd.nr_pages < nr_pages) {
/*
* Page could be there, find_get_pages_contig() breaks on
@@ -312,9 +316,6 @@ __generic_file_splice_read(struct file *
*/
page = find_get_page(mapping, index);
if (!page) {
- page_cache_readahead_ondemand(mapping, &in->f_ra, in,
- NULL, index, nr_pages - spd.nr_pages);
-
/*
* page didn't exist, allocate one.
*/
@@ -326,8 +327,6 @@ __generic_file_splice_read(struct file *
GFP_KERNEL);
if (unlikely(error)) {
page_cache_release(page);
- if (error == -EEXIST)
- continue;
break;
}
/*
@@ -369,12 +368,19 @@ __generic_file_splice_read(struct file *
*/
if (!PageUptodate(page)) {
/*
- * If in nonblock mode then dont block on waiting
- * for an in-flight io page
+ * On retrying sequential reads in non-blocking mode:
+ * 1. (prev_index <= index - 2) => return partial data
+ * 2. (prev_index == index - 1) => return EAGAIN
+ * 3. (prev_index == index) => wait on I/O
+ * This works nicely for page aligned reads.
*/
- if (flags & SPLICE_F_NONBLOCK) {
- if (TestSetPageLocked(page))
- break;
+ if ((*flags & SPLICE_F_NONBLOCK) &&
+ TestSetPageLocked(page) &&
+ in->f_ra.prev_index != index) {
+ if (in->f_ra.prev_index != index - 1)
+ index--;
+ *flags |= SPLICE_INTERNAL_WILLBLOCK;
+ break;
} else
lock_page(page);

@@ -452,7 +458,6 @@ fill_it:
*/
while (page_nr < nr_pages)
page_cache_release(pages[page_nr++]);
- in->f_ra.prev_index = index;

if (spd.nr_pages)
return splice_to_pipe(pipe, &spd);
@@ -480,7 +485,7 @@ ssize_t generic_file_splice_read(struct
spliced = 0;

while (len) {
- ret = __generic_file_splice_read(in, ppos, pipe, len, flags);
+ ret = __generic_file_splice_read(in, ppos, pipe, len, &flags);

if (ret < 0)
break;
@@ -496,8 +501,12 @@ ssize_t generic_file_splice_read(struct
*ppos += ret;
len -= ret;
spliced += ret;
+
+ if (flags & SPLICE_INTERNAL_WILLBLOCK)
+ break;
}

+ in->f_ra.prev_index = (*ppos - 1) >> PAGE_CACHE_SHIFT;
if (spliced)
return spliced;

--- linux-2.6.22-rc3-mm1.orig/include/linux/pipe_fs_i.h
+++ linux-2.6.22-rc3-mm1/include/linux/pipe_fs_i.h
@@ -82,6 +82,8 @@ int generic_pipe_buf_steal(struct pipe_i
#define SPLICE_F_MORE (0x04) /* expect more data */
#define SPLICE_F_GIFT (0x08) /* pages passed in are a gift */

+#define SPLICE_INTERNAL_WILLBLOCK (0x100) /* read on next page will block */
+
/*
* Passed to the actors
*/

2007-06-04 08:07:31

[permalink] [raw]

Subject: Re: [PATCH] sendfile removal

On Mon, Jun 04 2007, Fengguang Wu wrote:
> Hi Jens,
>
> This is another try, still not in a comfortable state though.
> //Busy waiting is possible for interleaved reads.

A few random comments...

Adding an internal flag is fine, but please put it at the upper end of
the spectrum. So, use (1 << 31) for that flag.

And please work on the #splice branch of the block repo, not -mm. There
are quite a few things pending for inclusion in there, and I'm sure your
patch as-is wont apply.

--
Jens Axboe

2007-06-04 11:22:23