Re: ready for release of coreutils-8.11?

[Pádraig Brady]

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.

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

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

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.