Re: ready for release of coreutils-8.11?

[Eric Sandeen]

On 4/12/11 7:08 PM, Pádraig Brady wrote:
> On 12/04/11 15:35, Eric Sandeen wrote:
>> On 4/12/11 9:04 AM, Pádraig Brady wrote:
>>> On 12/04/11 13:09, Jim Meyering wrote:
>>>> Pádraig Brady wrote:
>>>>> On 12/04/11 11:59, Jim Meyering wrote:
>>>>>> Pádraig Brady wrote:
>>>>>>> On 11/04/11 21:09, Jim Meyering wrote:
>>>>>>>> I've run the test suite on F15 x86_64 using each of ext3, ext4, btrfs 
>>>>>>>> and xfs.
>>>>>>>> All tests passed on the first three FS types.
>>>>>>>> On xfs there was only one failure: cp/sparse-fiemap.
>>>>>>>> I pared it down to this stand-alone reproducer:
>>>>>>>>
>>>>>>>>     rm -f j1 j2
>>>>>>>>     perl \
>>>>>>>>       -e 'BEGIN{$n=7*1024; *F=*STDOUT}' \
>>>>>>>>       -e 'for (1..21) { sysseek (*F, $n, 1)' \
>>>>>>>>       -e '&& syswrite (*F, chr($_)x$n) or die "$!"}' > j1
>>>>>>>>     cp --sparse=always j1 j2
>>>>>>>>     diff -u <(filefrag -v j1) <(filefrag -vs j2)
>>>>>>>>
>>>>>>>> Here's the output.
>>>>>>>> The difference that triggers the test failure is
>>>>>>>> the fact that the "length" numbers differ:
>>>>>>>>
>>>>>>>>     --- /proc/self/fd/11    2011-04-11 22:01:00.932737472 +0200
>>>>>>>>     +++ /proc/self/fd/12    2011-04-11 22:01:00.932737472 +0200
>>>>>>>>     @@ -1,6 +1,6 @@
>>>>>>>>      Filesystem type is: 58465342
>>>>>>>>     -File size of j1 is 301056 (74 blocks, blocksize 4096)
>>>>>>>>     +File size of j2 is 301056 (74 blocks, blocksize 4096)
>>>>>>>>       ext logical physical expected length flags
>>>>>>>>     -   0       1     7310              31
>>>>>>>>     -   1      33     7342     7340     95 eof
>>>>>>>>     -j1: 3 extents found
>>>>>>>>     +   0       1     7469              31
>>>>>>>>     +   1      33     7501     7499     63 eof
>>>>>>>>     +j2: 3 extents found
>>>>>>>
>>>>>>> Hmm, this is with 128K block sizes?
>>>>>>
>>>>>> The perl snippet performs 21 pairs of lseek/syswrite calls,
>>>>>> each of length 7KiB.  Not sure about the 3 extents, or why
>>>>>> only the first 4KiB block and one other in the middle of the
>>>>>> file end up being holes on XFS.
>>>>>
>>>>>> Hey, do you feel like extending your fiemap-capable script
>>>>>> to print FIEMAP data?  Then we wouldn't have to rely on filefrag,
>>>>>> which appears to be confused here.
>>>>>
>>>>> I'm not sure filefrag is at fault here.
>>>>> It just shifts and prints the returned lengths.
>>>>> Could you post the output of this (with the -r option)
>>>>> http://oss.oracle.com/~mason/fiemap-test.c
>>>>
>>>> Good idea.
>>>>
>>>>   $ gcc -O -Wall fiemap-test.c
>>>>   fiemap-test.c: In function 'show_extents_table':
>>>>   fiemap-test.c:138:9: warning: variable 'last_logical' set but not used \
>>>>     [-Wunused-but-set-variable]
>>>>
>>>> Note that here I'm using 1..31, not 1..21:
>>>>
>>>>   $ rm -f j1 j2; perl -e 'BEGIN{$n=7*1024; *F=*STDOUT}' \
>>>>     -e 'for (1..31) { sysseek (*F, $n, 1)' \
>>>>     -e '&& syswrite (*F, chr($_)x$n) or die "$!"}' > j1
>>>>   $ cp --sparse=always j1 j2 && cmp j1 j2
>>>>
>>>> Uh oh.  du reports different sizes:
>>>>
>>>>   $ du -sh j1 j2
>>>>   504K    j1
>>>>   632K    j2
>>>>
>>>> Not only that, but note how the sizes are *larger*
>>>> than raw byte counts.  This means there's a hole at EOF:
>>>>
>>>>   $ wc -c j1 j2
>>>>   444416 j1
>>>>   444416 j2
>>>>
>>>> Here's the result of running fiemap-test:
>>>>
>>>>   $ {./a.out -r j1; ./a.out -r j2}|sed 's/:   /:/g'
>>>>   Extent   0: logical:    4096 physical: 4272128 length:  126976 flags 
>>>> 0x000
>>>>   Extent   1: logical:  135168 physical: 4403200 length:  389120 flags 
>>>> 0x001
>>>>   Extent   0: logical:    4096 physical: 4792320 length:  126976 flags 
>>>> 0x000
>>>>   Extent   1: logical:  135168 physical: 5193728 length:  520192 flags 
>>>> 0x001
>>>>
>>>> No wonder those final "length" numbers differed.
>>>> This looks like an XFS bug.
>>>
>>> Well not necessarily.
>>> I suppose that XFS may be allocating the extents differently
>>> in the destination for performance reasons.
>>> I.E. It's laying out the 128KiB chunks first and not
>>> trimming trailing slop.
>>>
>>> So if that was the case, we'd have to get our fiemap comparison
>>> to discount anything after a specified length.
>>
>> I'd say don't get too fancy.
>>
>> The only thing the fileystem needs to get right on the copy is file size and 
>> contents.
>> If those match after a copy, the test should pass.
> 
> Right. However, we'd like to verify that holes
> are being propagated from source to dest,
> and a fiemap list is the only way I know to test that.

But unfortunately the filesystem is not actually required to put a hole 
anywhere...

> Also it's unlikely I think that holes would be
> introduced differently in the dest, given the
> processing that cp does.

Depends on what writeback is doing during the test, etc, and probably many 
other things.

> So I'm leaning towards relaxing the test,
> by ignoring the length of the last extent
> along the lines of the following.

I think it's fragile because the fs can change behavior at will.  But it's your 
testcase. :)

-Eric

> cheers,
> Pádraig.
> 
> diff --git a/tests/filefrag-extent-compare b/tests/filefrag-extent-compare
> index 2c33584..c8b840c 100644
> --- a/tests/filefrag-extent-compare
> +++ b/tests/filefrag-extent-compare
> @@ -56,11 +56,24 @@ merge_extents \@b;
>  my $i = 0;
>  while (defined $a[$i])
>    {
> -    $a[$i]->{L_BLK} == $b[$i]->{L_BLK} && $a[$i]->{LEN} == $b[$i]->{LEN}
> -      or die "$ME: differing extent:\n"
> -        . "  [$i]=$a[$i]->{L_BLK} $a[$i]->{LEN}\n"
> -          . "  [$i]=$b[$i]->{L_BLK} $b[$i]->{LEN}\n";
> -    $i++;
> +    if ($a[$i]->{L_BLK} == $b[$i]->{L_BLK} && $a[$i]->{LEN} == $b[$i]->{LEN})
> +      {
> +        $i++;
> +      }
> +    elsif ($i == (@a - 1) && $a[$i]->{L_BLK} == $b[$i]->{L_BLK})
> +      {
> +        # On XFS it was seen that the size of the last extent can vary,
> +        # and can extend beyond the length of the file.
> +        # So ignore the length of the last extent, because if the
> +        # file is the wrong length we'll get failures elsewhere.
> +        last;
> +      }
> +    else
> +      {
> +        die "$ME: differing extent:\n"
> +          . "  [$i]=$a[$i]->{L_BLK} $a[$i]->{LEN}\n"
> +            . "  [$i]=$b[$i]->{L_BLK} $b[$i]->{LEN}\n";
> +      }
>    }
> 
>  ### Setup "GNU" style for perl-mode and cperl-mode.