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@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. 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. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
