[PATCHSET v2 0/2] fstests: improve coredump capture and storage

[PATCHSET v2 0/2] fstests: improve coredump capture and storage

[Darrick J. Wong]

Hi all,

While I was debugging some core dumps resulting from xfs_repair fuzz
testing, I noticed that fstests was no longer putting core dumps into
the test results directory.  The root cause of this turned out to be
that systemd enables kernel.core_uses_pid, which changes the core
pattern from "core" to "core.$pid".  This is a good change since we can
now capture *all* the coredumps produced by a test, but bad in that the
check script doesn't know about this.

Therefore, fix check to detect multiple corefiles and preserve them, and
compress coredumps if desired.

v2: let the user specify which program they want to use to compress the
    coredumps

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=compress-core-dumps
---
 README    |    4 ++++
 check     |   26 ++++++++++++++++++++++----
 common/rc |   20 ++++++++++++++++++++
 3 files changed, 46 insertions(+), 4 deletions(-)

[PATCH 1/2] check: detect and preserve all coredumps made by a test

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If someone sets kernel.core_uses_pid (or kernel.core_pattern), any
coredumps generated by fstests might have names that are longer than
just "core".  Since the pid isn't all that useful by itself, let's
record the coredumps by hash when we save them, so that we don't waste
space storing identical crash dumps.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 check     |   26 ++++++++++++++++++++++----
 common/rc |   16 ++++++++++++++++
 2 files changed, 38 insertions(+), 4 deletions(-)


diff --git a/check b/check
index af23572ccc..654d986b27 100755
--- a/check
+++ b/check
@@ -913,11 +913,19 @@ function run_section()
 			sts=$?
 		fi
 
-		if [ -f core ]; then
-			_dump_err_cont "[dumped core]"
-			mv core $RESULT_BASE/$seqnum.core
+		# If someone sets kernel.core_pattern or kernel.core_uses_pid,
+		# coredumps generated by fstests might have a longer name than
+		# just "core".  Use globbing to find the most common patterns,
+		# assuming there are no other coredump capture packages set up.
+		local cores=0
+		for i in core core.*; do
+			test -f "$i" || continue
+			if ((cores++ == 0)); then
+				_dump_err_cont "[dumped core]"
+			fi
+			_save_coredump "$i"
 			tc_status="fail"
-		fi
+		done
 
 		if [ -f $seqres.notrun ]; then
 			$timestamp && _timestamp
@@ -950,6 +958,16 @@ function run_section()
 			# of the check script itself.
 			(_adjust_oom_score 250; _check_filesystems) || tc_status="fail"
 			_check_dmesg || tc_status="fail"
+
+			# Save any coredumps from the post-test fs checks
+			for i in core core.*; do
+				test -f "$i" || continue
+				if ((cores++ == 0)); then
+					_dump_err_cont "[dumped core]"
+				fi
+				_save_coredump "$i"
+				tc_status="fail"
+			done
 		fi
 
 		# Reload the module after each test to check for leaks or
diff --git a/common/rc b/common/rc
index d877ac77a0..152b8bb414 100644
--- a/common/rc
+++ b/common/rc
@@ -4949,6 +4949,22 @@ _create_file_sized()
 	return $ret
 }
 
+_save_coredump()
+{
+	local path="$1"
+
+	local core_hash="$(_md5_checksum "$path")"
+	local out_file="$RESULT_BASE/$seqnum.core.$core_hash"
+
+	if [ -s "$out_file" ]; then
+		rm -f "$path"
+		return
+	fi
+	rm -f "$out_file"
+
+	mv "$path" "$out_file"
+}
+
 init_rc
 
 ################################################################################

[PATCH 2/2] check: optionally compress core dumps

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Add a new option, COREDUMP_COMPRESSOR, that will be used to compress
core dumps collected during a fstests run.  The program specified must
accept the -f -9 arguments that gzip has.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 README    |    4 ++++
 common/rc |   14 +++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)


diff --git a/README b/README
index 80d148be82..4c4f22f853 100644
--- a/README
+++ b/README
@@ -212,6 +212,10 @@ Tools specification:
     - Set FSSTRESS_AVOID and/or FSX_AVOID, which contain options added to
       the end of fsstresss and fsx invocations, respectively, in case you wish
       to exclude certain operational modes from these tests.
+ - core dumps:
+    - Set COREDUMP_COMPRESSOR to a compression program to compress crash dumps.
+      This program must accept '-f' and the name of a file to compress.  In
+      other words, it must emulate gzip.
 
 Kernel/Modules related configuration:
  - Set TEST_FS_MODULE_RELOAD=1 to unload the module and reload it between
diff --git a/common/rc b/common/rc
index 152b8bb414..c68869b7dc 100644
--- a/common/rc
+++ b/common/rc
@@ -4956,13 +4956,17 @@ _save_coredump()
 	local core_hash="$(_md5_checksum "$path")"
 	local out_file="$RESULT_BASE/$seqnum.core.$core_hash"
 
-	if [ -s "$out_file" ]; then
-		rm -f "$path"
-		return
-	fi
-	rm -f "$out_file"
+	for dump in "$out_file"*; do
+		if [ -s "$dump" ]; then
+			rm -f "$path"
+			return 0
+		fi
+	done
 
 	mv "$path" "$out_file"
+	test -z "$COREDUMP_COMPRESSOR" && return 0
+
+	$COREDUMP_COMPRESSOR -f "$out_file"
 }
 
 init_rc

Re: [PATCH 1/2] check: detect and preserve all coredumps made by a test

[Zorro Lang]

On Tue, Oct 11, 2022 at 06:45:13PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If someone sets kernel.core_uses_pid (or kernel.core_pattern), any
> coredumps generated by fstests might have names that are longer than
> just "core".  Since the pid isn't all that useful by itself, let's
> record the coredumps by hash when we save them, so that we don't waste
> space storing identical crash dumps.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  check     |   26 ++++++++++++++++++++++----
>  common/rc |   16 ++++++++++++++++
>  2 files changed, 38 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/check b/check
> index af23572ccc..654d986b27 100755
> --- a/check
> +++ b/check
> @@ -913,11 +913,19 @@ function run_section()
>  			sts=$?
>  		fi
>  
> -		if [ -f core ]; then
> -			_dump_err_cont "[dumped core]"
> -			mv core $RESULT_BASE/$seqnum.core
> +		# If someone sets kernel.core_pattern or kernel.core_uses_pid,
> +		# coredumps generated by fstests might have a longer name than
> +		# just "core".  Use globbing to find the most common patterns,
> +		# assuming there are no other coredump capture packages set up.
> +		local cores=0
> +		for i in core core.*; do
> +			test -f "$i" || continue
> +			if ((cores++ == 0)); then
> +				_dump_err_cont "[dumped core]"
> +			fi
> +			_save_coredump "$i"
>  			tc_status="fail"
> -		fi
> +		done
>  
>  		if [ -f $seqres.notrun ]; then
>  			$timestamp && _timestamp
> @@ -950,6 +958,16 @@ function run_section()
>  			# of the check script itself.
>  			(_adjust_oom_score 250; _check_filesystems) || tc_status="fail"
>  			_check_dmesg || tc_status="fail"
> +
> +			# Save any coredumps from the post-test fs checks
> +			for i in core core.*; do
> +				test -f "$i" || continue
> +				if ((cores++ == 0)); then
> +					_dump_err_cont "[dumped core]"
> +				fi
> +				_save_coredump "$i"
> +				tc_status="fail"
> +			done
>  		fi
>  
>  		# Reload the module after each test to check for leaks or
> diff --git a/common/rc b/common/rc
> index d877ac77a0..152b8bb414 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4949,6 +4949,22 @@ _create_file_sized()
>  	return $ret
>  }
>  
> +_save_coredump()
> +{
> +	local path="$1"
> +
> +	local core_hash="$(_md5_checksum "$path")"
> +	local out_file="$RESULT_BASE/$seqnum.core.$core_hash"

I doubt this can work with fstests section, if we use section, the out_file
should be "$RESULT_BASE/$section/....", so I think if we can write this line
as:

  local out_file="$seqres.core.$core_hash"

Or use $REPORT_DIR?

Thanks,
Zorro

> +
> +	if [ -s "$out_file" ]; then
> +		rm -f "$path"
> +		return
> +	fi
> +	rm -f "$out_file"
> +
> +	mv "$path" "$out_file"
> +}
> +
>  init_rc
>  
>  ################################################################################
> 

Re: [PATCH 1/2] check: detect and preserve all coredumps made by a test

[Darrick J. Wong]

On Wed, Oct 12, 2022 at 11:47:21PM +0800, Zorro Lang wrote:
> On Tue, Oct 11, 2022 at 06:45:13PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > If someone sets kernel.core_uses_pid (or kernel.core_pattern), any
> > coredumps generated by fstests might have names that are longer than
> > just "core".  Since the pid isn't all that useful by itself, let's
> > record the coredumps by hash when we save them, so that we don't waste
> > space storing identical crash dumps.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  check     |   26 ++++++++++++++++++++++----
> >  common/rc |   16 ++++++++++++++++
> >  2 files changed, 38 insertions(+), 4 deletions(-)
> > 
> > 
> > diff --git a/check b/check
> > index af23572ccc..654d986b27 100755
> > --- a/check
> > +++ b/check
> > @@ -913,11 +913,19 @@ function run_section()
> >  			sts=$?
> >  		fi
> >  
> > -		if [ -f core ]; then
> > -			_dump_err_cont "[dumped core]"
> > -			mv core $RESULT_BASE/$seqnum.core
> > +		# If someone sets kernel.core_pattern or kernel.core_uses_pid,
> > +		# coredumps generated by fstests might have a longer name than
> > +		# just "core".  Use globbing to find the most common patterns,
> > +		# assuming there are no other coredump capture packages set up.
> > +		local cores=0
> > +		for i in core core.*; do
> > +			test -f "$i" || continue
> > +			if ((cores++ == 0)); then
> > +				_dump_err_cont "[dumped core]"
> > +			fi
> > +			_save_coredump "$i"
> >  			tc_status="fail"
> > -		fi
> > +		done
> >  
> >  		if [ -f $seqres.notrun ]; then
> >  			$timestamp && _timestamp
> > @@ -950,6 +958,16 @@ function run_section()
> >  			# of the check script itself.
> >  			(_adjust_oom_score 250; _check_filesystems) || tc_status="fail"
> >  			_check_dmesg || tc_status="fail"
> > +
> > +			# Save any coredumps from the post-test fs checks
> > +			for i in core core.*; do
> > +				test -f "$i" || continue
> > +				if ((cores++ == 0)); then
> > +					_dump_err_cont "[dumped core]"
> > +				fi
> > +				_save_coredump "$i"
> > +				tc_status="fail"
> > +			done
> >  		fi
> >  
> >  		# Reload the module after each test to check for leaks or
> > diff --git a/common/rc b/common/rc
> > index d877ac77a0..152b8bb414 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -4949,6 +4949,22 @@ _create_file_sized()
> >  	return $ret
> >  }
> >  
> > +_save_coredump()
> > +{
> > +	local path="$1"
> > +
> > +	local core_hash="$(_md5_checksum "$path")"
> > +	local out_file="$RESULT_BASE/$seqnum.core.$core_hash"
> 
> I doubt this can work with fstests section, if we use section, the out_file
> should be "$RESULT_BASE/$section/....", so I think if we can write this line
> as:
> 
>   local out_file="$seqres.core.$core_hash"
> 
> Or use $REPORT_DIR?

I'll change it to REPORT_DIR.

--D

> Thanks,
> Zorro
> 
> > +
> > +	if [ -s "$out_file" ]; then
> > +		rm -f "$path"
> > +		return
> > +	fi
> > +	rm -f "$out_file"
> > +
> > +	mv "$path" "$out_file"
> > +}
> > +
> >  init_rc
> >  
> >  ################################################################################
> > 
> 

[PATCH v2.1 1/2] check: detect and preserve all coredumps made by a test

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If someone sets kernel.core_uses_pid (or kernel.core_pattern), any
coredumps generated by fstests might have names that are longer than
just "core".  Since the pid isn't all that useful by itself, let's
record the coredumps by hash when we save them, so that we don't waste
space storing identical crash dumps.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v2.1: use REPORT_DIR per maintainer suggestion
---
 check     |   26 ++++++++++++++++++++++----
 common/rc |   16 ++++++++++++++++
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/check b/check
index d587a70546..29303db1c8 100755
--- a/check
+++ b/check
@@ -923,11 +923,19 @@ function run_section()
 			sts=$?
 		fi
 
-		if [ -f core ]; then
-			_dump_err_cont "[dumped core]"
-			mv core $RESULT_BASE/$seqnum.core
+		# If someone sets kernel.core_pattern or kernel.core_uses_pid,
+		# coredumps generated by fstests might have a longer name than
+		# just "core".  Use globbing to find the most common patterns,
+		# assuming there are no other coredump capture packages set up.
+		local cores=0
+		for i in core core.*; do
+			test -f "$i" || continue
+			if ((cores++ == 0)); then
+				_dump_err_cont "[dumped core]"
+			fi
+			_save_coredump "$i"
 			tc_status="fail"
-		fi
+		done
 
 		if [ -f $seqres.notrun ]; then
 			$timestamp && _timestamp
@@ -960,6 +968,16 @@ function run_section()
 			# of the check script itself.
 			(_adjust_oom_score 250; _check_filesystems) || tc_status="fail"
 			_check_dmesg || tc_status="fail"
+
+			# Save any coredumps from the post-test fs checks
+			for i in core core.*; do
+				test -f "$i" || continue
+				if ((cores++ == 0)); then
+					_dump_err_cont "[dumped core]"
+				fi
+				_save_coredump "$i"
+				tc_status="fail"
+			done
 		fi
 
 		# Reload the module after each test to check for leaks or
diff --git a/common/rc b/common/rc
index d877ac77a0..2e1891180a 100644
--- a/common/rc
+++ b/common/rc
@@ -4949,6 +4949,22 @@ _create_file_sized()
 	return $ret
 }
 
+_save_coredump()
+{
+	local path="$1"
+
+	local core_hash="$(_md5_checksum "$path")"
+	local out_file="$REPORT_DIR/$seqnum.core.$core_hash"
+
+	if [ -s "$out_file" ]; then
+		rm -f "$path"
+		return
+	fi
+	rm -f "$out_file"
+
+	mv "$path" "$out_file"
+}
+
 init_rc
 
 ################################################################################

Re: [PATCH v2.1 1/2] check: detect and preserve all coredumps made by a test

[Zorro Lang]

On Wed, Oct 12, 2022 at 05:19:50PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If someone sets kernel.core_uses_pid (or kernel.core_pattern), any
> coredumps generated by fstests might have names that are longer than
> just "core".  Since the pid isn't all that useful by itself, let's
> record the coredumps by hash when we save them, so that we don't waste
> space storing identical crash dumps.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v2.1: use REPORT_DIR per maintainer suggestion
> ---

This version looks good to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

>  check     |   26 ++++++++++++++++++++++----
>  common/rc |   16 ++++++++++++++++
>  2 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/check b/check
> index d587a70546..29303db1c8 100755
> --- a/check
> +++ b/check
> @@ -923,11 +923,19 @@ function run_section()
>  			sts=$?
>  		fi
>  
> -		if [ -f core ]; then
> -			_dump_err_cont "[dumped core]"
> -			mv core $RESULT_BASE/$seqnum.core
> +		# If someone sets kernel.core_pattern or kernel.core_uses_pid,
> +		# coredumps generated by fstests might have a longer name than
> +		# just "core".  Use globbing to find the most common patterns,
> +		# assuming there are no other coredump capture packages set up.
> +		local cores=0
> +		for i in core core.*; do
> +			test -f "$i" || continue
> +			if ((cores++ == 0)); then
> +				_dump_err_cont "[dumped core]"
> +			fi
> +			_save_coredump "$i"
>  			tc_status="fail"
> -		fi
> +		done
>  
>  		if [ -f $seqres.notrun ]; then
>  			$timestamp && _timestamp
> @@ -960,6 +968,16 @@ function run_section()
>  			# of the check script itself.
>  			(_adjust_oom_score 250; _check_filesystems) || tc_status="fail"
>  			_check_dmesg || tc_status="fail"
> +
> +			# Save any coredumps from the post-test fs checks
> +			for i in core core.*; do
> +				test -f "$i" || continue
> +				if ((cores++ == 0)); then
> +					_dump_err_cont "[dumped core]"
> +				fi
> +				_save_coredump "$i"
> +				tc_status="fail"
> +			done
>  		fi
>  
>  		# Reload the module after each test to check for leaks or
> diff --git a/common/rc b/common/rc
> index d877ac77a0..2e1891180a 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4949,6 +4949,22 @@ _create_file_sized()
>  	return $ret
>  }
>  
> +_save_coredump()
> +{
> +	local path="$1"
> +
> +	local core_hash="$(_md5_checksum "$path")"
> +	local out_file="$REPORT_DIR/$seqnum.core.$core_hash"
> +
> +	if [ -s "$out_file" ]; then
> +		rm -f "$path"
> +		return
> +	fi
> +	rm -f "$out_file"
> +
> +	mv "$path" "$out_file"
> +}
> +
>  init_rc
>  
>  ################################################################################
> 

Re: [PATCH 2/2] check: optionally compress core dumps

[Zorro Lang]

On Tue, Oct 11, 2022 at 06:45:18PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Add a new option, COREDUMP_COMPRESSOR, that will be used to compress
> core dumps collected during a fstests run.  The program specified must
> accept the -f -9 arguments that gzip has.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  README    |    4 ++++
>  common/rc |   14 +++++++++-----
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/README b/README
> index 80d148be82..4c4f22f853 100644
> --- a/README
> +++ b/README
> @@ -212,6 +212,10 @@ Tools specification:
>      - Set FSSTRESS_AVOID and/or FSX_AVOID, which contain options added to
>        the end of fsstresss and fsx invocations, respectively, in case you wish
>        to exclude certain operational modes from these tests.
> + - core dumps:
> +    - Set COREDUMP_COMPRESSOR to a compression program to compress crash dumps.
> +      This program must accept '-f' and the name of a file to compress.  In
> +      other words, it must emulate gzip.
>  
>  Kernel/Modules related configuration:
>   - Set TEST_FS_MODULE_RELOAD=1 to unload the module and reload it between
> diff --git a/common/rc b/common/rc
> index 152b8bb414..c68869b7dc 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4956,13 +4956,17 @@ _save_coredump()
>  	local core_hash="$(_md5_checksum "$path")"
>  	local out_file="$RESULT_BASE/$seqnum.core.$core_hash"
>  
> -	if [ -s "$out_file" ]; then
> -		rm -f "$path"
> -		return
> -	fi
> -	rm -f "$out_file"
> +	for dump in "$out_file"*; do
> +		if [ -s "$dump" ]; then
> +			rm -f "$path"
> +			return 0
> +		fi
> +	done
>  
>  	mv "$path" "$out_file"
> +	test -z "$COREDUMP_COMPRESSOR" && return 0
> +
> +	$COREDUMP_COMPRESSOR -f "$out_file"

This patch looks good to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

I'm just not sure if all/most compressor supports "-f" option, I use bzip2
and gzip mostly, they both support that.

Thanks,
Zorro

>  }
>  
>  init_rc
> 

Re: [PATCH v2.1 1/2] check: detect and preserve all coredumps made by a test

[Darrick J. Wong]

On Thu, Oct 13, 2022 at 07:44:46PM +0800, Zorro Lang wrote:
> On Wed, Oct 12, 2022 at 05:19:50PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > If someone sets kernel.core_uses_pid (or kernel.core_pattern), any
> > coredumps generated by fstests might have names that are longer than
> > just "core".  Since the pid isn't all that useful by itself, let's
> > record the coredumps by hash when we save them, so that we don't waste
> > space storing identical crash dumps.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > v2.1: use REPORT_DIR per maintainer suggestion
> > ---
> 
> This version looks good to me,
> Reviewed-by: Zorro Lang <zlang@redhat.com>

It occurred to me overnight that ./check doesn't export REPORT_DIR, so
I'll push out a v2.2 that adds that.  Currently the lack of an export
doesn't affect anyone, but as soon as any tests want to call
_save_coredump they're going to run into that issue.

(...and yes, I do have future fuzz tests that will call it from a test
in between fuzz field cycles.)

--D

> >  check     |   26 ++++++++++++++++++++++----
> >  common/rc |   16 ++++++++++++++++
> >  2 files changed, 38 insertions(+), 4 deletions(-)
> > 
> > diff --git a/check b/check
> > index d587a70546..29303db1c8 100755
> > --- a/check
> > +++ b/check
> > @@ -923,11 +923,19 @@ function run_section()
> >  			sts=$?
> >  		fi
> >  
> > -		if [ -f core ]; then
> > -			_dump_err_cont "[dumped core]"
> > -			mv core $RESULT_BASE/$seqnum.core
> > +		# If someone sets kernel.core_pattern or kernel.core_uses_pid,
> > +		# coredumps generated by fstests might have a longer name than
> > +		# just "core".  Use globbing to find the most common patterns,
> > +		# assuming there are no other coredump capture packages set up.
> > +		local cores=0
> > +		for i in core core.*; do
> > +			test -f "$i" || continue
> > +			if ((cores++ == 0)); then
> > +				_dump_err_cont "[dumped core]"
> > +			fi
> > +			_save_coredump "$i"
> >  			tc_status="fail"
> > -		fi
> > +		done
> >  
> >  		if [ -f $seqres.notrun ]; then
> >  			$timestamp && _timestamp
> > @@ -960,6 +968,16 @@ function run_section()
> >  			# of the check script itself.
> >  			(_adjust_oom_score 250; _check_filesystems) || tc_status="fail"
> >  			_check_dmesg || tc_status="fail"
> > +
> > +			# Save any coredumps from the post-test fs checks
> > +			for i in core core.*; do
> > +				test -f "$i" || continue
> > +				if ((cores++ == 0)); then
> > +					_dump_err_cont "[dumped core]"
> > +				fi
> > +				_save_coredump "$i"
> > +				tc_status="fail"
> > +			done
> >  		fi
> >  
> >  		# Reload the module after each test to check for leaks or
> > diff --git a/common/rc b/common/rc
> > index d877ac77a0..2e1891180a 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -4949,6 +4949,22 @@ _create_file_sized()
> >  	return $ret
> >  }
> >  
> > +_save_coredump()
> > +{
> > +	local path="$1"
> > +
> > +	local core_hash="$(_md5_checksum "$path")"
> > +	local out_file="$REPORT_DIR/$seqnum.core.$core_hash"
> > +
> > +	if [ -s "$out_file" ]; then
> > +		rm -f "$path"
> > +		return
> > +	fi
> > +	rm -f "$out_file"
> > +
> > +	mv "$path" "$out_file"
> > +}
> > +
> >  init_rc
> >  
> >  ################################################################################
> > 
> 

Re: [PATCH 2/2] check: optionally compress core dumps

[Darrick J. Wong]

On Thu, Oct 13, 2022 at 07:51:02PM +0800, Zorro Lang wrote:
> On Tue, Oct 11, 2022 at 06:45:18PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Add a new option, COREDUMP_COMPRESSOR, that will be used to compress
> > core dumps collected during a fstests run.  The program specified must
> > accept the -f -9 arguments that gzip has.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  README    |    4 ++++
> >  common/rc |   14 +++++++++-----
> >  2 files changed, 13 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/README b/README
> > index 80d148be82..4c4f22f853 100644
> > --- a/README
> > +++ b/README
> > @@ -212,6 +212,10 @@ Tools specification:
> >      - Set FSSTRESS_AVOID and/or FSX_AVOID, which contain options added to
> >        the end of fsstresss and fsx invocations, respectively, in case you wish
> >        to exclude certain operational modes from these tests.
> > + - core dumps:
> > +    - Set COREDUMP_COMPRESSOR to a compression program to compress crash dumps.
> > +      This program must accept '-f' and the name of a file to compress.  In
> > +      other words, it must emulate gzip.
> >  
> >  Kernel/Modules related configuration:
> >   - Set TEST_FS_MODULE_RELOAD=1 to unload the module and reload it between
> > diff --git a/common/rc b/common/rc
> > index 152b8bb414..c68869b7dc 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -4956,13 +4956,17 @@ _save_coredump()
> >  	local core_hash="$(_md5_checksum "$path")"
> >  	local out_file="$RESULT_BASE/$seqnum.core.$core_hash"
> >  
> > -	if [ -s "$out_file" ]; then
> > -		rm -f "$path"
> > -		return
> > -	fi
> > -	rm -f "$out_file"
> > +	for dump in "$out_file"*; do
> > +		if [ -s "$dump" ]; then
> > +			rm -f "$path"
> > +			return 0
> > +		fi
> > +	done
> >  
> >  	mv "$path" "$out_file"
> > +	test -z "$COREDUMP_COMPRESSOR" && return 0
> > +
> > +	$COREDUMP_COMPRESSOR -f "$out_file"
> 
> This patch looks good to me,
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> I'm just not sure if all/most compressor supports "-f" option, I use bzip2
> and gzip mostly, they both support that.

As do xz, lz4, and zstd, so I think that's sufficient coverage.

The only one I know of that won't be compatible is zip, since it uses -f
for "freshen archive".

--D

> Thanks,
> Zorro
> 
> >  }
> >  
> >  init_rc
> > 
> 

Re: [PATCH v2.1 1/2] check: detect and preserve all coredumps made by a test

[Zorro Lang]

On Thu, Oct 13, 2022 at 08:48:41AM -0700, Darrick J. Wong wrote:
> On Thu, Oct 13, 2022 at 07:44:46PM +0800, Zorro Lang wrote:
> > On Wed, Oct 12, 2022 at 05:19:50PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > If someone sets kernel.core_uses_pid (or kernel.core_pattern), any
> > > coredumps generated by fstests might have names that are longer than
> > > just "core".  Since the pid isn't all that useful by itself, let's
> > > record the coredumps by hash when we save them, so that we don't waste
> > > space storing identical crash dumps.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > > v2.1: use REPORT_DIR per maintainer suggestion
> > > ---
> > 
> > This version looks good to me,
> > Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> It occurred to me overnight that ./check doesn't export REPORT_DIR, so
> I'll push out a v2.2 that adds that.  Currently the lack of an export
> doesn't affect anyone, but as soon as any tests want to call
> _save_coredump they're going to run into that issue.

Hmm... the RESULT_DIR is exported, you can use it, or use $seqres directly due
to it's defined in common/preamble (although is not exported).

./common/preamble:42:   seqres=$RESULT_DIR/$seq

What do you think?

Thanks,
Zorro

> 
> (...and yes, I do have future fuzz tests that will call it from a test
> in between fuzz field cycles.)
> 
> --D
> 
> > >  check     |   26 ++++++++++++++++++++++----
> > >  common/rc |   16 ++++++++++++++++
> > >  2 files changed, 38 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/check b/check
> > > index d587a70546..29303db1c8 100755
> > > --- a/check
> > > +++ b/check
> > > @@ -923,11 +923,19 @@ function run_section()
> > >  			sts=$?
> > >  		fi
> > >  
> > > -		if [ -f core ]; then
> > > -			_dump_err_cont "[dumped core]"
> > > -			mv core $RESULT_BASE/$seqnum.core
> > > +		# If someone sets kernel.core_pattern or kernel.core_uses_pid,
> > > +		# coredumps generated by fstests might have a longer name than
> > > +		# just "core".  Use globbing to find the most common patterns,
> > > +		# assuming there are no other coredump capture packages set up.
> > > +		local cores=0
> > > +		for i in core core.*; do
> > > +			test -f "$i" || continue
> > > +			if ((cores++ == 0)); then
> > > +				_dump_err_cont "[dumped core]"
> > > +			fi
> > > +			_save_coredump "$i"
> > >  			tc_status="fail"
> > > -		fi
> > > +		done
> > >  
> > >  		if [ -f $seqres.notrun ]; then
> > >  			$timestamp && _timestamp
> > > @@ -960,6 +968,16 @@ function run_section()
> > >  			# of the check script itself.
> > >  			(_adjust_oom_score 250; _check_filesystems) || tc_status="fail"
> > >  			_check_dmesg || tc_status="fail"
> > > +
> > > +			# Save any coredumps from the post-test fs checks
> > > +			for i in core core.*; do
> > > +				test -f "$i" || continue
> > > +				if ((cores++ == 0)); then
> > > +					_dump_err_cont "[dumped core]"
> > > +				fi
> > > +				_save_coredump "$i"
> > > +				tc_status="fail"
> > > +			done
> > >  		fi
> > >  
> > >  		# Reload the module after each test to check for leaks or
> > > diff --git a/common/rc b/common/rc
> > > index d877ac77a0..2e1891180a 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -4949,6 +4949,22 @@ _create_file_sized()
> > >  	return $ret
> > >  }
> > >  
> > > +_save_coredump()
> > > +{
> > > +	local path="$1"
> > > +
> > > +	local core_hash="$(_md5_checksum "$path")"
> > > +	local out_file="$REPORT_DIR/$seqnum.core.$core_hash"
> > > +
> > > +	if [ -s "$out_file" ]; then
> > > +		rm -f "$path"
> > > +		return
> > > +	fi
> > > +	rm -f "$out_file"
> > > +
> > > +	mv "$path" "$out_file"
> > > +}
> > > +
> > >  init_rc
> > >  
> > >  ################################################################################
> > > 
> > 
> 

Re: [PATCH v2.1 1/2] check: detect and preserve all coredumps made by a test

[Darrick J. Wong]

On Fri, Oct 14, 2022 at 12:03:26AM +0800, Zorro Lang wrote:
> On Thu, Oct 13, 2022 at 08:48:41AM -0700, Darrick J. Wong wrote:
> > On Thu, Oct 13, 2022 at 07:44:46PM +0800, Zorro Lang wrote:
> > > On Wed, Oct 12, 2022 at 05:19:50PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > If someone sets kernel.core_uses_pid (or kernel.core_pattern), any
> > > > coredumps generated by fstests might have names that are longer than
> > > > just "core".  Since the pid isn't all that useful by itself, let's
> > > > record the coredumps by hash when we save them, so that we don't waste
> > > > space storing identical crash dumps.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > > v2.1: use REPORT_DIR per maintainer suggestion
> > > > ---
> > > 
> > > This version looks good to me,
> > > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > 
> > It occurred to me overnight that ./check doesn't export REPORT_DIR, so
> > I'll push out a v2.2 that adds that.  Currently the lack of an export
> > doesn't affect anyone, but as soon as any tests want to call
> > _save_coredump they're going to run into that issue.
> 
> Hmm... the RESULT_DIR is exported, you can use it, or use $seqres directly due
> to it's defined in common/preamble (although is not exported).
> 
> ./common/preamble:42:   seqres=$RESULT_DIR/$seq
> 
> What do you think?

seqres is defined differently in check than in common/preamble, but I
guess RESULT_DIR will work.

--D

> Thanks,
> Zorro
> 
> > 
> > (...and yes, I do have future fuzz tests that will call it from a test
> > in between fuzz field cycles.)
> > 
> > --D
> > 
> > > >  check     |   26 ++++++++++++++++++++++----
> > > >  common/rc |   16 ++++++++++++++++
> > > >  2 files changed, 38 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/check b/check
> > > > index d587a70546..29303db1c8 100755
> > > > --- a/check
> > > > +++ b/check
> > > > @@ -923,11 +923,19 @@ function run_section()
> > > >  			sts=$?
> > > >  		fi
> > > >  
> > > > -		if [ -f core ]; then
> > > > -			_dump_err_cont "[dumped core]"
> > > > -			mv core $RESULT_BASE/$seqnum.core
> > > > +		# If someone sets kernel.core_pattern or kernel.core_uses_pid,
> > > > +		# coredumps generated by fstests might have a longer name than
> > > > +		# just "core".  Use globbing to find the most common patterns,
> > > > +		# assuming there are no other coredump capture packages set up.
> > > > +		local cores=0
> > > > +		for i in core core.*; do
> > > > +			test -f "$i" || continue
> > > > +			if ((cores++ == 0)); then
> > > > +				_dump_err_cont "[dumped core]"
> > > > +			fi
> > > > +			_save_coredump "$i"
> > > >  			tc_status="fail"
> > > > -		fi
> > > > +		done
> > > >  
> > > >  		if [ -f $seqres.notrun ]; then
> > > >  			$timestamp && _timestamp
> > > > @@ -960,6 +968,16 @@ function run_section()
> > > >  			# of the check script itself.
> > > >  			(_adjust_oom_score 250; _check_filesystems) || tc_status="fail"
> > > >  			_check_dmesg || tc_status="fail"
> > > > +
> > > > +			# Save any coredumps from the post-test fs checks
> > > > +			for i in core core.*; do
> > > > +				test -f "$i" || continue
> > > > +				if ((cores++ == 0)); then
> > > > +					_dump_err_cont "[dumped core]"
> > > > +				fi
> > > > +				_save_coredump "$i"
> > > > +				tc_status="fail"
> > > > +			done
> > > >  		fi
> > > >  
> > > >  		# Reload the module after each test to check for leaks or
> > > > diff --git a/common/rc b/common/rc
> > > > index d877ac77a0..2e1891180a 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -4949,6 +4949,22 @@ _create_file_sized()
> > > >  	return $ret
> > > >  }
> > > >  
> > > > +_save_coredump()
> > > > +{
> > > > +	local path="$1"
> > > > +
> > > > +	local core_hash="$(_md5_checksum "$path")"
> > > > +	local out_file="$REPORT_DIR/$seqnum.core.$core_hash"
> > > > +
> > > > +	if [ -s "$out_file" ]; then
> > > > +		rm -f "$path"
> > > > +		return
> > > > +	fi
> > > > +	rm -f "$out_file"
> > > > +
> > > > +	mv "$path" "$out_file"
> > > > +}
> > > > +
> > > >  init_rc
> > > >  
> > > >  ################################################################################
> > > > 
> > > 
> > 
> 

Re: [PATCH v2.1 1/2] check: detect and preserve all coredumps made by a test

[Darrick J. Wong]

On Thu, Oct 13, 2022 at 09:27:08AM -0700, Darrick J. Wong wrote:
> On Fri, Oct 14, 2022 at 12:03:26AM +0800, Zorro Lang wrote:
> > On Thu, Oct 13, 2022 at 08:48:41AM -0700, Darrick J. Wong wrote:
> > > On Thu, Oct 13, 2022 at 07:44:46PM +0800, Zorro Lang wrote:
> > > > On Wed, Oct 12, 2022 at 05:19:50PM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > > 
> > > > > If someone sets kernel.core_uses_pid (or kernel.core_pattern), any
> > > > > coredumps generated by fstests might have names that are longer than
> > > > > just "core".  Since the pid isn't all that useful by itself, let's
> > > > > record the coredumps by hash when we save them, so that we don't waste
> > > > > space storing identical crash dumps.
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > > ---
> > > > > v2.1: use REPORT_DIR per maintainer suggestion
> > > > > ---
> > > > 
> > > > This version looks good to me,
> > > > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > > 
> > > It occurred to me overnight that ./check doesn't export REPORT_DIR, so
> > > I'll push out a v2.2 that adds that.  Currently the lack of an export
> > > doesn't affect anyone, but as soon as any tests want to call
> > > _save_coredump they're going to run into that issue.
> > 
> > Hmm... the RESULT_DIR is exported, you can use it, or use $seqres directly due
> > to it's defined in common/preamble (although is not exported).
> > 
> > ./common/preamble:42:   seqres=$RESULT_DIR/$seq
> > 
> > What do you think?
> 
> seqres is defined differently in check than in common/preamble, but I
> guess RESULT_DIR will work.

Nope, it won't, because RESULT_DIR ends up getting set to something
like /var/tmp/fstests/xfs_moocow/xfs, seqnum gets set to xfs/350, and
then you end up with garbage paths like:

/var/tmp/fstests/xfs_moocow/xfs/xfs/350.core.XXX

Soooo you were right, I should have used seqres from the start.  The
multiple definitions are confusing, but they end up resolving to the
same pathnames(!) so it's all good.

--D

> --D
> 
> > Thanks,
> > Zorro
> > 
> > > 
> > > (...and yes, I do have future fuzz tests that will call it from a test
> > > in between fuzz field cycles.)
> > > 
> > > --D
> > > 
> > > > >  check     |   26 ++++++++++++++++++++++----
> > > > >  common/rc |   16 ++++++++++++++++
> > > > >  2 files changed, 38 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/check b/check
> > > > > index d587a70546..29303db1c8 100755
> > > > > --- a/check
> > > > > +++ b/check
> > > > > @@ -923,11 +923,19 @@ function run_section()
> > > > >  			sts=$?
> > > > >  		fi
> > > > >  
> > > > > -		if [ -f core ]; then
> > > > > -			_dump_err_cont "[dumped core]"
> > > > > -			mv core $RESULT_BASE/$seqnum.core
> > > > > +		# If someone sets kernel.core_pattern or kernel.core_uses_pid,
> > > > > +		# coredumps generated by fstests might have a longer name than
> > > > > +		# just "core".  Use globbing to find the most common patterns,
> > > > > +		# assuming there are no other coredump capture packages set up.
> > > > > +		local cores=0
> > > > > +		for i in core core.*; do
> > > > > +			test -f "$i" || continue
> > > > > +			if ((cores++ == 0)); then
> > > > > +				_dump_err_cont "[dumped core]"
> > > > > +			fi
> > > > > +			_save_coredump "$i"
> > > > >  			tc_status="fail"
> > > > > -		fi
> > > > > +		done
> > > > >  
> > > > >  		if [ -f $seqres.notrun ]; then
> > > > >  			$timestamp && _timestamp
> > > > > @@ -960,6 +968,16 @@ function run_section()
> > > > >  			# of the check script itself.
> > > > >  			(_adjust_oom_score 250; _check_filesystems) || tc_status="fail"
> > > > >  			_check_dmesg || tc_status="fail"
> > > > > +
> > > > > +			# Save any coredumps from the post-test fs checks
> > > > > +			for i in core core.*; do
> > > > > +				test -f "$i" || continue
> > > > > +				if ((cores++ == 0)); then
> > > > > +					_dump_err_cont "[dumped core]"
> > > > > +				fi
> > > > > +				_save_coredump "$i"
> > > > > +				tc_status="fail"
> > > > > +			done
> > > > >  		fi
> > > > >  
> > > > >  		# Reload the module after each test to check for leaks or
> > > > > diff --git a/common/rc b/common/rc
> > > > > index d877ac77a0..2e1891180a 100644
> > > > > --- a/common/rc
> > > > > +++ b/common/rc
> > > > > @@ -4949,6 +4949,22 @@ _create_file_sized()
> > > > >  	return $ret
> > > > >  }
> > > > >  
> > > > > +_save_coredump()
> > > > > +{
> > > > > +	local path="$1"
> > > > > +
> > > > > +	local core_hash="$(_md5_checksum "$path")"
> > > > > +	local out_file="$REPORT_DIR/$seqnum.core.$core_hash"
> > > > > +
> > > > > +	if [ -s "$out_file" ]; then
> > > > > +		rm -f "$path"
> > > > > +		return
> > > > > +	fi
> > > > > +	rm -f "$out_file"
> > > > > +
> > > > > +	mv "$path" "$out_file"
> > > > > +}
> > > > > +
> > > > >  init_rc
> > > > >  
> > > > >  ################################################################################
> > > > > 
> > > > 
> > > 
> > 

[PATCH v2.2 1/2] check: detect and preserve all coredumps made by a test

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If someone sets kernel.core_uses_pid (or kernel.core_pattern), any
coredumps generated by fstests might have names that are longer than
just "core".  Since the pid isn't all that useful by itself, let's
record the coredumps by hash when we save them, so that we don't waste
space storing identical crash dumps.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Zorro Lang <zlang@redhat.com>
---
v2.1: use REPORT_DIR per maintainer suggestion
v2.2: use seqres, not REPORT_DIR, since REPORT_DIR doesnt work
---
 check     |   26 ++++++++++++++++++++++----
 common/rc |   22 ++++++++++++++++++++++
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/check b/check
index 3b7bb84ca2..9bebd2a1b9 100755
--- a/check
+++ b/check
@@ -923,11 +923,19 @@ function run_section()
 			sts=$?
 		fi
 
-		if [ -f core ]; then
-			_dump_err_cont "[dumped core]"
-			mv core $RESULT_BASE/$seqnum.core
+		# If someone sets kernel.core_pattern or kernel.core_uses_pid,
+		# coredumps generated by fstests might have a longer name than
+		# just "core".  Use globbing to find the most common patterns,
+		# assuming there are no other coredump capture packages set up.
+		local cores=0
+		for i in core core.*; do
+			test -f "$i" || continue
+			if ((cores++ == 0)); then
+				_dump_err_cont "[dumped core]"
+			fi
+			(_adjust_oom_score 250; _save_coredump "$i")
 			tc_status="fail"
-		fi
+		done
 
 		if [ -f $seqres.notrun ]; then
 			$timestamp && _timestamp
@@ -960,6 +968,16 @@ function run_section()
 			# of the check script itself.
 			(_adjust_oom_score 250; _check_filesystems) || tc_status="fail"
 			_check_dmesg || tc_status="fail"
+
+			# Save any coredumps from the post-test fs checks
+			for i in core core.*; do
+				test -f "$i" || continue
+				if ((cores++ == 0)); then
+					_dump_err_cont "[dumped core]"
+				fi
+				(_adjust_oom_score 250; _save_coredump "$i")
+				tc_status="fail"
+			done
 		fi
 
 		# Reload the module after each test to check for leaks or
diff --git a/common/rc b/common/rc
index d877ac77a0..1a0f167f84 100644
--- a/common/rc
+++ b/common/rc
@@ -4949,6 +4949,28 @@ _create_file_sized()
 	return $ret
 }
 
+# Save an arbitrary coredump to the report directory.
+_save_coredump()
+{
+	local path="$1"
+
+	if [ -z "$seqres" ]; then
+		echo "$path: seqres is not defined; ignoring coredump!"
+		return 1
+	fi
+
+	local core_hash="$(_md5_checksum "$path")"
+	local out_file="${seqres}.core.${core_hash}"
+
+	if [ -s "$out_file" ]; then
+		rm -f "$path"
+		return
+	fi
+	rm -f "$out_file"
+
+	mv "$path" "$out_file"
+}
+
 init_rc
 
 ################################################################################