All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/2] fsstress: add splice support
@ 2019-01-19  2:42 Zorro Lang
  2019-01-19  2:42 ` [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output Zorro Lang
  0 siblings, 1 reply; 13+ messages in thread
From: Zorro Lang @ 2019-01-19  2:42 UTC (permalink / raw)
  To: fstests

Support the splice syscall in fsstress.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---

V3 did below changes:
1) Change the logic to calculate the 'len'
2) Change the logic to calculete the 'off2'
3) If ret2 < 0, break from the outer while loop too
4) Always prints errno (even it's 0) at the end.

Thanks,
Zorro

 ltp/fsstress.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 173 insertions(+)

diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 99a1d733..c04feb78 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -85,6 +85,7 @@ typedef enum {
 	OP_RMDIR,
 	OP_SETATTR,
 	OP_SETXATTR,
+	OP_SPLICE,
 	OP_STAT,
 	OP_SYMLINK,
 	OP_SYNC,
@@ -194,6 +195,7 @@ void	resvsp_f(int, long);
 void	rmdir_f(int, long);
 void	setattr_f(int, long);
 void	setxattr_f(int, long);
+void	splice_f(int, long);
 void	stat_f(int, long);
 void	symlink_f(int, long);
 void	sync_f(int, long);
@@ -244,6 +246,7 @@ opdesc_t	ops[] = {
 	{ OP_RMDIR, "rmdir", rmdir_f, 1, 1 },
 	{ OP_SETATTR, "setattr", setattr_f, 0, 1 },
 	{ OP_SETXATTR, "setxattr", setxattr_f, 1, 1 },
+	{ OP_SPLICE, "splice", splice_f, 1, 1 },
 	{ OP_STAT, "stat", stat_f, 1, 0 },
 	{ OP_SYMLINK, "symlink", symlink_f, 2, 1 },
 	{ OP_SYNC, "sync", sync_f, 1, 1 },
@@ -2764,6 +2767,176 @@ setxattr_f(int opno, long r)
 #endif
 }
 
+void
+splice_f(int opno, long r)
+{
+	struct pathname		fpath1;
+	struct pathname		fpath2;
+	struct stat64		stat1;
+	struct stat64		stat2;
+	char			inoinfo1[1024];
+	char			inoinfo2[1024];
+	loff_t			lr;
+	loff_t			off1, off2;
+	size_t			len;
+	loff_t			offset1, offset2;
+	size_t			length;
+	size_t			total;
+	int			v1;
+	int			v2;
+	int			fd1;
+	int			fd2;
+	ssize_t			ret1 = 0, ret2 = 0;
+	size_t			bytes;
+	int			e;
+	int			filedes[2];
+
+	/* Load paths */
+	init_pathname(&fpath1);
+	if (!get_fname(FT_REGm, r, &fpath1, NULL, NULL, &v1)) {
+		if (v1)
+			printf("%d/%d: splice read - no filename\n",
+				procid, opno);
+		goto out_fpath1;
+	}
+
+	init_pathname(&fpath2);
+	if (!get_fname(FT_REGm, random(), &fpath2, NULL, NULL, &v2)) {
+		if (v2)
+			printf("%d/%d: splice write - no filename\n",
+				procid, opno);
+		goto out_fpath2;
+	}
+
+	/* Open files */
+	fd1 = open_path(&fpath1, O_RDONLY);
+	e = fd1 < 0 ? errno : 0;
+	check_cwd();
+	if (fd1 < 0) {
+		if (v1)
+			printf("%d/%d: splice read - open %s failed %d\n",
+				procid, opno, fpath1.path, e);
+		goto out_fpath2;
+	}
+
+	fd2 = open_path(&fpath2, O_WRONLY);
+	e = fd2 < 0 ? errno : 0;
+	check_cwd();
+	if (fd2 < 0) {
+		if (v2)
+			printf("%d/%d: splice write - open %s failed %d\n",
+				procid, opno, fpath2.path, e);
+		goto out_fd1;
+	}
+
+	/* Get file stats */
+	if (fstat64(fd1, &stat1) < 0) {
+		if (v1)
+			printf("%d/%d: splice read - fstat64 %s failed %d\n",
+				procid, opno, fpath1.path, errno);
+		goto out_fd2;
+	}
+	inode_info(inoinfo1, sizeof(inoinfo1), &stat1, v1);
+
+	if (fstat64(fd2, &stat2) < 0) {
+		if (v2)
+			printf("%d/%d: splice write - fstat64 %s failed %d\n",
+				procid, opno, fpath2.path, errno);
+		goto out_fd2;
+	}
+	inode_info(inoinfo2, sizeof(inoinfo2), &stat2, v2);
+
+	/* Calculate offsets */
+	len = (random() % FILELEN_MAX) + 1;
+	if (len == 0)
+		len = stat1.st_blksize;
+	if (len > stat1.st_size)
+		len = stat1.st_size;
+
+	lr = ((int64_t)random() << 32) + random();
+	if (stat1.st_size == len)
+		off1 = 0;
+	else
+		off1 = (off64_t)(lr % MIN(stat1.st_size - len, MAXFSIZE));
+	off1 %= maxfsize;
+
+	/*
+	 * splice can overlap write, so the offset of the target file can be
+	 * any number (< maxfsize)
+	 */
+	lr = ((int64_t)random() << 32) + random();
+	off2 = (off64_t)(lr % maxfsize);
+
+	/*
+	 * Due to len, off1 and off2 will be changed later, so record the
+	 * original number at here
+	 */
+	length = len;
+	offset1 = off1;
+	offset2 = off2;
+
+	/* Pipe initialize */
+	if (pipe(filedes) < 0) {
+		if (v1 || v2) {
+			printf("%d/%d: splice - pipe failed %d\n",
+				procid, opno, errno);
+			goto out_fd2;
+		}
+	}
+
+	bytes = 0;
+	total = 0;
+	while (len > 0) {
+		/* move to pipe buffer */
+		ret1 = splice(fd1, &off1, filedes[1], NULL, len, 0);
+		if (ret1 < 0) {
+			break;
+		}
+		bytes = ret1;
+
+		/* move from pipe buffer to dst file */
+		while (bytes > 0) {
+			ret2 = splice(filedes[0], NULL, fd2, &off2, bytes, 0);
+			if (ret2 < 0) {
+				break;
+			}
+			bytes -= ret2;
+		}
+		if (ret2 < 0)
+			break;
+
+		len -= ret1;
+		total += ret1;
+	}
+
+	if (ret1 < 0 || ret2 < 0)
+		e = errno;
+	else
+		e = 0;
+	if (v1 || v2) {
+		printf("%d/%d: splice %s%s [%lld,%lld] -> %s%s [%lld,%lld] %d",
+			procid, opno,
+			fpath1.path, inoinfo1, (long long)offset1, (long long)length,
+			fpath2.path, inoinfo2, (long long)offset2, (long long)length, e);
+
+		if (length && length > total)
+			printf(" asked for %lld, spliced %lld??\n",
+				(long long)length, (long long)total);
+		printf("\n");
+	}
+
+	close(filedes[0]);
+	close(filedes[1]);
+out_fd2:
+	close(fd2);
+out_fd1:
+	close(fd1);
+out_fpath2:
+	free_pathname(&fpath2);
+out_fpath1:
+	free_pathname(&fpath1);
+}
+
 void
 creat_f(int opno, long r)
 {
-- 
2.17.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-19  2:42 [PATCH v3 1/2] fsstress: add splice support Zorro Lang
@ 2019-01-19  2:42 ` Zorro Lang
  2019-01-21 22:44   ` Dave Chinner
  0 siblings, 1 reply; 13+ messages in thread
From: Zorro Lang @ 2019-01-19  2:42 UTC (permalink / raw)
  To: fstests

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
 xfsrestore: directory post-processing
 xfsrestore: restoring non-directory files
 xfsrestore: restore complete: SECS seconds elapsed
-- 
2.17.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-19  2:42 ` [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output Zorro Lang
@ 2019-01-21 22:44   ` Dave Chinner
  2019-01-22  3:33     ` Amir Goldstein
  0 siblings, 1 reply; 13+ messages in thread
From: Dave Chinner @ 2019-01-21 22:44 UTC (permalink / raw)
  To: Zorro Lang; +Cc: fstests

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?

Cheers,

dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-21 22:44   ` Dave Chinner
@ 2019-01-22  3:33     ` Amir Goldstein
  2019-01-22  4:58       ` Zorro Lang
  0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2019-01-22  3:33 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Zorro Lang, fstests

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).

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-22  3:33     ` Amir Goldstein
@ 2019-01-22  4:58       ` Zorro Lang
  2019-01-22 22:22         ` Dave Chinner
  0 siblings, 1 reply; 13+ messages in thread
From: Zorro Lang @ 2019-01-22  4:58 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Amir Goldstein, fstests

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.
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.

Thanks,
Zorro

> 
> Thanks,
> Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-22  4:58       ` Zorro Lang
@ 2019-01-22 22:22         ` Dave Chinner
  2019-01-23  3:44           ` Zorro Lang
  0 siblings, 1 reply; 13+ messages in thread
From: Dave Chinner @ 2019-01-22 22:22 UTC (permalink / raw)
  To: Zorro Lang; +Cc: Amir Goldstein, fstests

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...

> 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.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-22 22:22         ` Dave Chinner
@ 2019-01-23  3:44           ` Zorro Lang
  2019-01-23  4:12             ` Dave Chinner
  2019-01-23  7:35             ` Amir Goldstein
  0 siblings, 2 replies; 13+ messages in thread
From: Zorro Lang @ 2019-01-23  3:44 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Amir Goldstein, fstests

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-23  3:44           ` Zorro Lang
@ 2019-01-23  4:12             ` Dave Chinner
  2019-01-23  7:35             ` Amir Goldstein
  1 sibling, 0 replies; 13+ messages in thread
From: Dave Chinner @ 2019-01-23  4:12 UTC (permalink / raw)
  To: Zorro Lang; +Cc: Amir Goldstein, fstests

On Wed, Jan 23, 2019 at 11:44:06AM +0800, Zorro Lang wrote:
> 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:
> > > > 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."

splice() != sendfile().

sendfile has been around since the 2.2 days, originally implemented
to allow web servers to do zero-copy of file data to the network.
The ability to do file-to-file copies was in the original 2.2/2.4
implementation, but it was removed in the 2.5 series.  Back around
2.6.32 it got converted to use the splice(2) kernel infrastructure
and started supporting files as the destination again, and that's
where it all went off the rails....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-23  3:44           ` Zorro Lang
  2019-01-23  4:12             ` Dave Chinner
@ 2019-01-23  7:35             ` Amir Goldstein
  2019-01-23  8:41               ` Zorro Lang
  2019-01-23 22:06               ` Dave Chinner
  1 sibling, 2 replies; 13+ messages in thread
From: Amir Goldstein @ 2019-01-23  7:35 UTC (permalink / raw)
  To: Zorro Lang; +Cc: Dave Chinner, fstests, Darrick J. Wong, Eric Sandeen

> > > 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.
>

On the same discussion when Zorro added "insert" op, I asked
why not convert test to whitelist of ops instead of having to update
the blacklist every time fsstress is extended (which happened 3 times
since we had this discussion).

To my question, Dave answered:
(https://marc.info/?l=fstests&m=149013211607727&w=2)

> That means dump tests will /never/ exercise new fsstress operations
> which, IMO, is much worse than having to keep a test up to date with
> new operations.
>
> i.e. The whole point of adding new operations to fsstress and have
> tests run them is that we automatically get coverage of new
> functionality. Neutering this for all xfsdump tests by using "-z" is
> - philosophically speaking - the wrong thing to do.

For the life of me, I cannot follow the logic of how this argument ends
up with a decision to update the blacklist. It is just me?

Yes, if we use a whitelist we will /never/ exercise new fsstress operations for
xfs/068. It may be the wrong thing to do "philosophically speaking" but in
practice, we are doing this anyway, just manually every time an op is added.

This is just madness.

Eryu, can you please step in and end this ordeal once and for all.

If we still want to exercise new fsstress ops for xfs/022 and future xfsdump
tests (do we?) the solution is quite simple:
factor out _create_dumpdir_stress_num_param and pass in whitelist
of ops only from xfs/068 and be done with that.

Zorro, be the final decision as it may, blacklist or whitelist, the test 068 is
currently verifying that the file count is what happened to be the file count
after addition of "insert" and "mread/mwrite" ops, which to the best of my
knowledge, no one verified is the correct file count.

So if we want to leave this test "unchanged", we'd better revert your last two
changes to 068.out and resort to Eric's original test output and also
exclude (or not include in whitelist) insert/mread/mwrite ops.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-23  7:35             ` Amir Goldstein
@ 2019-01-23  8:41               ` Zorro Lang
  2019-01-23  8:59                 ` Amir Goldstein
  2019-01-23 22:06               ` Dave Chinner
  1 sibling, 1 reply; 13+ messages in thread
From: Zorro Lang @ 2019-01-23  8:41 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Dave Chinner, fstests, Darrick J. Wong, Eric Sandeen

On Wed, Jan 23, 2019 at 09:35:47AM +0200, Amir Goldstein wrote:
> > > > 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.
> >
> 
> On the same discussion when Zorro added "insert" op, I asked
> why not convert test to whitelist of ops instead of having to update
> the blacklist every time fsstress is extended (which happened 3 times
> since we had this discussion).
> 
> To my question, Dave answered:
> (https://marc.info/?l=fstests&m=149013211607727&w=2)
> 
> > That means dump tests will /never/ exercise new fsstress operations
> > which, IMO, is much worse than having to keep a test up to date with
> > new operations.
> >
> > i.e. The whole point of adding new operations to fsstress and have
> > tests run them is that we automatically get coverage of new
> > functionality. Neutering this for all xfsdump tests by using "-z" is
> > - philosophically speaking - the wrong thing to do.
> 
> For the life of me, I cannot follow the logic of how this argument ends
> up with a decision to update the blacklist. It is just me?
> 
> Yes, if we use a whitelist we will /never/ exercise new fsstress operations for
> xfs/068. It may be the wrong thing to do "philosophically speaking" but in
> practice, we are doing this anyway, just manually every time an op is added.
> 
> This is just madness.
> 
> Eryu, can you please step in and end this ordeal once and for all.
> 
> If we still want to exercise new fsstress ops for xfs/022 and future xfsdump
> tests (do we?) the solution is quite simple:
> factor out _create_dumpdir_stress_num_param and pass in whitelist
> of ops only from xfs/068 and be done with that.
> 
> Zorro, be the final decision as it may, blacklist or whitelist, the test 068 is
> currently verifying that the file count is what happened to be the file count
> after addition of "insert" and "mread/mwrite" ops, which to the best of my
> knowledge, no one verified is the correct file count.
> 
> So if we want to leave this test "unchanged", we'd better revert your last two
> changes to 068.out and resort to Eric's original test output and also
> exclude (or not include in whitelist) insert/mread/mwrite ops.

:) I don't mind re-adding mread/mwrite operations into FSSTRESS_AVOID.

At that time, we haven't decided either "change 068.out" or "avoid new operations"
is better. So maybe both are fine (my personal point). But Darrick *had to* brought
in FSSTRESS_AVOID into common/dump (commit ID: 451504891), he can't change 068.out
directly due to dedupe/clonerange/copyrange aren't supported by all xfs
configurations.

So my original point is: change 068.out if add common&stable operations into
fsstress, otherwise add into FSSTRESS_AVOID in common/dump. As Dave specified
so many problems of splice(), I don't mind avoiding it. But mread/mwrite are
common functions, as we've changed 068.out for them, how about keep the current
code until we find something wrong in the future :)

Thanks,
Zorro

> 
> Thanks,
> Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-23  8:41               ` Zorro Lang
@ 2019-01-23  8:59                 ` Amir Goldstein
  0 siblings, 0 replies; 13+ messages in thread
From: Amir Goldstein @ 2019-01-23  8:59 UTC (permalink / raw)
  To: Zorro Lang; +Cc: Dave Chinner, fstests, Darrick J. Wong, Eric Sandeen

On Wed, Jan 23, 2019 at 10:37 AM Zorro Lang <zlang@redhat.com> wrote:
>
> On Wed, Jan 23, 2019 at 09:35:47AM +0200, Amir Goldstein wrote:
> > > > > 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.
> > >
> >
> > On the same discussion when Zorro added "insert" op, I asked
> > why not convert test to whitelist of ops instead of having to update
> > the blacklist every time fsstress is extended (which happened 3 times
> > since we had this discussion).
> >
> > To my question, Dave answered:
> > (https://marc.info/?l=fstests&m=149013211607727&w=2)
> >
> > > That means dump tests will /never/ exercise new fsstress operations
> > > which, IMO, is much worse than having to keep a test up to date with
> > > new operations.
> > >
> > > i.e. The whole point of adding new operations to fsstress and have
> > > tests run them is that we automatically get coverage of new
> > > functionality. Neutering this for all xfsdump tests by using "-z" is
> > > - philosophically speaking - the wrong thing to do.
> >
> > For the life of me, I cannot follow the logic of how this argument ends
> > up with a decision to update the blacklist. It is just me?
> >
> > Yes, if we use a whitelist we will /never/ exercise new fsstress operations for
> > xfs/068. It may be the wrong thing to do "philosophically speaking" but in
> > practice, we are doing this anyway, just manually every time an op is added.
> >
> > This is just madness.
> >
> > Eryu, can you please step in and end this ordeal once and for all.
> >
> > If we still want to exercise new fsstress ops for xfs/022 and future xfsdump
> > tests (do we?) the solution is quite simple:
> > factor out _create_dumpdir_stress_num_param and pass in whitelist
> > of ops only from xfs/068 and be done with that.
> >
> > Zorro, be the final decision as it may, blacklist or whitelist, the test 068 is
> > currently verifying that the file count is what happened to be the file count
> > after addition of "insert" and "mread/mwrite" ops, which to the best of my
> > knowledge, no one verified is the correct file count.
> >
> > So if we want to leave this test "unchanged", we'd better revert your last two
> > changes to 068.out and resort to Eric's original test output and also
> > exclude (or not include in whitelist) insert/mread/mwrite ops.
>
> :) I don't mind re-adding mread/mwrite operations into FSSTRESS_AVOID.
>
> At that time, we haven't decided either "change 068.out" or "avoid new operations"
> is better. So maybe both are fine (my personal point). But Darrick *had to* brought
> in FSSTRESS_AVOID into common/dump (commit ID: 451504891), he can't change 068.out
> directly due to dedupe/clonerange/copyrange aren't supported by all xfs
> configurations.
>
> So my original point is: change 068.out if add common&stable operations into
> fsstress, otherwise add into FSSTRESS_AVOID in common/dump. As Dave specified
> so many problems of splice(), I don't mind avoiding it. But mread/mwrite are
> common functions, as we've changed 068.out for them, how about keep the current
> code until we find something wrong in the future :)
>

Frankly, I don't care what the decision is only that we properly
document it near the
code and stop having those discussions every time fsstress gets a new op.
I personally don't think that the fact that splice() is broken in
older kernels matters
at all. The argument that different xfs configurations will produce
different output
(which is the case for dedue/clone) is the only argument that is relevant IMO.
I would very much like for Eryu to weight in on the decision.

To be fair, it you do want to keep mread/mwrite (and what about
insert?) than you
should manually check the number of files created by the fsstress configuration
and verify that it matches the number you updated in golden output.
Blindly updating the golden output to whatever the output happens to be was
wrong and we need to fix that.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-23  7:35             ` Amir Goldstein
  2019-01-23  8:41               ` Zorro Lang
@ 2019-01-23 22:06               ` Dave Chinner
  2019-01-24  9:28                 ` Amir Goldstein
  1 sibling, 1 reply; 13+ messages in thread
From: Dave Chinner @ 2019-01-23 22:06 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Zorro Lang, fstests, Darrick J. Wong, Eric Sandeen

On Wed, Jan 23, 2019 at 09:35:47AM +0200, Amir Goldstein wrote:
> > > > 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.
> >
> 
> On the same discussion when Zorro added "insert" op, I asked
> why not convert test to whitelist of ops instead of having to update
> the blacklist every time fsstress is extended (which happened 3 times
> since we had this discussion).
> 
> To my question, Dave answered:
> (https://marc.info/?l=fstests&m=149013211607727&w=2)
> 
> > That means dump tests will /never/ exercise new fsstress operations
> > which, IMO, is much worse than having to keep a test up to date with
> > new operations.
> >
> > i.e. The whole point of adding new operations to fsstress and have
> > tests run them is that we automatically get coverage of new
> > functionality. Neutering this for all xfsdump tests by using "-z" is
> > - philosophically speaking - the wrong thing to do.
> 
> For the life of me, I cannot follow the logic of how this argument ends
> up with a decision to update the blacklist. It is just me?

I'm not sure why you are so worked up over this

I was saying that that turning off everything (i.e. -z) and using a
whitelist is the wrong thing to do because it means nobody will ever
consider that xfsdump might need to be tested against that new
fsstress functionality/.

i.e. out of sight, out of mind.

The current blacklist method means that whenever something is added
to fsstress, tests will fail and the person making the modification
will have to consider if that operation should be used in the test
that fails.

That's the whole point - if the test doesn't fail, it nobody ever
considers if the new operation is relevant for xfsdump tests and so
it will never get added. That's bad if we actually need test
coverage of that functionality. However, if the test fails on new
additions, then people will consider whether it should be added or
not, and we discuss it appropriately for *that specific addtion*.

IOWs, using "-z" and a whitelist is philosophically the wrong thing
to do because it means that nobody will ever consider whether tests
using fstress should have new fsstress functionality enabled or
disabled.

> Yes, if we use a whitelist we will /never/ exercise new fsstress operations for
> xfs/068. It may be the wrong thing to do "philosophically speaking" but in
> practice, we are doing this anyway, just manually every time an op is added.

Because all the recent additions are the same. i.e.  fsstress is
just creating regular files differently. It has no impact on xfsdump
does except to change the number of files created and the directory
layout.

If this new functionality were creating a new type of file that
xfsdump has to handle, or adding new attributes or changing the
metadata of the existing files, then we want to make sure xfsdump is
tested against that, and so we'd be changing the golden output after
careful checking that both xfsdump and xfs_restore are working
correctly and the file count is correct.

But when all we are doing is creating normal, regular files just
with a different syscall, it makes no sense to perturb the existing
test then we have to go and validate that the new set of files being
tested is actually scanned correctly, is complete and correct. Using
a blacklist to avoid unnecessary perturbation such as in cases like
this is the right thing to do because we've had to determine if the
new functionality is a useful addition to xfsdump/restore test
coverage or not.

> If we still want to exercise new fsstress ops for xfs/022 and future xfsdump
> tests (do we?) the solution is quite simple:
> factor out _create_dumpdir_stress_num_param and pass in whitelist
> of ops only from xfs/068 and be done with that.
> 
> Zorro, be the final decision as it may, blacklist or whitelist, the test 068 is
> currently verifying that the file count is what happened to be the file count
> after addition of "insert" and "mread/mwrite" ops, which to the best of my
> knowledge, no one verified is the correct file count.

I suspect that it's just commit 5d36d8585afc ("xfs/068: update golden
output due to new operations in fsstress)" that you are ranting
about - the insert operation was added back in 2015 and xfs/068 had
this added to it:

+# need to ensure new fsstress operations don't perturb expected output
+FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID"

Which followed the rules of "don't perturb if unnecessary". This was
removed by the above commit, which didn't follow that rule. And that
happened while I was on sabbatical and so nobody was there to say
"don't do that" or "how did you validate that the count of files is
correct".  It's probably correct given that we haven't seen any
issues for the past 18 months anywhere and we've since added a bunch
of new and reviewed fsstress avoids for xfs/068, so I think this
commit is the anomaly rather than the rule....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
  2019-01-23 22:06               ` Dave Chinner
@ 2019-01-24  9:28                 ` Amir Goldstein
  0 siblings, 0 replies; 13+ messages in thread
From: Amir Goldstein @ 2019-01-24  9:28 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Zorro Lang, fstests, Darrick J. Wong, Eric Sandeen

> > On the same discussion when Zorro added "insert" op, I asked
> > why not convert test to whitelist of ops instead of having to update
> > the blacklist every time fsstress is extended (which happened 3 times
> > since we had this discussion).
> >
> > To my question, Dave answered:
> > (https://marc.info/?l=fstests&m=149013211607727&w=2)
> >
> > > That means dump tests will /never/ exercise new fsstress operations
> > > which, IMO, is much worse than having to keep a test up to date with
> > > new operations.
> > >
> > > i.e. The whole point of adding new operations to fsstress and have
> > > tests run them is that we automatically get coverage of new
> > > functionality. Neutering this for all xfsdump tests by using "-z" is
> > > - philosophically speaking - the wrong thing to do.
> >
> > For the life of me, I cannot follow the logic of how this argument ends
> > up with a decision to update the blacklist. It is just me?
>
> I'm not sure why you are so worked up over this
>

Me neither. Why do we even get worked up over anything? ;-)

> I was saying that that turning off everything (i.e. -z) and using a
> whitelist is the wrong thing to do because it means nobody will ever
> consider that xfsdump might need to be tested against that new
> fsstress functionality/.
>
> i.e. out of sight, out of mind.
>
> The current blacklist method means that whenever something is added
> to fsstress, tests will fail and the person making the modification
> will have to consider if that operation should be used in the test
> that fails.
>
> That's the whole point - if the test doesn't fail, it nobody ever
> considers if the new operation is relevant for xfsdump tests and so
> it will never get added. That's bad if we actually need test
> coverage of that functionality. However, if the test fails on new
> additions, then people will consider whether it should be added or
> not, and we discuss it appropriately for *that specific addtion*.
>
> IOWs, using "-z" and a whitelist is philosophically the wrong thing
> to do because it means that nobody will ever consider whether tests
> using fstress should have new fsstress functionality enabled or
> disabled.
>

Fine.

I posted a patch to add file count by 'find' to golden output.
This way, at least when updating the golden output for new ops,
reviewers can clearly see that the file count reported by xfsrestore
is sane.

BTW, file count reported by xfsrestore has 1 directory more than
file count reported by 'find'. I don't know why that is, but it has been
like that since the original version of golden output by Eric (155cf517
"xfs/068: fix expected output file for proper restore output")

> > Yes, if we use a whitelist we will /never/ exercise new fsstress operations for
> > xfs/068. It may be the wrong thing to do "philosophically speaking" but in
> > practice, we are doing this anyway, just manually every time an op is added.
>
> Because all the recent additions are the same. i.e.  fsstress is
> just creating regular files differently. It has no impact on xfsdump
> does except to change the number of files created and the directory
> layout.
>
> If this new functionality were creating a new type of file that
> xfsdump has to handle, or adding new attributes or changing the
> metadata of the existing files, then we want to make sure xfsdump is
> tested against that, and so we'd be changing the golden output after
> careful checking that both xfsdump and xfs_restore are working
> correctly and the file count is correct.
>
> But when all we are doing is creating normal, regular files just
> with a different syscall, it makes no sense to perturb the existing
> test then we have to go and validate that the new set of files being
> tested is actually scanned correctly, is complete and correct. Using
> a blacklist to avoid unnecessary perturbation such as in cases like
> this is the right thing to do because we've had to determine if the
> new functionality is a useful addition to xfsdump/restore test
> coverage or not.
>

Reality check: the only functionality that added new attributes/metadata
lately is clone/reflink and for practical reasons (not supported on all xfs
config) new functionality was not added to dump/restore test.
It is most likely going to be the same outcome if a future new metadata
feature is added.

Worse is that if we go by your philosophy, than xfsdump is just one of
many other functionalities that we do not re-verify test coverage whenever
adding new fstress ops.
Take xfs/107 for example. It uses a whitelist and constant random seed
to check project quota.
Should we have examined adding insert/collapse/zero/clonerange/etc..
to this test? No. What would have been the maintenance burden if we
had many more tests that we had to examine every time that a new
fsstress op is added.


> > If we still want to exercise new fsstress ops for xfs/022 and future xfsdump
> > tests (do we?) the solution is quite simple:
> > factor out _create_dumpdir_stress_num_param and pass in whitelist
> > of ops only from xfs/068 and be done with that.
> >
> > Zorro, be the final decision as it may, blacklist or whitelist, the test 068 is
> > currently verifying that the file count is what happened to be the file count
> > after addition of "insert" and "mread/mwrite" ops, which to the best of my
> > knowledge, no one verified is the correct file count.
>
> I suspect that it's just commit 5d36d8585afc ("xfs/068: update golden
> output due to new operations in fsstress)" that you are ranting

There are two commits:
d613bee2 xfs/068: update golden output due to new operations in fsstress
5d36d858 xfs/068: update golden output due to new operations in fsstress

One embraces "insert" the other embraces
"mread/mwrite/aread/awrite/writev/readv"

> about - the insert operation was added back in 2015 and xfs/068 had
> this added to it:
>
> +# need to ensure new fsstress operations don't perturb expected output
> +FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID"
>
> Which followed the rules of "don't perturb if unnecessary". This was
> removed by the above commit, which didn't follow that rule. And that
> happened while I was on sabbatical and so nobody was there to say
> "don't do that" or "how did you validate that the count of files is
> correct".  It's probably correct given that we haven't seen any
> issues for the past 18 months anywhere and we've since added a bunch
> of new and reviewed fsstress avoids for xfs/068, so I think this
> commit is the anomaly rather than the rule....
>

Any development workflow that depends on Dave Chinner not being on
sabbatical is a wrong development workflow (because you deserve rest ;-))
Seriously, it was my opinion that we are better off embracing new ops at the
time, there were no outstanding objections and Eryu called it.
We may have done wrong by not asking "how did you validate that the
count of files is correct" or maybe Zorro did verify - in the past now.

Now I am stepping away from this discussion because I am not a stake
holder. xfstests maintainer and xfsprogs maintainer are the main stake
holders here. The patch that I posted comes to serve both in a way that
if we ever do decide to embrace any new ops into the test, it will be
easier for reviewers to verify that the file count is correct.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-01-24  9:28 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-19  2:42 [PATCH v3 1/2] fsstress: add splice support Zorro Lang
2019-01-19  2:42 ` [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output Zorro Lang
2019-01-21 22:44   ` Dave Chinner
2019-01-22  3:33     ` Amir Goldstein
2019-01-22  4:58       ` Zorro Lang
2019-01-22 22:22         ` Dave Chinner
2019-01-23  3:44           ` Zorro Lang
2019-01-23  4:12             ` Dave Chinner
2019-01-23  7:35             ` Amir Goldstein
2019-01-23  8:41               ` Zorro Lang
2019-01-23  8:59                 ` Amir Goldstein
2019-01-23 22:06               ` Dave Chinner
2019-01-24  9:28                 ` Amir Goldstein

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.