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

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

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    |    1 +
 check     |   26 ++++++++++++++++++++++----
 common/rc |   29 +++++++++++++++++++++++++++++
 3 files changed, 52 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 d1f3d56bf8..9750d06a9a 100644
--- a/common/rc
+++ b/common/rc
@@ -4948,6 +4948,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>

Compress coredumps whenever desired to save space.

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


diff --git a/README b/README
index 80d148be82..ec923ca564 100644
--- a/README
+++ b/README
@@ -241,6 +241,7 @@ Misc:
    this option is supported for all filesystems currently only -overlay is
    expected to run without issues. For other filesystems additional patches
    and fixes to the test suite might be needed.
+ - Set COMPRESS_COREDUMPS=1 to compress core dumps with gzip -9.
 
 ______________________
 USING THE FSQA SUITE
diff --git a/common/rc b/common/rc
index 9750d06a9a..d3af4e07b2 100644
--- a/common/rc
+++ b/common/rc
@@ -4955,12 +4955,25 @@ _save_coredump()
 	local core_hash="$(_md5_checksum "$path")"
 	local out_file="$RESULT_BASE/$seqnum.core.$core_hash"
 
+	if [ "$COMPRESS_COREDUMPS" = "1" ]; then
+		out_file="${out_file}.gz"
+	fi
+
 	if [ -s "$out_file" ]; then
 		rm -f "$path"
 		return
 	fi
 	rm -f "$out_file"
 
+	if [ "$COMPRESS_COREDUMPS" = "1" ]; then
+		if gzip -9 < "$path" > "$out_file"; then
+			rm -f "$path"
+		else
+			rm -f "$out_file"
+		fi
+		return
+	fi
+
 	mv "$path" "$out_file"
 }
 

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

[Zorro Lang]

On Wed, Oct 05, 2022 at 03:31:15PM -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

I'm wondering if it should be "for i in core*" ? The coredump file only can be
"core" with dot ".", can it with "-" or "_" or others?


> +			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 d1f3d56bf8..9750d06a9a 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4948,6 +4948,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
>  
>  ################################################################################
> 

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

[Zorro Lang]

On Wed, Oct 05, 2022 at 03:31:21PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Compress coredumps whenever desired to save space.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  README    |    1 +
>  common/rc |   13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> 
> diff --git a/README b/README
> index 80d148be82..ec923ca564 100644
> --- a/README
> +++ b/README
> @@ -241,6 +241,7 @@ Misc:
>     this option is supported for all filesystems currently only -overlay is
>     expected to run without issues. For other filesystems additional patches
>     and fixes to the test suite might be needed.
> + - Set COMPRESS_COREDUMPS=1 to compress core dumps with gzip -9.

This patch looks good to me, just one question I'm thinking -- should this
parameter be under "Misc:" or "Tools specification:" part? If the former is
good, then:

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


>  
>  ______________________
>  USING THE FSQA SUITE
> diff --git a/common/rc b/common/rc
> index 9750d06a9a..d3af4e07b2 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4955,12 +4955,25 @@ _save_coredump()
>  	local core_hash="$(_md5_checksum "$path")"
>  	local out_file="$RESULT_BASE/$seqnum.core.$core_hash"
>  
> +	if [ "$COMPRESS_COREDUMPS" = "1" ]; then
> +		out_file="${out_file}.gz"
> +	fi
> +
>  	if [ -s "$out_file" ]; then
>  		rm -f "$path"
>  		return
>  	fi
>  	rm -f "$out_file"
>  
> +	if [ "$COMPRESS_COREDUMPS" = "1" ]; then
> +		if gzip -9 < "$path" > "$out_file"; then
> +			rm -f "$path"
> +		else
> +			rm -f "$out_file"
> +		fi
> +		return
> +	fi
> +
>  	mv "$path" "$out_file"
>  }
>  
> 

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

[Darrick J. Wong]

On Fri, Oct 07, 2022 at 01:18:55PM +0800, Zorro Lang wrote:
> On Wed, Oct 05, 2022 at 03:31:15PM -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
> 
> I'm wondering if it should be "for i in core*" ? The coredump file only can be
> "core" with dot ".", can it with "-" or "_" or others?

The ".$pid" pattern is encoded in the kernel function format_corename:

	/* Backward compatibility with core_uses_pid:
	 *
	 * If core_pattern does not include a %p (as is the default)
	 * and core_uses_pid is set, then .%pid will be appended to
	 * the filename. Do not do this for piped commands. */
	if (!ispipe && !pid_in_pattern && core_uses_pid) {
		err = cn_printf(cn, ".%d", task_tgid_vnr(current));
		if (err)
			return err;
	}

Note that even this is an incomplete solution because the sysadmin could
configure a totally different corename pattern, pipe it to another
program, or whatever, and fstests fails to capture any dumps at all.

Longer term it'd be theoretically nice to turn core_pattern into a
cgroup-manageable tunable and teach ./check to capture all the
coredumps, but I am not volunteering to write that much new
infrastructure. :/

--D

> 
> > +			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 d1f3d56bf8..9750d06a9a 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -4948,6 +4948,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
> >  
> >  ################################################################################
> > 
> 

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

[Darrick J. Wong]

On Fri, Oct 07, 2022 at 08:45:26PM +0800, Zorro Lang wrote:
> On Wed, Oct 05, 2022 at 03:31:21PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Compress coredumps whenever desired to save space.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  README    |    1 +
> >  common/rc |   13 +++++++++++++
> >  2 files changed, 14 insertions(+)
> > 
> > 
> > diff --git a/README b/README
> > index 80d148be82..ec923ca564 100644
> > --- a/README
> > +++ b/README
> > @@ -241,6 +241,7 @@ Misc:
> >     this option is supported for all filesystems currently only -overlay is
> >     expected to run without issues. For other filesystems additional patches
> >     and fixes to the test suite might be needed.
> > + - Set COMPRESS_COREDUMPS=1 to compress core dumps with gzip -9.
> 
> This patch looks good to me, just one question I'm thinking -- should this
> parameter be under "Misc:" or "Tools specification:" part? If the former is
> good, then:

I was thinking misc, buuut it occurs to me that perhaps we ought to let
people specify a different compression program, e.g.

COMPRESS_COREDUMPS=xz ./check generic/444

in which case this would be a tool spec thing.

I think I'll go back and rework this to do that.

--D

> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> 
> >  
> >  ______________________
> >  USING THE FSQA SUITE
> > diff --git a/common/rc b/common/rc
> > index 9750d06a9a..d3af4e07b2 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -4955,12 +4955,25 @@ _save_coredump()
> >  	local core_hash="$(_md5_checksum "$path")"
> >  	local out_file="$RESULT_BASE/$seqnum.core.$core_hash"
> >  
> > +	if [ "$COMPRESS_COREDUMPS" = "1" ]; then
> > +		out_file="${out_file}.gz"
> > +	fi
> > +
> >  	if [ -s "$out_file" ]; then
> >  		rm -f "$path"
> >  		return
> >  	fi
> >  	rm -f "$out_file"
> >  
> > +	if [ "$COMPRESS_COREDUMPS" = "1" ]; then
> > +		if gzip -9 < "$path" > "$out_file"; then
> > +			rm -f "$path"
> > +		else
> > +			rm -f "$out_file"
> > +		fi
> > +		return
> > +	fi
> > +
> >  	mv "$path" "$out_file"
> >  }
> >  
> > 
>