fsstress: add splice support
PATCH v3 1/2 fsstress: add splice support
Support the splice syscall in fsstress.
---
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(+)
ltp/fsstress.c +173 -0
@@ -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) {
Signature · 1 line
2.17.2
PATCH v3 2/2 xfs/068: new fsstress operation breaks xfsrestore output
After fsstress has new operation, xfs/068 has different dump output, so change the expected number of files and directories.
--- tests/xfs/068.out | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tests/xfs/068.out +1 -1
@@ -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
Signature · 1 line
2.17.2
Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
Quoted 13 lines · Zorro Lang
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 <[email protected]>--- tests/xfs/068.out | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)tests/xfs/068.out +0
-0
tests/xfs/068.out xfsrestore: session id: ID · line 25
Quoted 5 lines
tests/xfs/068.out +1
-1@@ -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.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
Quoted 25 lines · Dave Chinner
Quoted 13 lines · Zorro Lang
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 <[email protected]>--- tests/xfs/068.out | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)tests/xfs/068.out +0
-0tests/xfs/068.out xfsrestore: session id: ID · line 25
Quoted 5 lines
tests/xfs/068.out +1
-1@@ -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 processedShouldn'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?
Quoted 3 lines
i.e. changing
FSSTRESS_AVOIDin 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.
Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
Quoted 30 lines · Amir Goldstein
Quoted 25 lines · Dave Chinner
Quoted 13 lines · Zorro Lang
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 <[email protected]>--- tests/xfs/068.out | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)tests/xfs/068.out +0
-0tests/xfs/068.out xfsrestore: session id: ID · line 25
Quoted 5 lines
tests/xfs/068.out +1
-1@@ -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 processedShouldn'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?
Quoted 3 lines
i.e. changing
FSSTRESS_AVOIDin common/dump as is done already to turn off dedupe/clone/copy_file_range for xfsdump/restore tests?
Quoted 8 lines
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
Quoted 3 lines
Thanks,
Amir.
Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
Quoted 47 lines · Zorro Lang
Quoted 30 lines · Amir Goldstein
Quoted 25 lines · Dave Chinner
Quoted 13 lines · Zorro Lang
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 <[email protected]>--- tests/xfs/068.out | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)tests/xfs/068.out +0
-0tests/xfs/068.out xfsrestore: session id: ID · line 25
Quoted 5 lines
tests/xfs/068.out +1
-1@@ -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 processedShouldn'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?
Quoted 3 lines
i.e. changing
FSSTRESS_AVOIDin common/dump as is done already to turn off dedupe/clone/copy_file_range for xfsdump/restore tests?Quoted 8 lines
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_AVOIDin 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.
Quoted 2 lines
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 <[email protected]> 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...
Quoted 2 lines
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.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
Quoted 66 lines · Dave Chinner
Quoted 47 lines · Zorro Lang
Quoted 30 lines · Amir Goldstein
Quoted 25 lines · Dave Chinner
Quoted 13 lines · Zorro Lang
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 <[email protected]>--- tests/xfs/068.out | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)tests/xfs/068.out +0
-0tests/xfs/068.out xfsrestore: session id: ID · line 25
Quoted 5 lines
tests/xfs/068.out +1
-1@@ -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 processedShouldn'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?
Quoted 3 lines
i.e. changing
FSSTRESS_AVOIDin common/dump as is done already to turn off dedupe/clone/copy_file_range for xfsdump/restore tests?Quoted 8 lines
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_AVOIDin 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.Quoted 2 lines
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
0ff28d9f4674d781e492bcff6f32f0fe48cf0fedAuthor: Christophe Leroy <[email protected]> Date: Wed May 6 17:26:47 2015 +0200splice:
sendfile()at once fails for big filesUsing 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 usessendfile()to send a file to anAF_ALGsocket for hashing.
.....
Quoted 8 lines
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."
Quoted 12 lines
Quoted 3 lines
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.
Quoted 4 lines
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
Quoted 7 lines
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
Quoted 41 lines · Zorro Lang
Quoted 31 lines · Dave Chinner
Quoted 12 lines · Zorro Lang
Quoted 4 lines
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_AVOIDin 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.Quoted 2 lines
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
0ff28d9f4674d781e492bcff6f32f0fe48cf0fedAuthor: Christophe Leroy <[email protected]> Date: Wed May 6 17:26:47 2015 +0200splice:
sendfile()at once fails for big filesUsing 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 usessendfile()to send a file to anAF_ALGsocket for hashing.
.....Quoted 8 lines
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...
Quoted 3 lines
Sorry I didn't learn about that. I just checked "man 2 splice", it said:
"Thesplice()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.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
In reply to Zorro LangQuoted 11 lines
Quoted 11 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 4 lines
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_AVOIDin 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)
Quoted 8 lines
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.
Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
Quoted 53 lines · Amir Goldstein
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 4 lines
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_AVOIDin 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)Quoted 8 lines
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_paramand pass in whitelist of ops only from xfs/068 and be done with that.
Quoted 8 lines
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
Quoted 3 lines
Thanks,
Amir.
Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
Quoted 72 lines · Zorro Lang
Quoted 53 lines · Amir Goldstein
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 4 lines
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_AVOIDin 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)Quoted 8 lines
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_paramand pass in whitelist of ops only from xfs/068 and be done with that.Quoted 8 lines
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_AVOIDinto common/dump (commit ID: 451504891), he can't change 068.out directly due to dedupe/clonerange/copyrange aren't supported by all xfs configurations.
Quoted 6 lines
So my original point is: change 068.out if add common&stable operations into fsstress, otherwise add into
FSSTRESS_AVOIDin common/dump. As Dave specified so many problems ofsplice(), 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.
Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
In reply to Amir GoldsteinQuoted 32 lines · Amir Goldstein
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 4 lines
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_AVOIDin 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)Quoted 4 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 9 lines
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_paramand 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.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v3 2/2] xfs/068: new fsstress operation breaks xfsrestore output
Quoted 17 lines
Quoted 17 lines
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)Quoted 8 lines
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.
Quoted 5 lines
Quoted 2 lines
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? ;-)
Quoted 19 lines
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*.
Quoted 5 lines
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")
Quoted 16 lines
Quoted 3 lines
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_restoreare working correctly and the file count is correct.
Quoted 9 lines
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.
Quoted 5 lines
Quoted 5 lines
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_paramand pass in whitelist of ops only from xfs/068 and be done with that.
Quoted 7 lines
Quoted 4 lines
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"
Quoted 6 lines
about - the insert operation was added back in 2015 and xfs/068 had this added to it:
(quoted diff) +2
-0+# need to ensure new fsstress operations don't perturb expected output +FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID"
Quoted 9 lines
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.