Re: [PATCH] btrfs: test log replay after fsync of file with prealloc extents beyond eof

[Dave Chinner <david@fromorbit.com>]

On Wed, Feb 23, 2022 at 12:12:10PM +0000, Filipe Manana wrote:
> On Tue, Feb 22, 2022 at 11:44 PM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Thu, Feb 17, 2022 at 12:14:21PM +0000, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > Test that after a full fsync of a file with preallocated extents beyond
> > > the file's size, if a power failure happens, the preallocated extents
> > > still exist after we mount the filesystem.
> > >
> > > This test currently fails and there is a patch for btrfs that fixes this
> > > issue and has the following subject:
> > >
> > >   "btrfs: fix lost prealloc extents beyond eof after full fsync"
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > ---
> > >  tests/btrfs/261     | 79 +++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/btrfs/261.out | 10 ++++++
> >
> > What is btrfs specific about this test?
> 
> The comments that explain the steps are very btrfs specific.
> Without them it would be hard to understand why the test uses that
> specific file size, block size, mention of the btrfs inode's full sync
> bit, etc.

But the behaviour and layout should end up being the same for all
filesystems, right?

We have plenty of generic tests that are designed with a
specific configuration on a specific filesystem to reproduce a bug
seen on that filesystem, but as long as all filesystems should be
expected to behave the same way, it's a generic test.

AFAICT, the behaviour described and exercised by the test should be
the same for all filesystems that support preallocation beyond EOF.
Hence it isn't btrfs specific behaviour being exercised, just a
reproducing a bug where btrfs deviates from the correct behaviour
that should be consistent across all filesystems.

> Some years ago, when you maintained fstests, you complained once about
> this type of "too btrfs specific" comments on generic tests.

I can change my mind if I want. Besides, I'm looking at this new
test purely on it's own merits so past discussions aren't really
relevant. Maybe you didn't understand the context under which I was
considering a patch to be "too fs specific" rather than generic.

There's a big difference between "only btrfs will behave this way"
and "all filesystems should get the same result, but btrfs sometimes
fails to get that results in this specific case".  The former should
be a btrfs-only test, the latter should be a generic test.

Which class this test falls into is exactly what I'm asking here -
should all filesystems get the same result, or is successful
result encoded in the golden output behaviour that only btrfs will
ever produce?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com