Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output

[Zorro Lang <zlang@redhat.com>]

On Wed, Jan 23, 2019 at 09:22:16AM +1100, Dave Chinner wrote:
> On Tue, Jan 22, 2019 at 12:58:54PM +0800, Zorro Lang wrote:
> > On Tue, Jan 22, 2019 at 05:33:19AM +0200, Amir Goldstein wrote:
> > > On Tue, Jan 22, 2019 at 12:44 AM Dave Chinner <david@fromorbit.com> wrote:
> > > >
> > > > On Sat, Jan 19, 2019 at 10:42:58AM +0800, Zorro Lang wrote:
> > > > > After fsstress has new operation, xfs/068 has different dump output,
> > > > > so change the expected number of files and directories.
> > > > >
> > > > > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > > > > ---
> > > > >  tests/xfs/068.out | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tests/xfs/068.out b/tests/xfs/068.out
> > > > > index fa3a5523..f9ec37b8 100644
> > > > > --- a/tests/xfs/068.out
> > > > > +++ b/tests/xfs/068.out
> > > > > @@ -22,7 +22,7 @@ xfsrestore: session id: ID
> > > > >  xfsrestore: media ID: ID
> > > > >  xfsrestore: searching media for directory dump
> > > > >  xfsrestore: reading directories
> > > > > -xfsrestore: 383 directories and 1335 entries processed
> > > > > +xfsrestore: 416 directories and 1373 entries processed
> > > >
> > > > Shouldn't you tell fsstress to avoid this new operation so none of
> > > > the tests that rely on the number of files created by a specific
> > > > fstress seed break?
> > > >
> > > > i.e. changing FSSTRESS_AVOID in common/dump as is done already to
> > > > turn off dedupe/clone/copy_file_range for xfsdump/restore tests?
> > > >
> > > 
> > > IIRC, dedupe/clone/copy_file_range were blacklisted not for not
> > > changing golden output, but because they make the golden output
> > > dependent on xfs reflink feature (i.e. on mkfs options).
> > > 
> > > This is not the case with the newly added SPLICE operation and
> > > updating the golden output rather than blacklisting the new operation
> > > is what was decided and done two times in recent history (also patches
> > > by Zorro).
> > 
> > Yes, I saw that FSSTRESS_AVOID in common/dump. But I thought (if I'm wrong
> > please feel free to correct me):
> > 1) dedupe/clone/copyrange might not be compiled in fsstress, they depend
> >    on "#ifndef ... #endif". That will make different xfsdump/restore output
> >    when a fs with or without these operations. Then there's not a fixed 068.out.
> >    So they must be avoided.
> 
> > 2) splice syscall is an old common function, which is first appeared in Linux
> >    2.6.17. So I think it's safe to be added.
> 
> What makes you think it worked in 2.6.17? I mean:
> 
> commit 0ff28d9f4674d781e492bcff6f32f0fe48cf0fed
> Author: Christophe Leroy <christophe.leroy@c-s.fr>
> Date:   Wed May 6 17:26:47 2015 +0200
> 
>     splice: sendfile() at once fails for big files
>     
>     Using sendfile with below small program to get MD5 sums of some files,
>     it appear that big files (over 64kbytes with 4k pages system) get a
>     wrong MD5 sum while small files get the correct sum.
>     This program uses sendfile() to send a file to an AF_ALG socket
>     for hashing.
> .....
> 
> i.e. using sendfile to copy large files resulted in corrupt copies
> up until 2015.
> 
> Basically, sendfile is a legacy interface that has spend a very
> large amount of it's time broken, full of deadlocks (i.e. unusable)
> and not used by anyone. It's not something we really want to enable
> in xfstests because it will cause all sorts of failures on older
> distros...

Sorry I didn't learn about that. I just checked "man 2 splice", it said:
"The splice() system call first appeared in Linux 2.6.17; library support was
added to glibc in version 2.5."

> 
> > 3) Either change common/dump or xfs/068.out doesn't take much time/code, so I
> >    decided to change 068.out to make x/068 to do operations.
> 
> Did you validate that the file count is actually correct?
> 
> The file count in these dump/restore tests is how we tell we've
> broken bulkstat iteration - any mismatch on the file count indicates
> bulkstat didn't find something it should have or it counted things
> twice. Hence I'm wary making changes to the file set size the
> dump/restore tests use.
> 
> And, really, changing the file set size somewhat invalidates the
> long regression history we have with these existing tests. i.e.
> don't change tests that work correctly unless it's absolutely
> necessary. It only adds to the risk of introducing new bugs.

Thanks so much for your detailed description :) I'll turn to change
FSSTRESS_AVOID in next V4 patch.

Thanks,
Zorro

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com