Patchwork [v3] xfstests:Make 225 compare map and fiemap at each block.

login
register
mail settings
Submitter Yongqiang Yang
Date May 23, 2011, 7:07 a.m.
Message ID <1306134423-10028-1-git-send-email-xiaoqiangnk@gmail.com>
Download mbox | patch
Permalink /patch/96831/
State Not Applicable
Headers show

Comments

Yongqiang Yang - May 23, 2011, 7:07 a.m.
Due to my carelessness,  I induced a ugly patch to ext4's fiemap, as a result
delayed-extents that did not start at the head block of a page was ignored
in ext4 with 1K block, but 225 could not find it.  So I looked into the 225
and could not figure out logic in compare_map_and_fiemap(), which seemed to
mix extents with blocks.  Then I made 225 compare map and fiemap at each block,
the new 225 can find the bug I induced and another bug in ext4 with 1k block,
which ignored delayed-extents after a hole, which did not start at the head
block of a page.


The new 225 works well on ext3, ext4 and xfs with both 1K and 4K block.

Reviewed-by: josef@rehat.com
Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
v2->v3:
  Add much more specific changelog expalining the bug the old 225 could not find.

v1->v2:
Josef <josef@redhat.com> reviewed the v1 patch and pointed out the bug which
made xfs not work and several coding styles.

According to reply from Josef, I
 fixed the bug which added ';' after an if statement.
 removed the trailing whitespace.

Apart from things above, I
 made check_weird_fs_hole() verify bytes read by pread().

 src/fiemap-tester.c |  251 ++++++++++++++++++++++++++++-----------------------
 1 files changed, 140 insertions(+), 111 deletions(-)
Dave Chinner - May 24, 2011, 1:51 a.m.
On Mon, May 23, 2011 at 03:07:03PM +0800, Yongqiang Yang wrote:
> Due to my carelessness,  I induced a ugly patch to ext4's fiemap, as a result
> delayed-extents that did not start at the head block of a page was ignored
> in ext4 with 1K block, but 225 could not find it. 

When ext4 is using 1k block sizes, fiemap-tester does not find
problems that may exist on ext4 with delayed allocation extents on
the first block of a page.

> So I looked into the 225
> and could not figure out logic in compare_map_and_fiemap(), which seemed to
> mix extents with blocks.

Once again, "I don't understand it" is not a reason for a whoelsale
rewrite.

> Then I made 225 compare map and fiemap at each block,
> the new 225 can find the bug I induced and another bug in ext4 with 1k block,
> which ignored delayed-extents after a hole, which did not start at the head
> block of a page.

Which means that the first paragraph should read:

"When ext4 is using 1k block sizes, fiemap-tester does not find
problems that may exist on ext4 with delayed allocation extents on
the first block of a page or directly after a hole."

That's a concise description of the overall problem you are fixing
in this patch. Next you need to describe the different changes being
made in the patch and the bugs they are fixing.  There appear to be:

	- an off-by one in map array allocation
	- zeroing the end block in the map array
	- making check_weird_fs_hole() verify bytes read by pread()
	- moving truncate/seek of the test file around
	- changing the way map/fiemap are compared

Also, you haven't addressed any of the comments I made in my
original review:

	- removing the changelog from the header comment
	- adding comments on check_data/check_hole explaining what
	  they are checking
	- explaining why the existing map/fiemap compare couldn't
	  detect the problems with delayed extents on ext4? i.e.
	  what's the bug that you are fixing so we can determine if
	  it really does need a rewrite to fix?

Cheers,

Dave.
Yongqiang Yang - May 24, 2011, 2:42 a.m.
Thank you for your review.
On Tue, May 24, 2011 at 9:51 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Mon, May 23, 2011 at 03:07:03PM +0800, Yongqiang Yang wrote:
>> Due to my carelessness,  I induced a ugly patch to ext4's fiemap, as a result
>> delayed-extents that did not start at the head block of a page was ignored
>> in ext4 with 1K block, but 225 could not find it.
>
> When ext4 is using 1k block sizes, fiemap-tester does not find
> problems that may exist on ext4 with delayed allocation extents on
> the first block of a page.
>
>> So I looked into the 225
>> and could not figure out logic in compare_map_and_fiemap(), which seemed to
>> mix extents with blocks.
>
> Once again, "I don't understand it" is not a reason for a whoelsale
> rewrite.
>
>> Then I made 225 compare map and fiemap at each block,
>> the new 225 can find the bug I induced and another bug in ext4 with 1k block,
>> which ignored delayed-extents after a hole, which did not start at the head
>> block of a page.
>
> Which means that the first paragraph should read:
>
> "When ext4 is using 1k block sizes, fiemap-tester does not find
> problems that may exist on ext4 with delayed allocation extents on
> the first block of a page or directly after a hole."
>
> That's a concise description of the overall problem you are fixing
> in this patch. Next you need to describe the different changes being
> made in the patch and the bugs they are fixing.  There appear to be:
>
>        - an off-by one in map array allocation
>        - zeroing the end block in the map array
>        - making check_weird_fs_hole() verify bytes read by pread()
>        - moving truncate/seek of the test file around
>        - changing the way map/fiemap are compared
Yes, thank you.
>
> Also, you haven't addressed any of the comments I made in my
> original review:
>
>        - removing the changelog from the header comment
The change is large, so I think  it's convenient for others to leave
an e-mail in the file in case that they have some questions.   e.g.  I
should cc to josef this time.  If you don't think it is necessary, I
will remove it.

>        - adding comments on check_data/check_hole explaining what
>          they are checking
Sorry, I will adding comments in later version.
>        - explaining why the existing map/fiemap compare couldn't
>          detect the problems with delayed extents on ext4? i.e.
>          what's the bug that you are fixing so we can determine if
>          it really does need a rewrite to fix?
I will try to figure it out.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
Dave Chinner - May 24, 2011, 5:18 a.m.
On Tue, May 24, 2011 at 10:42:23AM +0800, Yongqiang Yang wrote:
> Thank you for your review.
> On Tue, May 24, 2011 at 9:51 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Mon, May 23, 2011 at 03:07:03PM +0800, Yongqiang Yang wrote:
> >> Due to my carelessness,  I induced a ugly patch to ext4's fiemap, as a result
> >> delayed-extents that did not start at the head block of a page was ignored
> >> in ext4 with 1K block, but 225 could not find it.
> >
> > When ext4 is using 1k block sizes, fiemap-tester does not find
> > problems that may exist on ext4 with delayed allocation extents on
> > the first block of a page.
> >
> >> So I looked into the 225
> >> and could not figure out logic in compare_map_and_fiemap(), which seemed to
> >> mix extents with blocks.
> >
> > Once again, "I don't understand it" is not a reason for a whoelsale
> > rewrite.
> >
> >> Then I made 225 compare map and fiemap at each block,
> >> the new 225 can find the bug I induced and another bug in ext4 with 1k block,
> >> which ignored delayed-extents after a hole, which did not start at the head
> >> block of a page.
> >
> > Which means that the first paragraph should read:
> >
> > "When ext4 is using 1k block sizes, fiemap-tester does not find
> > problems that may exist on ext4 with delayed allocation extents on
> > the first block of a page or directly after a hole."
> >
> > That's a concise description of the overall problem you are fixing
> > in this patch. Next you need to describe the different changes being
> > made in the patch and the bugs they are fixing.  There appear to be:
> >
> >        - an off-by one in map array allocation
> >        - zeroing the end block in the map array
> >        - making check_weird_fs_hole() verify bytes read by pread()
> >        - moving truncate/seek of the test file around
> >        - changing the way map/fiemap are compared
> Yes, thank you.

I'm not asking you to ACK the list of changes in the bug - I'm
asking you to describe the symptoms of the problems those fixes
apparently address - how you've found them , what the test failures
looked like, etc, so there's some meaningful record of why those
changes were made.

> > Also, you haven't addressed any of the comments I made in my
> > original review:
> >
> >        - removing the changelog from the header comment
> The change is large, so I think  it's convenient for others to leave
> an e-mail in the file in case that they have some questions.

That's what git log/git blame is for. Details of who made what
change has no place in the code itself - recording that information
ina queriable format is precisely the reason we use a VCS.

Cheers,

Dave.
Yongqiang Yang - May 24, 2011, 7:13 a.m.
On Tue, May 24, 2011 at 9:51 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Mon, May 23, 2011 at 03:07:03PM +0800, Yongqiang Yang wrote:
>> Due to my carelessness,  I induced a ugly patch to ext4's fiemap, as a result
>> delayed-extents that did not start at the head block of a page was ignored
>> in ext4 with 1K block, but 225 could not find it.
>
> When ext4 is using 1k block sizes, fiemap-tester does not find
> problems that may exist on ext4 with delayed allocation extents on
> the first block of a page.
>
>> So I looked into the 225
>> and could not figure out logic in compare_map_and_fiemap(), which seemed to
>> mix extents with blocks.
>
> Once again, "I don't understand it" is not a reason for a whoelsale
> rewrite.
>
>> Then I made 225 compare map and fiemap at each block,
>> the new 225 can find the bug I induced and another bug in ext4 with 1k block,
>> which ignored delayed-extents after a hole, which did not start at the head
>> block of a page.
>
> Which means that the first paragraph should read:
>
> "When ext4 is using 1k block sizes, fiemap-tester does not find
> problems that may exist on ext4 with delayed allocation extents on
> the first block of a page or directly after a hole."
>
> That's a concise description of the overall problem you are fixing
> in this patch. Next you need to describe the different changes being
> made in the patch and the bugs they are fixing.  There appear to be:
>
>        - an off-by one in map array allocation
>        - zeroing the end block in the map array
>        - making check_weird_fs_hole() verify bytes read by pread()
>        - moving truncate/seek of the test file around
>        - changing the way map/fiemap are compared
>
> Also, you haven't addressed any of the comments I made in my
> original review:
>
>        - removing the changelog from the header comment
>        - adding comments on check_data/check_hole explaining what
>          they are checking
>        - explaining why the existing map/fiemap compare couldn't
>          detect the problems with delayed extents on ext4? i.e.
>          what's the bug that you are fixing so we can determine if
>          it really does need a rewrite to fix?
Sorry for being dense, I still can not understand logic in 225.  I
will take an example to explain the bug which 225 does not report.  I
think fiemap and map info should be compared at each block, I tried to
find this logic in 225, but I could not.

Assume that a file is 'HDDHD', and set blocks_to_map in
compare_fiemap_and map() to 5.
Due to a bug I induced in ext4, only the 1st extent is returned.
for-loop will check block 0 and break with i = 2. then cur_extent will
be 2.  Next, do-loop will lookup from 2 and no extent is returned,
then for-loop break with i = 3. then cur_extent will be 3.  Then
do-loop will lookup from 3 and no extent is returned, then for-loop
break with i=4. then do-loop will lookup from 4 and the 2nd delayed
extent is returned, and block 4 is verified.  225 does not find the
bug.

Actually, 225 should report the bug at first, because blocks_to_map is set to 5.

I am not sure this can be understood.

Thanks,
Yongqiang.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>

Patch

diff --git a/src/fiemap-tester.c b/src/fiemap-tester.c
index 1663f84..319a9bb 100644
--- a/src/fiemap-tester.c
+++ b/src/fiemap-tester.c
@@ -14,6 +14,9 @@ 
  * You should have received a copy of the GNU General Public License
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ * Compare map and fiemap at each block,
+ * Yongqiang Yang <xiaoqiangnk@gmail.com>, 2011
  */
 
 #include <stdio.h>
@@ -57,7 +60,7 @@  generate_file_mapping(int blocks, int prealloc)
 	int num_types = 2, cur_block = 0;
 	int i = 0;
 
-	map = malloc(sizeof(char) * blocks);
+	map = malloc(sizeof(char) * (blocks + 1));
 	if (!map)
 		return NULL;
 
@@ -81,6 +84,7 @@  generate_file_mapping(int blocks, int prealloc)
 		cur_block++;
 	}
 
+	map[blocks] = 0;
 	return map;
 }
 
@@ -247,55 +251,36 @@  check_flags(struct fiemap *fiemap, int blocksize)
 }
 
 static int
-check_data(struct fiemap *fiemap, __u64 logical_offset, int blocksize,
+check_data(struct fiemap_extent *extent ,  __u64 logical_offset, int blocksize,
 	   int last, int prealloc)
 {
-	struct fiemap_extent *extent;
-	__u64 orig_offset = logical_offset;
-	int c, found = 0;
-
-	for (c = 0; c < fiemap->fm_mapped_extents; c++) {
-		__u64 start, end;
-		extent = &fiemap->fm_extents[c];
-
-		start = extent->fe_logical;
-		end = extent->fe_logical + extent->fe_length;
-
-		if (logical_offset > end)
-			continue;
-
-		if (logical_offset + blocksize < start)
-			break;
-
-		if (logical_offset >= start &&
-		    logical_offset < end) {
-			if (prealloc &&
-			    !(extent->fe_flags & FIEMAP_EXTENT_UNWRITTEN)) {
-				printf("ERROR: preallocated extent is not "
-				       "marked with FIEMAP_EXTENT_UNWRITTEN: "
-				       "%llu\n",
-				       (unsigned long long)
-				       (start / blocksize));
-				return -1;
-			}
-
-			if (logical_offset + blocksize > end) {
-				logical_offset = end+1;
-				continue;
-			} else {
-				found = 1;
-				break;
-			}
+	int found = 0;
+	__u64 start, end;
+
+	start = extent->fe_logical;
+	end = extent->fe_logical + extent->fe_length;
+
+	if (logical_offset >= start &&
+	    logical_offset < end) {
+		if (prealloc &&
+		    !(extent->fe_flags & FIEMAP_EXTENT_UNWRITTEN)) {
+			printf("ERROR: preallocated extent is not "
+			       "marked with FIEMAP_EXTENT_UNWRITTEN: "
+			       "%llu\n",
+			       (unsigned long long)
+			       (start / blocksize));
+			return -1;
 		}
+		found = 1;
 	}
 
 	if (!found) {
 		printf("ERROR: couldn't find extent at %llu\n",
-		       (unsigned long long)(orig_offset / blocksize));
+		       (unsigned long long)(logical_offset / blocksize));
 	} else if (last &&
-		   !(fiemap->fm_extents[c].fe_flags & FIEMAP_EXTENT_LAST)) {
+		   !(extent->fe_flags & FIEMAP_EXTENT_LAST)) {
 		printf("ERROR: last extent not marked as last: %llu\n",
-		       (unsigned long long)(orig_offset / blocksize));
+		       (unsigned long long)(logical_offset / blocksize));
 		found = 0;
 	}
 
@@ -306,7 +291,7 @@  static int
 check_weird_fs_hole(int fd, __u64 logical_offset, int blocksize)
 {
 	static int warning_printed = 0;
-	int block, i;
+	int block, i, count;
 	size_t buf_len = sizeof(char) * blocksize;
 	char *buf;
 
@@ -330,15 +315,16 @@  check_weird_fs_hole(int fd, __u64 logical_offset, int blocksize)
 		return -1;
 	}
 
-	if (pread(fd, buf, buf_len, (off_t)logical_offset) < 0) {
+	count = pread(fd, buf, buf_len, (off_t)logical_offset);
+	if (count < 0) {
 		perror("Error reading from file");
 		free(buf);
 		return -1;
 	}
 
-	for (i = 0; i < buf_len; i++) {
+	for (i = 0; i < count; i++) {
 		if (buf[i] != 0) {
-			printf("ERROR: FIEMAP claimed there was data (%c) at "
+			printf("ERROR: FIEMAP claimed there was data (%x) at "
 			       "block %llu that should have been a hole, and "
 			       "FIBMAP confirmed that it was allocated, but "
 			       "it should be filled with 0's, but it was not "
@@ -370,35 +356,25 @@  check_weird_fs_hole(int fd, __u64 logical_offset, int blocksize)
 }
 
 static int
-check_hole(struct fiemap *fiemap, int fd, __u64 logical_offset, int blocksize)
+check_hole(struct fiemap_extent *extent, int fd, __u64 logical_offset,
+	   int blocksize)
 {
-	struct fiemap_extent *extent;
-	int c;
+	__u64 start, end;
 
-	for (c = 0; c < fiemap->fm_mapped_extents; c++) {
-		__u64 start, end;
-		extent = &fiemap->fm_extents[c];
-
-		start = extent->fe_logical;
-		end = extent->fe_logical + extent->fe_length;
-
-		if (logical_offset > end)
-			continue;
-		if (logical_offset + blocksize < start)
-			break;
+	start = extent->fe_logical;
+	end = extent->fe_logical + extent->fe_length;
 
-		if (logical_offset >= start &&
-		    logical_offset < end) {
+	if (logical_offset >= start &&
+	    logical_offset < end) {
 
-			if (check_weird_fs_hole(fd, logical_offset,
-						blocksize) == 0)
-				break;
+		if (check_weird_fs_hole(fd, logical_offset,
+					blocksize) == 0)
+			return 0;
 
-			printf("ERROR: found an allocated extent where a hole "
-			       "should be: %llu\n",
-			       (unsigned long long)(start / blocksize));
-			return -1;
-		}
+		printf("ERROR: found an allocated extent where a hole "
+		       "should be: %llu\n",
+		       (unsigned long long)(logical_offset / blocksize));
+		return -1;
 	}
 
 	return 0;
@@ -423,9 +399,11 @@  compare_fiemap_and_map(int fd, char *map, int blocks, int blocksize, int syncfil
 {
 	struct fiemap *fiemap;
 	char *fiebuf;
-	int blocks_to_map, ret, cur_extent = 0, last_data;
+	int blocks_to_map, ret, last_data = -1;
 	__u64 map_start, map_length;
 	int i, c;
+	int cur_block = 0;
+	int last_found = 0;
 
 	if (query_fiemap_count(fd, blocks, blocksize) < 0)
 		return -1;
@@ -451,8 +429,11 @@  compare_fiemap_and_map(int fd, char *map, int blocks, int blocksize, int syncfil
 	fiemap->fm_extent_count = blocks_to_map;
 	fiemap->fm_mapped_extents = 0;
 
+	/* check fiemap by looking at each block. */
 	do {
-		fiemap->fm_start = map_start;
+		int nr_extents;
+
+		fiemap->fm_start = cur_block * blocksize;
 		fiemap->fm_length = map_length;
 
 		ret = ioctl(fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
@@ -465,45 +446,93 @@  compare_fiemap_and_map(int fd, char *map, int blocks, int blocksize, int syncfil
 		if (check_flags(fiemap, blocksize))
 			goto error;
 
-		for (i = cur_extent, c = 1; i < blocks; i++, c++) {
-			__u64 logical_offset = i * blocksize;
-
-			if (c > fiemap->fm_mapped_extents) {
-				i++;
-				break;
+		nr_extents = fiemap->fm_mapped_extents;
+		if (nr_extents == 0) {
+			int block = cur_block + (map_length - 1) / blocksize;
+			for (; cur_block <= block &&
+			       cur_block < blocks; cur_block++) {
+				/* check hole */
+				if (map[cur_block] != 'H') {
+					printf("ERROR: map[%d] should not be "
+					       "a hole\n", cur_block);
+					goto error;
+				}
 			}
+			continue;
+		}
 
-			switch (map[i]) {
-			case 'D':
-				if (check_data(fiemap, logical_offset,
-					       blocksize, last_data == i, 0))
+		for (c = 0; c < nr_extents; c++) {
+			__u64 offset;
+			int block;
+			struct fiemap_extent *extent;
+
+
+			extent = &fiemap->fm_extents[c];
+			offset = extent->fe_logical;
+			block = offset / blocksize;
+
+			/* check hole. */
+			for (; cur_block < block && cur_block < blocks;
+			     cur_block++) {
+				if (map[cur_block] != 'H') {
+					printf("ERROR: map[%d] should not be "
+					       "a hole\n", cur_block);
 					goto error;
-				break;
-			case 'H':
-				if (check_hole(fiemap, fd, logical_offset,
-					       blocksize))
+				}
+			}
+
+			offset = extent->fe_logical + extent->fe_length;
+			block = offset / blocksize;
+			/* check data */
+			for (; cur_block < block && cur_block < blocks;
+			     cur_block++) {
+				int last;
+				offset = (__u64)cur_block * blocksize;
+				last = (cur_block == last_data);
+				last_found = last_found ? last_found : last;
+				switch (map[cur_block]) {
+				case 'D':
+					if (check_data(extent, offset,
+						       blocksize, last, 0))
+						goto error;
+					break;
+				case 'H':
+					if (check_hole(extent, fd, offset,
+						       blocksize))
+						goto error;
+					break;
+
+				case 'P':
+					if (check_data(extent, offset,
+						       blocksize, last, 1))
+						goto error;
+					break;
+				default:
+					printf("ERROR: weird value in "
+					       "map: %c\n", map[i]);
 					goto error;
-				break;
-			case 'P':
-				if (check_data(fiemap, logical_offset,
-					       blocksize, last_data == i, 1))
+				}
+			}
+
+			for (; cur_block < block; cur_block++) {
+				offset = (__u64)cur_block * blocksize;
+				if (check_hole(extent, fd, offset, blocksize))
 					goto error;
-				break;
-			default:
-				printf("ERROR: weird value in map: %c\n",
-				       map[i]);
-				goto error;
 			}
 		}
-		cur_extent = i;
-		map_start = i * blocksize;
-	} while (cur_extent < blocks);
+	} while (cur_block < blocks);
 
-	ret = 0;
-	return ret;
+	if (!last_found && last_data != -1) {
+		printf("ERROR: find no last extent\n");
+		goto error;
+	}
+
+	free(fiebuf);
+	return 0;
 error:
 	printf("map is '%s'\n", map);
 	show_extents(fiemap, blocksize);
+	free(fiebuf);
 	return -1;
 }
 
@@ -605,6 +634,18 @@  main(int argc, char **argv)
 		printf("Starting infinite run, if you don't see any output "
 		       "then its working properly.\n");
 	do {
+		if (ftruncate(fd, 0)) {
+			perror("Could not truncate file\n");
+			close(fd);
+			exit(1);
+		}
+
+		if (lseek(fd, 0, SEEK_SET)) {
+			perror("Could not seek set\n");
+			close(fd);
+			exit(1);
+		}
+
 		if (!map) {
 			blocks = random() % maxblocks;
 			if (blocks == 0) {
@@ -639,18 +680,6 @@  main(int argc, char **argv)
 		free(map);
 		map = NULL;
 
-		if (ftruncate(fd, 0)) {
-			perror("Could not truncate file\n");
-			close(fd);
-			exit(1);
-		}
-
-		if (lseek(fd, 0, SEEK_SET)) {
-			perror("Could not seek set\n");
-			close(fd);
-			exit(1);
-		}
-
 		if (runs) runs--;
 	} while (runs != 0);