[patch 1/2] splice: dont steal

[patch 1/2] splice: dont steal

[Nick Piggin]

Here are a couple of splice patches I found when digging in the area.
I could be wrong, so I'd appreciate confirmation.

Untested other than compile, because I don't have a good splice test
setup.

Considering these are data corruption / information leak issues, then
we could do worse than to merge them in 2.6.21 and earlier stable
trees.

Does anyone really use splice stealing?

--

Stealing pages with splice is problematic because we cannot just insert
an uptodate page into the pagecache and hope the filesystem can take care
of it later.

We also cannot just ClearPageUptodate, then hope prepare_write does not
write anything into the page, because I don't think prepare_write gives
that guarantee.

Remove support for SPLICE_F_MOVE for now. If we really want to bring it
back, we might be able to do so with a the new filesystem buffered write
aops APIs I'm working on. If we really don't want to bring it back, then
we should decide that sooner rather than later, and remove the flag and
all the stealing infrastructure before anybody starts using it.

Signed-off-by: Nick Piggin <npiggin@suse.de>

Index: linux-2.6/fs/splice.c
===================================================================
--- linux-2.6.orig/fs/splice.c
+++ linux-2.6/fs/splice.c
@@ -576,76 +576,51 @@ static int pipe_to_file(struct pipe_inod
 	if (this_len + offset > PAGE_CACHE_SIZE)
 		this_len = PAGE_CACHE_SIZE - offset;
 
-	/*
-	 * Reuse buf page, if SPLICE_F_MOVE is set and we are doing a full
-	 * page.
-	 */
-	if ((sd->flags & SPLICE_F_MOVE) && this_len == PAGE_CACHE_SIZE) {
+find_page:
+	page = find_lock_page(mapping, index);
+	if (!page) {
+		ret = -ENOMEM;
+		page = page_cache_alloc_cold(mapping);
+		if (unlikely(!page))
+			goto out_ret;
+
 		/*
-		 * If steal succeeds, buf->page is now pruned from the
-		 * pagecache and we can reuse it. The page will also be
-		 * locked on successful return.
+		 * This will also lock the page
 		 */
-		if (buf->ops->steal(pipe, buf))
-			goto find_page;
-
-		page = buf->page;
-		if (add_to_page_cache(page, mapping, index, GFP_KERNEL)) {
-			unlock_page(page);
-			goto find_page;
-		}
-
-		page_cache_get(page);
+		ret = add_to_page_cache_lru(page, mapping, index,
+					    GFP_KERNEL);
+		if (unlikely(ret))
+			goto out;
+	}
 
-		if (!(buf->flags & PIPE_BUF_FLAG_LRU))
-			lru_cache_add(page);
-	} else {
-find_page:
-		page = find_lock_page(mapping, index);
-		if (!page) {
-			ret = -ENOMEM;
-			page = page_cache_alloc_cold(mapping);
-			if (unlikely(!page))
-				goto out_ret;
-
-			/*
-			 * This will also lock the page
-			 */
-			ret = add_to_page_cache_lru(page, mapping, index,
-						    GFP_KERNEL);
+	/*
+	 * We get here with the page locked. If the page is also
+	 * uptodate, we don't need to do more. If it isn't, we
+	 * may need to bring it in if we are not going to overwrite
+	 * the full page.
+	 */
+	if (!PageUptodate(page)) {
+		if (this_len < PAGE_CACHE_SIZE) {
+			ret = mapping->a_ops->readpage(file, page);
 			if (unlikely(ret))
 				goto out;
-		}
 
-		/*
-		 * We get here with the page locked. If the page is also
-		 * uptodate, we don't need to do more. If it isn't, we
-		 * may need to bring it in if we are not going to overwrite
-		 * the full page.
-		 */
-		if (!PageUptodate(page)) {
-			if (this_len < PAGE_CACHE_SIZE) {
-				ret = mapping->a_ops->readpage(file, page);
-				if (unlikely(ret))
-					goto out;
-
-				lock_page(page);
-
-				if (!PageUptodate(page)) {
-					/*
-					 * Page got invalidated, repeat.
-					 */
-					if (!page->mapping) {
-						unlock_page(page);
-						page_cache_release(page);
-						goto find_page;
-					}
-					ret = -EIO;
-					goto out;
+			lock_page(page);
+
+			if (!PageUptodate(page)) {
+				/*
+				 * Page got invalidated, repeat.
+				 */
+				if (!page->mapping) {
+					unlock_page(page);
+					page_cache_release(page);
+					goto find_page;
 				}
-			} else
-				SetPageUptodate(page);
-		}
+				ret = -EIO;
+				goto out;
+			}
+		} else
+			SetPageUptodate(page);
 	}
 
 	ret = mapping->a_ops->prepare_write(file, page, offset, offset+this_len);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[patch 2/2] splice: dont readpage

[Nick Piggin]

Splice does not need to readpage to bring the page uptodate before writing
to it, because prepare_write will take care of that for us.

Splice is also wrong to SetPageUptodate before the page is actually uptodate.
This results in the old uninitialised memory leak. This gets fixed as a
matter of course when removing the readpage logic.

Signed-off-by: Nick Piggin <npiggin@suse.de>

Index: linux-2.6/fs/splice.c
===================================================================
--- linux-2.6.orig/fs/splice.c
+++ linux-2.6/fs/splice.c
@@ -593,36 +593,6 @@ find_page:
 			goto out;
 	}
 
-	/*
-	 * We get here with the page locked. If the page is also
-	 * uptodate, we don't need to do more. If it isn't, we
-	 * may need to bring it in if we are not going to overwrite
-	 * the full page.
-	 */
-	if (!PageUptodate(page)) {
-		if (this_len < PAGE_CACHE_SIZE) {
-			ret = mapping->a_ops->readpage(file, page);
-			if (unlikely(ret))
-				goto out;
-
-			lock_page(page);
-
-			if (!PageUptodate(page)) {
-				/*
-				 * Page got invalidated, repeat.
-				 */
-				if (!page->mapping) {
-					unlock_page(page);
-					page_cache_release(page);
-					goto find_page;
-				}
-				ret = -EIO;
-				goto out;
-			}
-		} else
-			SetPageUptodate(page);
-	}
-
 	ret = mapping->a_ops->prepare_write(file, page, offset, offset+this_len);
 	if (unlikely(ret)) {
 		loff_t isize = i_size_read(mapping->host);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 2/2] splice: dont readpage

[Jens Axboe]

On Wed, Mar 14 2007, Nick Piggin wrote:
> 
> Splice does not need to readpage to bring the page uptodate before writing
> to it, because prepare_write will take care of that for us.

Ah great, always good to get rid of some code.

> Splice is also wrong to SetPageUptodate before the page is actually uptodate.
> This results in the old uninitialised memory leak. This gets fixed as a
> matter of course when removing the readpage logic.

Leak, how? The page should still be locked all through to the copy.
Anyway, doesn't matter since you've killed it anyway. I have applied
this patch.

-- 
Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 2/2] splice: dont readpage

[Nick Piggin]

On Thu, Mar 15, 2007 at 12:54:54PM +0100, Jens Axboe wrote:
> On Wed, Mar 14 2007, Nick Piggin wrote:
> > 
> > Splice does not need to readpage to bring the page uptodate before writing
> > to it, because prepare_write will take care of that for us.
> 
> Ah great, always good to get rid of some code.

Yeah, it should especially make block (but not page) sized and aligned
writes into uncached files work much better, AFAIKS (won't require the
synchronous read).

> > Splice is also wrong to SetPageUptodate before the page is actually uptodate.
> > This results in the old uninitialised memory leak. This gets fixed as a
> > matter of course when removing the readpage logic.
> 
> Leak, how? The page should still be locked all through to the copy.
> Anyway, doesn't matter since you've killed it anyway. I have applied
> this patch.

The read side doesn't need to lock the page if it is uptodate, and doesn't.

Nick

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 2/2] splice: dont readpage

[Jens Axboe]

On Thu, Mar 15 2007, Nick Piggin wrote:
> On Thu, Mar 15, 2007 at 12:54:54PM +0100, Jens Axboe wrote:
> > On Wed, Mar 14 2007, Nick Piggin wrote:
> > > 
> > > Splice does not need to readpage to bring the page uptodate before writing
> > > to it, because prepare_write will take care of that for us.
> > 
> > Ah great, always good to get rid of some code.
> 
> Yeah, it should especially make block (but not page) sized and aligned
> writes into uncached files work much better, AFAIKS (won't require the
> synchronous read).

Yep, it's a nice improvement! Plus a cleanup.

> > > Splice is also wrong to SetPageUptodate before the page is actually uptodate.
> > > This results in the old uninitialised memory leak. This gets fixed as a
> > > matter of course when removing the readpage logic.
> > 
> > Leak, how? The page should still be locked all through to the copy.
> > Anyway, doesn't matter since you've killed it anyway. I have applied
> > this patch.
> 
> The read side doesn't need to lock the page if it is uptodate, and doesn't.

Oh, then there's definitely an issue.

-- 
Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/2] splice: dont steal

[Jens Axboe]

On Wed, Mar 14 2007, Nick Piggin wrote:
> Here are a couple of splice patches I found when digging in the area.
> I could be wrong, so I'd appreciate confirmation.
> 
> Untested other than compile, because I don't have a good splice test
> setup.
> 
> Considering these are data corruption / information leak issues, then
> we could do worse than to merge them in 2.6.21 and earlier stable
> trees.
> 
> Does anyone really use splice stealing?

That's a damn shame, I'd greatly prefer if we can try and fix it
instead. Splice isn't really all that used yet to my knowledge, but
stealing is one of the niftier features I think. Otherwise you're just
copying data again.

-- 
Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/2] splice: dont steal

[Nick Piggin]

On Thu, Mar 15, 2007 at 12:52:37PM +0100, Jens Axboe wrote:
> On Wed, Mar 14 2007, Nick Piggin wrote:
> > Here are a couple of splice patches I found when digging in the area.
> > I could be wrong, so I'd appreciate confirmation.
> > 
> > Untested other than compile, because I don't have a good splice test
> > setup.
> > 
> > Considering these are data corruption / information leak issues, then
> > we could do worse than to merge them in 2.6.21 and earlier stable
> > trees.
> > 
> > Does anyone really use splice stealing?
> 
> That's a damn shame, I'd greatly prefer if we can try and fix it
> instead. Splice isn't really all that used yet to my knowledge, but
> stealing is one of the niftier features I think. Otherwise you're just
> copying data again.

We should be able to allow for it with the new a_ops API I'm working
on.

Basically we can pass the page down to the filesystem, and tell it to
attempt to install that page in-place.

The problem is that we can't just put this page here hoping the fs can
take it, becaue it might fail allocating blocks, for example.

Anyway, we can still copy files with 1 less copy than read/write ;)

It is a nifty feature, but I think it is more of a niche than simply
saving that 1 copy, because you have to know that the source isn't
going to be used again.

But I'll try to support it with begin_write.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/2] splice: dont steal

[Jens Axboe]

On Thu, Mar 15 2007, Nick Piggin wrote:
> On Thu, Mar 15, 2007 at 12:52:37PM +0100, Jens Axboe wrote:
> > On Wed, Mar 14 2007, Nick Piggin wrote:
> > > Here are a couple of splice patches I found when digging in the area.
> > > I could be wrong, so I'd appreciate confirmation.
> > > 
> > > Untested other than compile, because I don't have a good splice test
> > > setup.
> > > 
> > > Considering these are data corruption / information leak issues, then
> > > we could do worse than to merge them in 2.6.21 and earlier stable
> > > trees.
> > > 
> > > Does anyone really use splice stealing?
> > 
> > That's a damn shame, I'd greatly prefer if we can try and fix it
> > instead. Splice isn't really all that used yet to my knowledge, but
> > stealing is one of the niftier features I think. Otherwise you're just
> > copying data again.
> 
> We should be able to allow for it with the new a_ops API I'm working
> on.

"Should be" and in progress stuff, is it guarenteed to get there?

> Basically we can pass the page down to the filesystem, and tell it to
> attempt to install that page in-place.
> 
> The problem is that we can't just put this page here hoping the fs can
> take it, becaue it might fail allocating blocks, for example.
> 
> Anyway, we can still copy files with 1 less copy than read/write ;)

It's not about 1 vs 2 copies, it's more about 0 vs 1 copy. But yes, we
can file copy with less copies.

> It is a nifty feature, but I think it is more of a niche than simply
> saving that 1 copy, because you have to know that the source isn't
> going to be used again.

Well yes, same as when you free() a page. A little more tricky, but
that's mainly the vm assumptions/requirements for page stealing.

> But I'll try to support it with begin_write.

-- 
Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/2] splice: dont steal

[Nick Piggin]

On Thu, Mar 15, 2007 at 01:27:23PM +0100, Jens Axboe wrote:
> On Thu, Mar 15 2007, Nick Piggin wrote:
> > 
> > We should be able to allow for it with the new a_ops API I'm working
> > on.
> 
> "Should be" and in progress stuff, is it guarenteed to get there?

Well considering that it is needed in order to solve 3 different deadlock
scenarios in the core write(2) path without taking a big performance hit,
I'd hope so ;)

It isn't guaranteed, but I have only had positive feedback so far. Would
take a while to actually get merged, though.

Nick

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/2] splice: dont steal

[Jens Axboe]

On Thu, Mar 15 2007, Nick Piggin wrote:
> On Thu, Mar 15, 2007 at 01:27:23PM +0100, Jens Axboe wrote:
> > On Thu, Mar 15 2007, Nick Piggin wrote:
> > > 
> > > We should be able to allow for it with the new a_ops API I'm working
> > > on.
> > 
> > "Should be" and in progress stuff, is it guarenteed to get there?
> 
> Well considering that it is needed in order to solve 3 different deadlock
> scenarios in the core write(2) path without taking a big performance hit,
> I'd hope so ;)
> 
> It isn't guaranteed, but I have only had positive feedback so far. Would
> take a while to actually get merged, though.

It's not that I don't believe you, I'm just a little reluctant to rip
stuff out with a promise to fix it later when foo and bar are merged,
since things like that have a tendency not to get done because they are
forgotten :-)

Do you have a test case for stealing failures? What I'm really asking is
how critical is this?

-- 
Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/2] splice: dont steal

[Nick Piggin]

On Thu, Mar 15, 2007 at 01:54:32PM +0100, Jens Axboe wrote:
> On Thu, Mar 15 2007, Nick Piggin wrote:
> > On Thu, Mar 15, 2007 at 01:27:23PM +0100, Jens Axboe wrote:
> > > On Thu, Mar 15 2007, Nick Piggin wrote:
> > > > 
> > > > We should be able to allow for it with the new a_ops API I'm working
> > > > on.
> > > 
> > > "Should be" and in progress stuff, is it guarenteed to get there?
> > 
> > Well considering that it is needed in order to solve 3 different deadlock
> > scenarios in the core write(2) path without taking a big performance hit,
> > I'd hope so ;)
> > 
> > It isn't guaranteed, but I have only had positive feedback so far. Would
> > take a while to actually get merged, though.
> 
> It's not that I don't believe you, I'm just a little reluctant to rip
> stuff out with a promise to fix it later when foo and bar are merged,
> since things like that have a tendency not to get done because they are
> forgotten :-)

Fair enough. The API side is trivial, all I need to do is set a single
flag and make splice pass down the page, and set that flag when stealing.
Filesystems might vary from trivial to impossible, but I think most should
be OK. If the flag is there then they at least have the option.


> Do you have a test case for stealing failures? What I'm really asking is
> how critical is this?

I guess you could fill a filesystem completely, and have a sparse file
in it. Then steal a page and splice it in. The prepare_write should fail,
but the page will still be in pagecache, until it gets reclaimed, then
it will go back to zeroes.

(no I don't have a test case ;)).

You could do something like remove the page if prepare_write fails, but
there is still a window where a read can see it. Basically I can't see
a way that it can possibly work within our current prepare_write API,
and it is a data corruption bug, so in my opinion it is a candidate for
2.6.21 + stable.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>