This patch pulls calls to buf->ops->confirm() from all actors passed
(also indirectly) to splice_from_pipe_feed().
Is avoiding the call to buf->ops->confirm() while splice()ing to
/dev/null is an intentional optimization? No other user does that
and this will remove this special case.
Against current linux.git 6313e3c21743cc88bb5bd8aa72948ee1e83937b6.
Signed-off-by: Michał Mirosław <[email protected]>
---
drivers/block/loop.c | 4 ----
fs/nfsd/vfs.c | 4 ----
fs/splice.c | 43 ++++++++++++++-----------------------------
3 files changed, 14 insertions(+), 37 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 7ea0bea..c87b084 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -397,10 +397,6 @@ lo_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
sector_t IV;
int size, ret;
- ret = buf->ops->confirm(pipe, buf);
- if (unlikely(ret))
- return ret;
-
IV = ((sector_t) page->index << (PAGE_CACHE_SHIFT - 9)) +
(buf->offset >> 9);
size = sd->len;
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 184938f..c6e0866 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -847,10 +847,6 @@ nfsd_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
size_t size;
int ret;
- ret = buf->ops->confirm(pipe, buf);
- if (unlikely(ret))
- return ret;
-
size = sd->len;
if (rqstp->rq_res.page_len == 0) {
diff --git a/fs/splice.c b/fs/splice.c
index ce2f025..50a5d978 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -682,19 +682,14 @@ static int pipe_to_sendpage(struct pipe_inode_info *pipe,
{
struct file *file = sd->u.file;
loff_t pos = sd->pos;
- int ret, more;
-
- ret = buf->ops->confirm(pipe, buf);
- if (!ret) {
- more = (sd->flags & SPLICE_F_MORE) || sd->len < sd->total_len;
- if (file->f_op && file->f_op->sendpage)
- ret = file->f_op->sendpage(file, buf->page, buf->offset,
- sd->len, &pos, more);
- else
- ret = -EINVAL;
- }
+ int more;
- return ret;
+ if (!likely(file->f_op && file->f_op->sendpage))
+ return -EINVAL;
+
+ more = (sd->flags & SPLICE_F_MORE) || sd->len < sd->total_len;
+ return file->f_op->sendpage(file, buf->page, buf->offset,
+ sd->len, &pos, more);
}
/*
@@ -727,13 +722,6 @@ int pipe_to_file(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
void *fsdata;
int ret;
- /*
- * make sure the data in this buffer is uptodate
- */
- ret = buf->ops->confirm(pipe, buf);
- if (unlikely(ret))
- return ret;
-
offset = sd->pos & ~PAGE_CACHE_MASK;
this_len = sd->len;
@@ -805,12 +793,17 @@ int splice_from_pipe_feed(struct pipe_inode_info *pipe, struct splice_desc *sd,
if (sd->len > sd->total_len)
sd->len = sd->total_len;
- ret = actor(pipe, buf, sd);
- if (ret <= 0) {
+ ret = buf->ops->confirm(pipe, buf);
+ if (unlikely(ret)) {
if (ret == -ENODATA)
ret = 0;
return ret;
}
+
+ ret = actor(pipe, buf, sd);
+ if (ret <= 0)
+ return ret;
+
buf->offset += ret;
buf->len -= ret;
@@ -1044,10 +1037,6 @@ static int write_pipe_buf(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
int ret;
void *data;
- ret = buf->ops->confirm(pipe, buf);
- if (ret)
- return ret;
-
data = buf->ops->map(pipe, buf, 0);
ret = kernel_write(sd->u.file, data + buf->offset, sd->len, sd->pos);
buf->ops->unmap(pipe, buf, data);
@@ -1495,10 +1484,6 @@ static int pipe_to_user(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
char *src;
int ret;
- ret = buf->ops->confirm(pipe, buf);
- if (unlikely(ret))
- return ret;
-
/*
* See if we can use the atomic maps, by prefaulting in the
* pages and doing an atomic copy
--
1.7.2.3
On 2010-12-12 22:23, Michał Mirosław wrote:
> This patch pulls calls to buf->ops->confirm() from all actors passed
> (also indirectly) to splice_from_pipe_feed().
Why? The point of ->confirm() is to ensure that the contents are
stable, otherwise the pages in the pipe could merely be in flight.
It's needed if you need to actually look at the data, rather than just
reference it.
--
Jens Axboe
On Mon, Dec 13, 2010 at 02:38:19PM +0100, Jens Axboe wrote:
> On 2010-12-12 22:23, Micha? Miros?aw wrote:
> > This patch pulls calls to buf->ops->confirm() from all actors passed
> > (also indirectly) to splice_from_pipe_feed().
> Why? The point of ->confirm() is to ensure that the contents are
> stable, otherwise the pages in the pipe could merely be in flight.
> It's needed if you need to actually look at the data, rather than just
> reference it.
I should have put this more clearly in the patch description:
the ->confirm() call is moved to splice_from_pipe_feed(), so that every
actor has its data guaranteed to be stable before it runs.
Best Regards,
Micha? Miros?aw
On 2010-12-13 16:04, Micha? Miros?aw wrote:
> On Mon, Dec 13, 2010 at 02:38:19PM +0100, Jens Axboe wrote:
>> On 2010-12-12 22:23, Micha? Miros?aw wrote:
>>> This patch pulls calls to buf->ops->confirm() from all actors passed
>>> (also indirectly) to splice_from_pipe_feed().
>> Why? The point of ->confirm() is to ensure that the contents are
>> stable, otherwise the pages in the pipe could merely be in flight.
>> It's needed if you need to actually look at the data, rather than just
>> reference it.
>
> I should have put this more clearly in the patch description:
> the ->confirm() call is moved to splice_from_pipe_feed(), so that every
> actor has its data guaranteed to be stable before it runs.
OK, that makes more sense. I'll queue it up.
--
Jens Axboe