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@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
