xfs: using extsize cause corruption with multi buffer page.

This mod make sure that buffer in a xfs_ioend are all in the
same extent. This is actually similar to what is done in xfs_convert_page() already.

This solve the problem of having multiple extent in one page.

With the current kernel if we have a page that look like this:

buffer  content
0       empty  b_state = 0
1       DATA   b_state = 0x1023
2       DATA   b_state = 0x1023
3       empty  b_state = 0
4       empty  b_state = 0
5       DATA   b_state = 0x1023
6       DATA   b_state = 0x1023
7       empty  b_state = 0

We endup with buffer 1-4 been tag as real and 5-EOF tag as unwritten. Instead of 1-2 real, 3-4 unwritten, 5-6 real, 7-EOF unwritten.

Signature · 6 lines

1.7.4.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 5 lines · Alain Renaud

This mod make sure that buffer in a xfs_ioend are all in the
same extent. This is actually similar to what is done in xfs_convert_page() already.

This solve the problem of having multiple extent in one page.

You need to describe how the change fixes the problem, not state that it does.

i.e. a fix is no good if you can't explain *why* it fixes the bug.

Quoted 12 lines

With the current kernel if we have a page that look like this:

buffer  content
0       empty  b_state = 0
1       DATA   b_state = 0x1023
2       DATA   b_state = 0x1023
3       empty  b_state = 0
4       empty  b_state = 0
5       DATA   b_state = 0x1023
6       DATA   b_state = 0x1023
7       empty  b_state = 0

Quoted 2 lines

We endup with buffer 1-4 been tag as real and 5-EOF tag as unwritten. Instead of 1-2 real, 3-4 unwritten, 5-6 real, 7-EOF unwritten.

Actaully, the buffer state is correct - it's the ioend construction that appears wrong and that leads to conversion being incorrect. Indeed, if you look at my previous email about the test case you'll see that the in-memory state is correct before the unmount....

As it is, the patch changes the way the new_ioend variable works - it's supposed to be set only if we have to map a new extent. That is, if we currently don't have a valid extent we need a new ioend when we map the new extent. The correct way to create a new extent is to mark the current imap as invalid so we have to do a new lookup....

fs/xfs/xfs_aops.c xfs_vm_writepage( · line 991

Quoted 7 lines

fs/xfs/xfs_aops.c +1 -0

@@ -985,6 +986,7 @@ xfs_vm_writepage(
  				ASSERT(buffer_mapped(bh));
  				imap_valid = 0;
  			}
+			new_ioend = 1;
  			continue;
  		}

This is the real change of logic, and indicates where the bug potentially lies. The same fix can be accomplished simply by replacing all your changes with this:

(quoted diff) +2 -3

@@ -981,10 +981,9 @@ xfs_vm_writepage(
                                 imap_valid = 0;
                         }
                 } else {
-                       if (PageUptodate(page)) {
+                       if (PageUptodate(page))
                                 ASSERT(buffer_mapped(bh));
-                               imap_valid = 0;
-                       }
+                       imap_valid = 0;
                         continue;
                 }
 

But this still doesn't explain what the problem actually is. All it indicates is that we have a buffer that is mapped but not up to date on a page that is also not up to date. What we need to do is understand why this is happening, and if changing this logic is the correct fix that won't have any unintended side effects.

So let's try to understand this a bit better. First of all, are extent size hints necessary to trigger this? With this change:

- xfs_io -f -c "extsize expr $pgsize \* 10" \
+ xfs_io -f -c "resvsp 0 expr $pgsize \* 10" \

I find that the same corruption occurs, so this is an unwritten extent problem, not an extent size hint issue. So now we have a much wider scope than initially indicated.

So, next question: what part does the truncate play, if any? So I comment out the truncate, and it appears that it has no effect on the result of the test. OK, lets drop that completely.

Ok, that leaves us with a test case that is kind of similar to part 4 of test 194, but with the initial condition of a prealocated region rather than an allocated block. And test 194 doesn't check the state after a unmount/mount cycle so even if it did have a preallocated initial state, it wouldn't detect this problem. IOWs, we have a whole new class of corner cases that we don't currently exercise. Alain, your test is going need some more work...

Alright, so lets look at why we fail to handle this case in writeback properly.

   xfs_io-2498  xfs_writepage:        dev 253:16 ino 0x23 pgoff 0x1000 size 0x1e00 offset 0 delalloc 0 unwritten 1
   xfs_io-2498  xfs_map_blocks_found: dev 253:16 ino 0x23 size 0x0 offset 0x1200 count 512 type  startoff 0x0 startblock 48 blockcount 0x50

kworker/0:15 xfs_unwritten_convert: dev 253:16 ino 0x23 isize 0x1e00 disize 0x0 offset 0x1200 count 2048

Ah, there's why it fails to handle the case correctly - the unwritten extent is a single map and hence if we won't do another lookup because it spans the entire page already. Hence if imap_valid is not cleared, we'll never look it up again and set new_ioend.

If the page is marked uptodate, then that means all the buffers have been correctly initialised and should all be marked up to date. Hence I'm not sure that the existing logic there is actually possible to hit. We know the buffer is not uptodate, and we use generic_write_end, which calls __block_commit_write(), and that only calls SetPageUptodate() when all the buffers are uptodate.

Indeed, if the buffer is not uptodate, then it means we didn't complete the write on it and it does not have valid data in it, so regardless of the page state we should not write it. That means we must always skip it and that means we always need a new ioend in this case. hence we should always clear imap_valid in this case. Hence i think the above hunk is the right fix for the problem.

Alain, can you check my logic, run it through your tests and if it works, respin the patch with this info in the commit message and a comment in the code indicating why we need to clear imap_valid unconditionally there?

Cheers,

Dave.

That means, I think, that the logic in the current code is just plain wrong. xfs_convert_page() aborts on pages and buffers that are both not uptodate, so they get handled by xfs_vm_writepage(). This particular branch we are looking at there is the the !buffer_uptodate() branch.

What your change does is this:

Quoted 8 lines

Signature · 6 lines

1.7.4.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

On filesytems with a block size smaller than PAGE_SIZE we currently have
a problem with unwritten extents. If a we have multi-block page for which an unwritten extent has been allocated, and only some of the buffers have been written to, and they are not contiguous, we can expose stale data from disk in the blocks between the writes after extent conversion.

Example of a page with unwritten and real data.

buffer  content
0       empty  b_state = 0
1       DATA   b_state = 0x1023 Uptodate,Dirty,Mapped,Unwritten
2       DATA   b_state = 0x1023 Uptodate,Dirty,Mapped,Unwritten
3       empty  b_state = 0
4       empty  b_state = 0
5       DATA   b_state = 0x1023 Uptodate,Dirty,Mapped,Unwritten
6       DATA   b_state = 0x1023 Uptodate,Dirty,Mapped,Unwritten
7       empty  b_state = 0

Buffers 1, 2, 5, and 6 have been written to, leaving 0, 3, 4, and 7 empty. Currently buffers 1, 2, 5, and 6 are added to a single ioend, and when IO has completed, extent conversion creates a real extent from block 1 through block 6, leaving 0 and 7 unwritten. However buffers 3 and 4 were not written to disk, so stale data is exposed from those blocks on a subsequent read.

Fix this by setting iomap_valid = 0 when we find a buffer that is not Uptodate. This ensures that buffers 5 and 6 are not added to the same ioend as buffers 1 and 2. Later these blocks will be converted into two separate real extents, leaving the blocks in between unwritten.

Signed-off-by Alain Renaud <[email protected]>

On filesytems with a block size smaller than PAGE_SIZE we currently have
a problem with unwritten extents. If a we have multi-block page for which an unwritten extent has been allocated, and only some of the buffers have been written to, and they are not contiguous, we can expose stale data from disk in the blocks between the writes after extent conversion.

Example of a page with unwritten and real data.

buffer  content
0       empty  b_state = 0
1       DATA   b_state = 0x1023 Uptodate,Dirty,Mapped,Unwritten
2       DATA   b_state = 0x1023 Uptodate,Dirty,Mapped,Unwritten
3       empty  b_state = 0
4       empty  b_state = 0
5       DATA   b_state = 0x1023 Uptodate,Dirty,Mapped,Unwritten
6       DATA   b_state = 0x1023 Uptodate,Dirty,Mapped,Unwritten
7       empty  b_state = 0

Buffers 1, 2, 5, and 6 have been written to, leaving 0, 3, 4, and 7 empty. Currently buffers 1, 2, 5, and 6 are added to a single ioend, and when IO has completed, extent conversion creates a real extent from block 1 through block 6, leaving 0 and 7 unwritten. However buffers 3 and 4 were not written to disk, so stale data is exposed from those blocks on a subsequent read.

Fix this by setting iomap_valid = 0 when we find a buffer that is not Uptodate. This ensures that buffers 5 and 6 are not added to the same ioend as buffers 1 and 2. Later these blocks will be converted into two separate real extents, leaving the blocks in between unwritten.

Signed-off-by Alain Renaud <[email protected]>