Review - writing to multiple non-contiguous unwritten extents within a page is broken.

[Nathan - probably another one for you]

This test run on ia64 (16k page size) on a 4k block size filesystem:

#!/bin/bash

file=$1
rm -f $file

xfs_io -f \

(quoted diff) +0 -8

-c "truncate 1048576" \
-c "resvsp 1032192 16384" \
-c "pwrite 1033216 2560" \
-c "pwrite 1040384 8192" \
-c "bmap -vvp" \
-c "fsync" \
-c "bmap -vvp" \
-c "close" \

$file

Which writes 3 unwritten blocks in a page (first block and last 2) results in a corrupted write.

The problem is that the second block on teh page is uninitialised and so is skipped by xfs_page_state_convert. The problem is that the xfs_ioend structures are not getting created correctly.

When we skip the uninitialised block, we add the second unwritten block we are writing to into the original ioend. While this results in the correct I/O being sent to disk, it results in a ioend with a start offset of 0 and a length of 3 blocks. When we do unwritten extent conversion based on this range, we convert the wrong blocks.

What we need to be doing is creating two xfs_ioend structures, one for the first block and one for the second set of blocks in the page. That way we get two separate I/O completion events and convert the ranges separately and correctly.

I've checked xfs_convert_page(), and I don't think it needs any fix here - it already appears to force multiple ioends to be used in this case...

Thoughts?

Cheers,

Dave.

Signature · 10 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

---
 fs/xfs/linux-2.6/xfs_aops.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c
===================================================================

fs/xfs/linux-2.6/xfs_aops.c +12 -1

@@ -1008,6 +1008,8 @@ xfs_page_state_convert(
 		if (buffer_unwritten(bh) || buffer_delay(bh) ||
 		    ((buffer_uptodate(bh) || PageUptodate(page)) &&
 		     !buffer_mapped(bh) && (unmapped || startio))) {
+			int new_ioend = 0;
+
 			/*
 			 * Make sure we don't use a read-only iomap
 			 */

@@ -1026,6 +1028,15 @@ xfs_page_state_convert(
 			}
 
 			if (!iomap_valid) {
+				/*
+				 * if we didn't have a valid mapping then we
+				 * need to ensure that we put the new mapping
+				 * in a new ioend structure. This needs to be
+				 * done to ensure that the ioends correctly
+				 * reflect the block mappings at io completion
+				 * for unwritten extent conversion.
+				 */
+				new_ioend = 1;
 				if (type == IOMAP_NEW) {
 					size = xfs_probe_cluster(inode,
 							page, bh, head, 0);

@@ -1045,7 +1056,7 @@ xfs_page_state_convert(
 				if (startio) {
 					xfs_add_to_ioend(inode, bh, offset,
 							type, &ioend,
-							!iomap_valid);
+							new_ioend);
 				} else {
 					set_buffer_dirty(bh);
 					unlock_buffer(bh);

Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c
===================================================================

fs/xfs/linux-2.6/xfs_aops.c +11 -0

@@ -1008,6 +1008,8 @@ xfs_page_state_convert(
 		if (buffer_unwritten(bh) || buffer_delay(bh) ||
 		    ((buffer_uptodate(bh) || PageUptodate(page)) &&
 		     !buffer_mapped(bh) && (unmapped || startio))) {
+			int new_ioend = 0;
+
 			/*
 			 * Make sure we don't use a read-only iomap
 			 */

@@ -1026,6 +1028,15 @@ xfs_page_state_convert(
 			}
 
 			if (!iomap_valid) {
+				/*
+				 * if we didn't have a valid mapping then we
+				 * need to ensure that we put the new mapping
+				 * in a new ioend structure. This needs to be
+				 * done to ensure that the ioends correctly
+				 * reflect the block mappings at io completion
+				 * for unwritten extent conversion.
+				 */
+				new_ioend = 1;
 				if (type == IOMAP_NEW) {
 					size = xfs_probe_cluster(inode,
 							page, bh, head, 0);