On Tue, May 24, 2011 at 9:51 AM, Dave Chinner <david@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx > -- Best Wishes Yongqiang Yang _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
