Re: [PATCH v3] generic: test fiemap offsets and < 512 byte ranges

[Eryu Guan <guan@eryu.me>]

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@bur.io/)
> > > 
> > > Signed-off-by: Boris Burkov <boris@bur.io>
> > 
> > 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