Enable XFSTESTS #20

donporter · 2021-02-04T20:49:52Z

Try turning on the XFSTEST jenkins job for this repo.

This change is

donporter · 2021-02-04T21:01:54Z

Test XFS please.

donporter · 2021-02-04T21:12:16Z

Test XFS please

donporter · 2021-02-04T21:12:51Z

Retest XFS please

…ironment

donporter · 2021-02-09T21:51:05Z

Retest XFS please. File system issue?

donporter · 2021-02-10T15:37:39Z

Retest XFS please. Out of memory issue in last test.

donporter · 2021-03-18T16:31:36Z

Retests Toku please. Some dicey stuff on sameagle; maybe also a legit bug...

donporter · 2021-03-18T16:31:58Z

Retest Toku please. Some dicey stuff on sameagle; maybe also a legit bug... Typo in previous request

msagarpatel

Reviewed 1 of 2 files at r1, 5 of 31 files at r2, 3 of 5 files at r3, 9 of 12 files at r4, 1 of 28 files at r5.
Reviewable status: 17 of 19 files reviewed, 8 unresolved discussions (waiting on @donporter)

benchmarks/clear-fs-caches.sh, line 36 at r4 (raw file):

elif [[ $fstype == "ftfs" ]]
then
    if [ -z ${module+x} ]; then echo "module is unset"; exit -1; fi

Is this necessary? module is defined in fs-info.sh, and that script is sourced by this script.

filesystem/ftfs_module.c, line 78 at r5 (raw file):

static void __exit ftfs_module_exit(void)
{
	destroy_ft_index();

Should all of these function declarations and definitions be removed as well?

Jenkinsfiles/Linux-xfs, line 13 at r4 (raw file):

    stage('Testing') {
      steps {
	timeout(time: 75, unit: 'MINUTES') {

15 minutes should be sufficient.

Jenkinsfiles/Linux-xfs, line 17 at r4 (raw file):

	     /usr/bin/sudo /bin/cp /host/playpen/btrfs/* /sbin/
	     /usr/bin/sudo /bin/ln -s $(/bin/pwd) /oscar/betrfs
	     cp -r /usr/src/linux-source-3.11.10-ftfs /oscar/betrfs/linux-3.11.10

Is there a reason this is being copied? I don't think it's needed.

qemu-utils/xfstests/config, line 34 at r4 (raw file):

## DEP: Version as of roughly Apr '17.  Older versions don't
##      build properly with autoconf
XFSTESTS_COMMIT=22ea2f8

Was rolling this forward intentional?

qemu-utils/xfstests/xfstests.sh, line 7 at r4 (raw file):

## Check argument count
if [ $# != 1 ] && [ $# != 2 ]; then

Remove this check.

qemu-utils/xfstests/xfstests.sh, line 14 at r4 (raw file):

## Make sure the first argument is what is expected
if [ $1 == "--force-hdd" ]; then

ditto

qemu-utils/xfstests/xfstests.sh, line 24 at r4 (raw file):

## Set KERNEL_VERSION if there is a second argument
if [[ $# -eq 2 ]]; then

Change to [[ $# -eq 1 ]] if check for --force-hdd and --force-ssd is removed.

msagarpatel

Reviewable status: 17 of 19 files reviewed, 7 unresolved discussions (waiting on @donporter)

Jenkinsfiles/Linux-xfs, line 19 at r5 (raw file):

	     cp -r /usr/src/linux-source-3.11.10-ftfs /oscar/betrfs/linux-3.11.10
	     cd /oscar/betrfs/qemu-utils/xfstests/
	     ./xfstests.sh --force-hdd

Just want to note that the parameter won't cause any problems, but it's not needed either.

qemu-utils/xfstests/xfstests.sh, line 24 at r4 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

Change to [[ $# -eq 1 ]] if check for --force-hdd and --force-ssd is removed.

Actually, remove this altogether. About the recommendations above: we can keep these checks, but they're not really needed in this repo.

msagarpatel

Reviewed 2 of 12 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @donporter)

donporter

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @msagarpatel)

benchmarks/clear-fs-caches.sh, line 36 at r4 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

Is this necessary? module is defined in fs-info.sh, and that script is sourced by this script.

Sort of. It is more like useful debugging. I hit a case where it was not set correctly, and it was not trivial to figure out why. This line of debugging info seemed worth keeping for a clearer error, should a similar problem arise in the future.

filesystem/ftfs_module.c, line 78 at r5 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

Should all of these function declarations and definitions be removed as well?

I don't understand this comment. We definitely need a module exit function. Perhaps placed in the wrong code?

Jenkinsfiles/Linux-xfs, line 13 at r4 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

15 minutes should be sufficient.

Done.

Jenkinsfiles/Linux-xfs, line 17 at r4 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

Is there a reason this is being copied? I don't think it's needed.

The linux source is not in the public repo.

qemu-utils/xfstests/config, line 34 at r4 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

Was rolling this forward intentional?

Yes.

qemu-utils/xfstests/xfstests.sh, line 7 at r4 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

Remove this check.

Done.

qemu-utils/xfstests/xfstests.sh, line 14 at r4 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

ditto

Done.

qemu-utils/xfstests/xfstests.sh, line 24 at r4 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

Actually, remove this altogether. About the recommendations above: we can keep these checks, but they're not really needed in this repo.

Done.

donporter

Reviewable status: 17 of 19 files reviewed, 7 unresolved discussions (waiting on @msagarpatel)

filesystem/ftfs_module.c, line 78 at r5 (raw file):

Previously, donporter (Don Porter) wrote…

I don't understand this comment. We definitely need a module exit function. Perhaps placed in the wrong code?

nm, I see. Explanation: what is going on here is that we moved a bunch of this code into the unmount path, rather than module unload path. The stuff that remains here really belongs here.

The code used to take some parameters at module load time that are really mountpoint-specific. For xfstests, we migrated some of them (and initialization) to mount time.

msagarpatel

Reviewed 2 of 2 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @donporter)

benchmarks/clear-fs-caches.sh, line 36 at r4 (raw file):

Previously, donporter (Don Porter) wrote…

Sort of. It is more like useful debugging. I hit a case where it was not set correctly, and it was not trivial to figure out why. This line of debugging info seemed worth keeping for a clearer error, should a similar problem arise in the future.

Would it be better to use set -u in conjunction with set -e? set -u will:

print an error message about a variable not being defined
return non-zero error code.

The latter will cause the script to stop executing if used in conjunction with set -e.

filesystem/ftfs_module.c, line 78 at r5 (raw file):

Previously, donporter (Don Porter) wrote…

nm, I see. Explanation: what is going on here is that we moved a bunch of this code into the unmount path, rather than module unload path. The stuff that remains here really belongs here.

The code used to take some parameters at module load time that are really mountpoint-specific. For xfstests, we migrated some of them (and initialization) to mount time.

My bad, I thought I retracted this comment. Thanks for the context of the changes though.

Jenkinsfiles/Linux-xfs, line 17 at r4 (raw file):

Previously, donporter (Don Porter) wrote…

The linux source is not in the public repo.

Yes, but the Jenkins jobs don't need the kernel tree. The Jenkins nodes' kernel is being mapped into the container. Refer to this parameter above: -v /playpen/linux-source-3.11.10-ftfs:/usr/src/linux-source-3.11.10-ftfs.

qemu-utils/xfstests/config, line 34 at r4 (raw file):

Previously, donporter (Don Porter) wrote…

Yes.

Resolved. Should we roll forward in the private repo?

donporter

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @msagarpatel)

benchmarks/clear-fs-caches.sh, line 36 at r4 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

Would it be better to use set -u in conjunction with set -e? set -u will:

print an error message about a variable not being defined

return non-zero error code.

The latter will cause the script to stop executing if used in conjunction with set -e.

Yes! Level up my bash!

Jenkinsfiles/Linux-xfs, line 17 at r4 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

Yes, but the Jenkins jobs don't need the kernel tree. The Jenkins nodes' kernel is being mapped into the container. Refer to this parameter above: -v /playpen/linux-source-3.11.10-ftfs:/usr/src/linux-source-3.11.10-ftfs.

Sure, let's try it.

qemu-utils/xfstests/config, line 34 at r4 (raw file):

Previously, msagarpatel (Sagar Patel) wrote…

Resolved. Should we roll forward in the private repo?

We are rolling forward to match private.

msagarpatel

Reviewed 2 of 2 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @donporter)

msagarpatel

Reviewed 1 of 1 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @donporter)

donporter · 2021-03-19T18:16:36Z

Retest Benchmarks please. Timeout - may need to bump it up

donporter · 2021-03-19T18:16:55Z

Retest Toku please. Also a timeout. May need to increase from enabling concurrent workers.

donporter added 2 commits February 4, 2021 15:49

Try turning on the xfstest job

cbf9d61

And the docker file

75aba4c

donporter added 4 commits February 5, 2021 08:30

Copy the Linux source

79088a4

Debug

30ac125

Fixes

cae5718

Import updates to the script, including some hacks to the testing env…

a3d8f8f

…ironment

donporter added 2 commits February 9, 2021 17:08

Import more missing scripts/fixes

e308c56

Add debug target to makefile

cb62f6e

donporter added 15 commits February 10, 2021 15:01

Debug

a5959ea

Debug

6e87985

Debug

44aa556

Merge branch 'master' into don/xfstest

6b522eb

Import mount argument changes needed for xfstests to work 1/2

b924b2e

Import mount argument changes needed for xfstests to work 2/2

16be710

Build fix

31d530e

Build fix

d5533fa

Build fix

55533a3

Script debug

f8e255c

A bit more debug

24a321d

Correct mount options

10b952f

Remove and restore the loopback device before/after fsck in xfstests.

14d222c

Remove debug clutter

02fecd8

Remove a bit more debugging cruft

fcd3206

donporter marked this pull request as ready for review March 18, 2021 15:48

Get proper versions of ftfs files for xfstest

cae56a2

msagarpatel requested changes Mar 18, 2021

View reviewed changes

msagarpatel reviewed Mar 18, 2021

View reviewed changes

donporter commented Mar 18, 2021

View reviewed changes

Address review feedback

6c0628c

donporter commented Mar 18, 2021

View reviewed changes

msagarpatel requested changes Mar 18, 2021

View reviewed changes

More review comments

782c893

donporter commented Mar 18, 2021

View reviewed changes

msagarpatel previously approved these changes Mar 18, 2021

View reviewed changes

Put back the cp so we get pthread_config_union.h

28f093f

donporter dismissed msagarpatel’s stale review via 28f093f March 19, 2021 01:13

msagarpatel approved these changes Mar 19, 2021

View reviewed changes

donporter merged commit 90244e8 into master Mar 20, 2021

donporter deleted the don/xfstest branch March 20, 2021 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable XFSTESTS #20

Enable XFSTESTS #20

donporter commented Feb 4, 2021 •

edited

Loading

donporter commented Feb 4, 2021

donporter commented Feb 4, 2021

donporter commented Feb 4, 2021

donporter commented Feb 9, 2021

donporter commented Feb 10, 2021

donporter commented Mar 18, 2021

donporter commented Mar 18, 2021

msagarpatel left a comment

msagarpatel left a comment

msagarpatel left a comment

donporter left a comment

donporter left a comment

msagarpatel left a comment

donporter left a comment

msagarpatel left a comment

msagarpatel left a comment

donporter commented Mar 19, 2021

donporter commented Mar 19, 2021

Enable XFSTESTS #20

Enable XFSTESTS #20

Conversation

donporter commented Feb 4, 2021 • edited Loading

donporter commented Feb 4, 2021

donporter commented Feb 4, 2021

donporter commented Feb 4, 2021

donporter commented Feb 9, 2021

donporter commented Feb 10, 2021

donporter commented Mar 18, 2021

donporter commented Mar 18, 2021

msagarpatel left a comment

Choose a reason for hiding this comment

msagarpatel left a comment

Choose a reason for hiding this comment

msagarpatel left a comment

Choose a reason for hiding this comment

donporter left a comment

Choose a reason for hiding this comment

donporter left a comment

Choose a reason for hiding this comment

msagarpatel left a comment

Choose a reason for hiding this comment

donporter left a comment

Choose a reason for hiding this comment

msagarpatel left a comment

Choose a reason for hiding this comment

msagarpatel left a comment

Choose a reason for hiding this comment

donporter commented Mar 19, 2021

donporter commented Mar 19, 2021

donporter commented Feb 4, 2021 •

edited

Loading