data loss in 2.6.9-rc1-mm1

I've hit the following data loss problem under 2.6.9-rc1-mm1.

If I copy data from a file to another the target will be smaller then the source file.

$ uname -r
2.6.9-rc1-mm1

$ dd if=/dev/zero of=testfile bs=$((1024*1024)) count=10
10+0 records in
10+0 records out
10485760 bytes transferred in 0.028418 seconds (368981986 bytes/sec)

$ du -sb testfile
10485760        testfile

$ cat testfile > testfile.1

$ du -sb testfile.1
10481664        testfile.1

The difference is exactly 4096 bytes, i.e. 1 whole page. Seems like an off-by-one error somewhere in the file access or page cache code.

It would be interesting to know whether the read is truncated or whether the write is truncated. So could you tell us what is returned by:

cat testfile | wc -c

Also your .config would probably be helpful.

The difference is exactly 4096 bytes, i.e. 1 whole page. Seems like an off-by-one error somewhere in the file access or page cache code.

It would be interesting to know whether the read is truncated or whether the write is truncated.

$ uname -r
2.6.9-rc1-mm1

$ dd if=/dev/zero of=testfile bs=$((1024*1024)) count=10
10+0 records in
10+0 records out
10485760 bytes transferred in 0.028418 seconds (368981986 bytes/sec)

$ du -sb testfile
10485760        testfile

$ cat testfile > testfile.1

$ du -sb testfile.1
10481664        testfile.1

What does ls -l testfile.1 give?

What you describe actually can be correct behaviour, since the file is all zeros.

Although yes, it seems highly improbable someone implemented an optimization that cuts away just one of 2560 pages.

Hi!

I've hit the following data loss problem under 2.6.9-rc1-mm1.

If I copy data from a file to another the target will be smaller then the source file.

2.6.9-rc1 does not have this problem
2.6.8.1-mm4 does not have this problem
2.6.9-rc1-mm1 _does have_ this problem

I tried this with reiserfs and xfs and it happened with both of them.

Hi!

I've hit the following data loss problem under 2.6.9-rc1-mm1.

If I copy data from a file to another the target will be smaller then the source file.

2.6.9-rc1 does not have this problem
2.6.8.1-mm4 does not have this problem
2.6.9-rc1-mm1 _does have_ this problem

Quoted 9 lines · Gergely Tamas

I've hit the following data loss problem under 2.6.9-rc1-mm1.

If I copy data from a file to another the target will be smaller then the source file.

2.6.9-rc1 does not have this problem
2.6.8.1-mm4 does not have this problem
2.6.9-rc1-mm1 _does have_ this problem

I've seen some errors from KDE too. Let me do your test...

# dd if=/dev/zero of=testfile bs=$((1024*1024)) count=10
10+0 records in
10+0 records out
# cat testfile > testfile.1
# ls -l test*

Hmm, 2.6.9-rc1-mm1 looks like not a release to trust your (page size multiple) data to! You should find the patch below fixes it (and, I hope, the issue the erroneous patches were trying to fix).

Signed-off-by Hugh Dickins <[email protected]>

Quoted 5 lines

Hmm, 2.6.9-rc1-mm1 looks like not a release to trust your (page size multiple) data to! You should find the patch below fixes it (and, I hope, the issue the erroneous patches were trying to fix).

Signed-off-by Hugh Dickins <[email protected]>

Quoted 13 lines · Denis Vlasenko

Quoted 9 lines · Gergely Tamas

I've hit the following data loss problem under 2.6.9-rc1-mm1.

If I copy data from a file to another the target will be smaller then the source file.

2.6.9-rc1 does not have this problem
2.6.8.1-mm4 does not have this problem
2.6.9-rc1-mm1 _does have_ this problem

I've seen some errors from KDE too. Let me do your test...

Review

Quoted 7 lines

# dd if=/dev/zero of=testfile bs=$((1024*1024)) count=10
10+0 records in
10+0 records out
# cat testfile > testfile.1
# ls -l test*

(quoted diff) +0 -2

-rw-r--r--    1 root     root     10485760 Aug 27 14:53 testfile
-rw-r--r--    1 root     root     10481664 Aug 27 14:53 testfile.1

Hmm, 2.6.9-rc1-mm1 looks like not a release to trust your (page size multiple) data to! You should find the patch below fixes it (and, I hope, the issue the erroneous patches were trying to fix).

Signed-off-by Hugh Dickins <[email protected]>

mm/filemap.c +8 -6

@@ -722,10 +722,7 @@ void do_generic_mapping_read(struct addr
 	offset = *ppos & ~PAGE_CACHE_MASK;
 
 	isize = i_size_read(inode);
-	if (!isize)
-		goto out;
-
-	end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
+	end_index = isize >> PAGE_CACHE_SHIFT;
 
 	for (;;) {
 		struct page *page;

@@ -733,6 +730,11 @@ void do_generic_mapping_read(struct addr
 
 		if (index > end_index)
 			goto out;
+		if (index == end_index) {
+			nr = isize & ~PAGE_CACHE_MASK;
+			if (nr <= offset)
+				goto out;
+		}
 
 		cond_resched();
 		page_cache_readahead(mapping, &ra, filp, index);

@@ -831,8 +833,8 @@ readpage:
 		 * another truncate extends the file - this is desired though).
 		 */
 		isize = i_size_read(inode);
-		end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
-		if (unlikely(!isize || index > end_index)) {
+		end_index = isize >> PAGE_CACHE_SHIFT;
+		if (unlikely(index > end_index)) {
 			page_cache_release(page);
 			goto out;
 		}

Quoted 4 lines · Hugh Dickins

Hmm, 2.6.9-rc1-mm1 looks like not a release to trust your (page size multiple) data to! You should find the patch below fixes it (and, I hope, the issue the erroneous patches were trying to fix).

Hmm.. now I fail to understand how this code works.

assuming page size is 4096, if the size of the file is 4096, is the end_index 0 or is it 1?

I had this assumption:

	file size in bytes			end_index
	-----------------			---------
		1 to 4096			0
		4097 to 2*4096			1
		2*4096+1 to 3*4096		2
		...				..

or is the isize value reported by i_size_read(inode) one less than the size of the real file?

What am I missing?

Quoted 10 lines · Ram Pai

Quoted 4 lines · Hugh Dickins

Hmm, 2.6.9-rc1-mm1 looks like not a release to trust your (page size multiple) data to! You should find the patch below fixes it (and, I hope, the issue the erroneous patches were trying to fix).

Hmm.. now I fail to understand how this code works.

assuming page size is 4096, if the size of the file is 4096, is the end_index 0 or is it 1?

Before your change and after mine, 1; with your change, 0.

Quoted 8 lines

I had this assumption:

	file size in bytes			end_index
	-----------------			---------
		1 to 4096			0
		4097 to 2*4096			1
		2*4096+1 to 3*4096		2
		...				..

Well, that's what you changed it to, when you patched from the original

		end_index = isize >> PAGE_CACHE_SHIFT;

to end_index = (isize - 1) >> PAGE_CACHE_SHIFT;

But the "nr <= offset" check(s) relies on the original convention:

		0 to 4095			0
		4096 to 8191			1
		...				..

Quoted 2 lines

or is the isize value reported by i_size_read(inode) one less than the size of the real file?

No!

Quoted 1 line

What am I missing?

You're expecting end_index to be the index of the last (possibly incomplete) page of the file. And that might be a reasonable way of working it (though the special case of an empty file hints not). But the nr,offset checks (I say checks because I added another like the one further down, hopefully to fix the extra readahead issue) require the original convention. Just try it out with numbers.

Hugh

got it! Everything got changed to the new convention except that the calculation of 'nr' just before the check "nr <= offset" .

I have generated this patch which takes care of that and hence fixes the data loss problem as well. I guess it is cleaner too.

This patch is generated w.r.t 2.6.8.1. If everybody blesses this patch I will forward it to Andrew.

Quoted 7 lines · Ram Pai

got it! Everything got changed to the new convention except that the calculation of 'nr' just before the check "nr <= offset" .

I have generated this patch which takes care of that and hence fixes the data loss problem as well. I guess it is cleaner too.

Quoted 2 lines

This patch is generated w.r.t 2.6.8.1. If everybody blesses this patch I will forward it to Andrew.

It looks like it should be OK... but at what point does it become simpler to use my patch which just moves the original calculation up, and does it again if we have to ->readpage()?

(assuming you agree that it solves the problem)

______________________________________________________________________

---

 linux-2.6-npiggin/mm/filemap.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)

diff -puN mm/filemap.c~mm-gmr-fix mm/filemap.c

mm/filemap.c renamed from mm/filemap.c~mm-gmr-fix +18 -11

@@ -724,6 +724,15 @@ void do_generic_mapping_read(struct addr
 		struct page *page;
 		unsigned long nr, ret;
 
+		/* nr is the maximum number of bytes to copy from this page */
+		nr = PAGE_CACHE_SIZE;
+		if (index == end_index) {
+			nr = isize & ~PAGE_CACHE_MASK;
+			if (nr <= offset)
+				goto out;
+		}
+		nr = nr - offset;
+
 		cond_resched();
 		page_cache_readahead(mapping, &ra, filp, index);
 

@@ -736,17 +745,6 @@ find_page:
 		if (!PageUptodate(page))
 			goto page_not_up_to_date;
 page_ok:
-		/* nr is the maximum number of bytes to copy from this page */
-		nr = PAGE_CACHE_SIZE;
-		if (index == end_index) {
-			nr = isize & ~PAGE_CACHE_MASK;
-			if (nr <= offset) {
-				page_cache_release(page);
-				goto out;
-			}
-		}
-		nr = nr - offset;
-
 		/* If users can be writing to this page using arbitrary
 		 * virtual addresses, take care about potential aliasing
 		 * before reading the page on the kernel side.

@@ -826,6 +824,15 @@ readpage:
 			page_cache_release(page);
 			goto out;
 		}
+		nr = PAGE_CACHE_SIZE;
+		if (index == end_index) {
+			nr = isize & ~PAGE_CACHE_MASK;
+			if (nr <= offset) {
+				page_cache_release(page);
+				goto out;
+			}
+		}
+		nr = nr - offset;
 		goto page_ok;
 
 readpage_error:

_

this is the 4th time we are trying to nail down the same thing. We
better get it right this time. So any correct patch is ok with me.

Quoted 15 lines · Nick Piggin

Quoted 8 lines · Ram Pai

got it! Everything got changed to the new convention except that the calculation of 'nr' just before the check "nr <= offset" .

I have generated this patch which takes care of that and hence fixes the data loss problem as well. I guess it is cleaner too.

This patch is generated w.r.t 2.6.8.1. If everybody blesses this patch I will forward it to Andrew.

It looks like it should be OK... but at what point does it become simpler to use my patch which just moves the original calculation up, and does it again if we have to ->readpage()?

Quoted 1 line

(assuming you agree that it solves the problem)

I agree your patch also solves the problem.

Either way is fine. Even Hugh's patch almost does the same thing as yours.

The only advantage with my page is it does the calculation in only one place and does not repeat it. Also I feel its more intuitive to

assume that index 0 covers range 0 to 4095 i.e index n covers range n*PAGE_SIZE to ((n+1)*PAGE_SIZE)-1. Currently the code assumes index 0 covers range 1 to 4096 i.e index n covers range (n*PAGE_SIZE)+1 to (n+1)*PAGE_SIZE.

this is the 4th time we are trying to nail down the same thing. We better get it right this time. So any correct patch is ok with me.

Quoted 15 lines · Ram Pai

Quoted 14 lines · Nick Piggin

Quoted 8 lines · Ram Pai

got it! Everything got changed to the new convention except that the calculation of 'nr' just before the check "nr <= offset" .

I have generated this patch which takes care of that and hence fixes the data loss problem as well. I guess it is cleaner too.

This patch is generated w.r.t 2.6.8.1. If everybody blesses this patch I will forward it to Andrew.

It looks like it should be OK... but at what point does it become simpler to use my patch which just moves the original calculation up, and does it again if we have to ->readpage()?

Quoted 6 lines

Quoted 1 line

(assuming you agree that it solves the problem)

I agree your patch also solves the problem.

Either way is fine. Even Hugh's patch almost does the same thing as yours.

Ahh, yep - Hugh just forgot to also move the "nr" calculation into the ->readpage path, so it hits twice on the fast path.

Quoted 2 lines

The only advantage with my page is it does the calculation in only one place and does not repeat it. Also I feel its more intuitive to

Well kind of - but you are having to jump through hoops to get there. Yours does the following checks:

	/* fast path, read nr_pages from pagecache */
	if (!isize)
		goto out;
	for (i = 0; i < nr_pages; i++) {
		if (index > end_index)
			goto out;
		if (index == end_index) {
			nr = ((isize - 1) & ~PAGE_CACHE_MASK) + 1;
			if (nr <= offset) {
				page_cache_release(page);
				goto out;
			}
		}

		/* slowpath, ->readpage */
		if (unlikely(!isize || index > end_index)) {
			page_cache_release(page);
			goto out;
		}
	}

Mine does:

	if (index > end_index)
		goto out;
	for (i = 0; i < pages_to_read; i++) {
		if (index == end_index) {
			nr = isize & ~PAGE_CACHE_MASK;
			if (nr <= offset)
				goto out;
		}

		/* slowpath, ->readpage */
		if (index > end_index) {
			page_cache_release(page);
			goto out;
		}
		if (index == end_index) {
			nr = isize & ~PAGE_CACHE_MASK;
			if (nr <= offset) {
				page_cache_release(page);
				goto out;
			}
		}
	}

So my fastpath is surely leaner, while the slowpath isn't a clear loser.

What's more, it looks like mine handles the corner case of reading off the end of a non-PAGE_SIZE file (but within the same page). I think yours will drop through and do the ->readpage, while mine doesn't...?

Quoted 4 lines

assume that index 0 covers range 0 to 4095 i.e index n covers range n*PAGE_SIZE to ((n+1)*PAGE_SIZE)-1. Currently the code assumes index 0 covers range 1 to 4096 i.e index n covers range (n*PAGE_SIZE)+1 to (n+1)*PAGE_SIZE.

It is definitely a pretty ugly function all round. I like the 0-4095 thing better too, but my counter argument to that is that this is the minimal change, and similar to how it has previously worked.

Quoted 2 lines

this is the 4th time we are trying to nail down the same thing. We better get it right this time. So any correct patch is ok with me.

I agree. We'll leave it to someone else to decide, then ;)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Well, guys, to make it 100% clear: if I apply the Nick's patch to the
2.6.9-rc1-mm1 tree, it will fix the data loss issue. Is that right?

Quoted 1 line · Rafael J. Wysocki

Well, guys, to make it 100% clear: if I apply the Nick's patch to the 2.6.9-rc1-mm1 tree, it will fix the data loss issue. Is that right?

Should do.

Or revert

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.9-rc1/2.6.9-rc1-mm1/broken-out/re-fix-pagecache-reading-off-by-one-cleanup.patch

and then

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.9-rc1/2.6.9-rc1-mm1/broken-out/re-fix-pagecache-reading-off-by-one.patch

Quoted 5 lines · Andrew Morton

Quoted 2 lines · Rafael J. Wysocki

Well, guys, to make it 100% clear: if I apply the Nick's patch to the 2.6.9-rc1-mm1 tree, it will fix the data loss issue. Is that right?

Should do.

It passes test cases that would previously fail here, so consider it lightly tested. Note that the patch is on top of 2.6.9-rc1 though, it becomes slightly deranged when applying straight onto mm. So don't do that.

..

Quoted 5 lines

Or revert

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.9-rc1/2 .6.9-rc1-mm1/broken-out/re-fix-pagecache-reading-off-by-one-cleanup.patch

Quoted 4 lines

and then

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.9-rc1/2 .6.9-rc1-mm1/broken-out/re-fix-pagecache-reading-off-by-one.patch

Once you have these backed out mine should apply fine, but it only closes some performance (not correctness) corner cases that the above patches attempted to.

Ahh, yep - Hugh just forgot to also move the "nr" calculation into the ->readpage path, so it hits twice on the fast path.

What's more, it looks like mine handles the corner case of reading off the end of a non-PAGE_SIZE file (but within the same page). I think yours will drop through and do the ->readpage, while mine doesn't...?

I agree. We'll leave it to someone else to decide, then ;)

Quoted 2 lines · Nick Piggin

Ahh, yep - Hugh just forgot to also move the "nr" calculation into the ->readpage path, so it hits twice on the fast path.

Yes, your patch is better than mine.

Quoted 3 lines

What's more, it looks like mine handles the corner case of reading off the end of a non-PAGE_SIZE file (but within the same page). I think yours will drop through and do the ->readpage, while mine doesn't...?

It's a common case, not a corner case: a short read to end of file, then app does another read which returns 0 for the end of file: that wouldn't normally fall through to readpage in Ram's case, but would do unnecessary page_cache_readahead (won't do much) and find_get_page.

Quoted 2 lines

I agree. We'll leave it to someone else to decide, then ;)

I vote for Nick's patch.

I do have one reservation on do_generic_mapping_read, common to all these versions, unrelated to the current issue.

I'm surprised to notice that you're careful to re-i_size_read after readpage, but otherwise rely on the initial i_size_read. We could go around this loop many many times, faulting user pages in actor, rescheduling away: the old (e.g. 2.4 or 2.6.0) code was deficient after readpage, but at least it reassessed i_size each time around the loop. I guess if the file is truncated meanwhile, the common case would be for a find_get_page to fail, and then the situation be corrected after readpage; perhaps it's more likely to show up as read returning too little on a large file being steadily appended. Maybe you already ruled these cases out as not worth the overhead of handling, but it does surprise me that the old code was safer in this respect.

Quoted 1 line · Hugh Dickins

On Sat, 28 Aug 2004, Nick Piggin wrote:

But anyway, I think we agree my (our) version should cover that.

Quoted 1 line

Quoted 5 lines

Quoted 1 line

I agree. We'll leave it to someone else to decide, then ;)

I vote for Nick's patch.

OK - maybe that can go for a spin in the next -mm. Andrew did you get it?

Quoted 4 lines

I do have one reservation on do_generic_mapping_read, common to all these versions, unrelated to the current issue.

Quoted 13 lines

I'm surprised to notice that you're careful to re-i_size_read after readpage, but otherwise rely on the initial i_size_read. We could go around this loop many many times, faulting user pages in actor, rescheduling away: the old (e.g. 2.4 or 2.6.0) code was deficient after readpage, but at least it reassessed i_size each time around the loop. I guess if the file is truncated meanwhile, the common case would be for a find_get_page to fail, and then the situation be corrected after readpage; perhaps it's more likely to show up as read returning too little on a large file being steadily appended. Maybe you already ruled these cases out as not worth the overhead of handling, but it does surprise me that the old code was safer in this respect.

Yeah I guess it is a case of doing the minimum that is really needed.

Although considering page_cache_readahead call can do an i_size_read, it might be nicer to probably change the interface to have it passed down instead

Quoted 3 lines

OK - maybe that can go for a spin in the next -mm. Andrew did you get it?

So in case my vote counts, add my vote too :) .

But the biggest performance boost has been seen with large max-readahead window sizes. Currently most of the underlying block devices default to 32 pages max-readahead even though the underlying device can handle much larger reads. We could extract much more sequential read performance if the max-readahead was set to much higher values like 256 pages which most modern devices are capable off. The problem AFAICT is that the block device layer defaults the max-readahead value for most block devices to 32, without consulting the capability of the underlying block device driver.