All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] fs/splice: Pull buf->ops->confirm() from splice_from_pipe actors
@ 2010-12-12 21:23 ` Michał Mirosław
  0 siblings, 0 replies; 6+ messages in thread
From: Michał Mirosław @ 2010-12-12 21:23 UTC (permalink / raw)
  To: Alexander Viro, linux-fsdevel
  Cc: linux-kernel, J. Bruce Fields, Neil Brown, Jens Axboe

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 <mirq-linux@rere.qmqm.pl>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH] fs/splice: Pull buf->ops->confirm() from splice_from_pipe actors
@ 2010-12-12 21:23 ` Michał Mirosław
  0 siblings, 0 replies; 6+ messages in thread
From: Michał Mirosław @ 2010-12-12 21:23 UTC (permalink / raw)
  To: Alexander Viro, linux-fsdevel
  Cc: linux-kernel, J. Bruce Fields, Neil Brown, Jens Axboe

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 <mirq-linux@rere.qmqm.pl>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/splice: Pull buf->ops->confirm() from splice_from_pipe actors
  2010-12-12 21:23 ` Michał Mirosław
@ 2010-12-13 13:38   ` Jens Axboe
  -1 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2010-12-13 13:38 UTC (permalink / raw)
  To: Michał Mirosław
  Cc: Alexander Viro, linux-fsdevel, linux-kernel, J. Bruce Fields, Neil Brown

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/splice: Pull buf->ops->confirm() from splice_from_pipe actors
@ 2010-12-13 13:38   ` Jens Axboe
  0 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2010-12-13 13:38 UTC (permalink / raw)
  To: Michał Mirosław
  Cc: Alexander Viro, linux-fsdevel, linux-kernel, J. Bruce Fields, Neil Brown

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

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/splice: Pull buf->ops->confirm() from splice_from_pipe actors
  2010-12-13 13:38   ` Jens Axboe
  (?)
@ 2010-12-13 15:04   ` Michał Mirosław
  2010-12-14 20:12     ` Jens Axboe
  -1 siblings, 1 reply; 6+ messages in thread
From: Michał Mirosław @ 2010-12-13 15:04 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Alexander Viro, linux-fsdevel, linux-kernel, J. Bruce Fields, Neil Brown

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/splice: Pull buf->ops->confirm() from  splice_from_pipe actors
  2010-12-13 15:04   ` Michał Mirosław
@ 2010-12-14 20:12     ` Jens Axboe
  0 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2010-12-14 20:12 UTC (permalink / raw)
  To: Michał Mirosław
  Cc: Alexander Viro, linux-fsdevel, linux-kernel, J. Bruce Fields, Neil Brown

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2010-12-14 20:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-12 21:23 [PATCH] fs/splice: Pull buf->ops->confirm() from splice_from_pipe actors Michał Mirosław
2010-12-12 21:23 ` Michał Mirosław
2010-12-13 13:38 ` Jens Axboe
2010-12-13 13:38   ` Jens Axboe
2010-12-13 15:04   ` Michał Mirosław
2010-12-14 20:12     ` Jens Axboe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.