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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 24 May 2011 10:42:23 +0800
From:	Yongqiang Yang <xiaoqiangnk@...il.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	sandeen@...hat.com, josef@...hat.com, linux-ext4@...r.kernel.org,
	xfs@....sgi.com
Subject: Re: [PATCH v3] xfstests:Make 225 compare map and fiemap at each block.

Thank you for your review.
On Tue, May 24, 2011 at 9:51 AM, Dave Chinner <david@...morbit.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.
>
> 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@...morbit.com
>



-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html