Thank you for your review. 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 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@xxxxxxxxxxxxx > -- Best Wishes Yongqiang Yang _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
