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

[Dave Chinner <david@fromorbit.com>]

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs