xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v3] xfstests:Make 225 compare map and fiemap at each block.
From: Yongqiang Yang <xiaoqiangnk@xxxxxxxxx>
Date: Tue, 24 May 2011 15:13:24 +0800
Cc: sandeen@xxxxxxxxxx, josef@xxxxxxxxxx, linux-ext4@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=CX+3UMfhAFPQ9HlhLqE24E9bT9Ir+FIuWW8JIBHluTM=; b=ih8q3LaEGuRBpoZT1hFlvJvT6cgK5z/79UE7DKR36X48VL43/Do2k1XpM5EPCrd2CR wWQyIe8N8t9TMsfIUDzQ5PZe6UnVtVD6/nw25o2MzDoJWH98gRW2eCpd9TSbyBhQSKnJ NkEjNiRbF+bdvty8zwNvqaEc9UOgQTUvm83yk=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=BSLLj4PhfhEnvXpG4UoCXtgLpL9imddxSLIYeTtdIe0aduT7wn6J8arMkoK91w++nt BKhtYvItVuEMHo6ytS89jHH+NN8b7w1kBguzdBaEqwoY1PH1eCUPsvkNWZjkPJzwWjrf /3MZFTvk/ADKKdko5mFsHyYlWoff3OEE6na78=
In-reply-to: <20110524015105.GE32466@dastard>
References: <1306134423-10028-1-git-send-email-xiaoqiangnk@xxxxxxxxx> <20110524015105.GE32466@dastard>
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

<Prev in Thread] Current Thread [Next in Thread>