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

[Dave Chinner <david@fromorbit.com>]

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.
-- 
Dave Chinner
david@fromorbit.com