On Wed, Apr 21, 2021 at 12:13:33PM -0700, Boris Burkov wrote: > On Sun, Apr 11, 2021 at 10:03:59PM +0800, Eryu Guan wrote: > > On Wed, Apr 07, 2021 at 01:13:26PM -0700, Boris Burkov wrote: > > > btrfs trims fiemap extents to the inputted offset, which leads to > > > inconsistent results for most inputs, and downright bizarre outputs like > > > [7..6] when the trimmed extent is at the end of an extent and shorter > > > than 512 bytes. > > > > > > The test writes out one extent of the file system's block size and tries > > > fiemaps at various offsets. It expects that all the fiemaps return the > > > full single extent. > > > > > > I ran it under the following fs, block size combinations: > > > ext2: 1024, 2048, 4096 > > > ext3: 1024, 2048, 4096 > > > ext4: 1024, 2048, 4096 > > > xfs: 512, 1024, 2048, 4096 > > > f2fs: 4096 > > > btrfs: 4096 > > > > > > This test is fixed for btrfs by: > > > btrfs: return whole extents in fiemap > > > (https://lore.kernel.org/linux-btrfs/274e5bcebdb05a8969fc300b4802f33da2fbf218.1617746680.git.boris@xxxxxx/) > > > > > > Signed-off-by: Boris Burkov <boris@xxxxxx> > > > > generic/473, which tests fiemap, has been marked as broken, as fiemap > > behavior is not consistent across filesystems, and the specific behavior > > tested by generic/473 is not defined and filesystems could have > > different implementations. > > > > I'm not sure if this test fits into the undefined-behavior fiemap > > categary. I think it's fine if it tests a well-defined & consistent > > behavior. > > > > Interesting, I didn't know about that test being marked as broken. > > I was worried about this problem to some extent and attempted to > mitigate it by only requiring that all the output be the same, rather > than matching some specific standard. > > Thinking about it further, I think this test is portable only so long as > the step where it writes a file with one extent is portable. > > If "pwrite 0 block-size" ends up as a file with multiple extents, then > it is possible one of the partial fiemaps will only intersect with a > subset of the extents and rightly return those. In fact, that was broken > in the original version of the test which explicitly used 4096 instead of > being detecting the block size. > > I do think it is nice to have this as a regression test for btrfs, since > we have pretty complicated logic for fiemap and it was so broken in this > case. If you prefer, I can make this a btrfs specific test. Yeah, a btrfs specific test seems safer, and we could move it to generic later if the behavior is well defined. Thanks, Eryu
