[PATCH 0/3] btrfs-progs: add eb leak detection and fixes

[PATCH 0/3] btrfs-progs: add eb leak detection and fixes

[Josef Bacik]

Hello,

I introduced an EB leak that we only discovered when we started running fstests
with my code applied to the btrfs-progs devel branch.  We really want to detect
this before we start using this code for fstests, so update all the run_check
related helpers to use a helper that will check for extent buffer leaks and fail
accordingly.  This will allow developers to discover they've introduced a
problem when they run make test after their changes.

This functionality of course uncovered a few leaks that currently exist in
btrfs-progs, so there are two fixes that precede the leak detection work in
order to make sure we are clean from the leak detection commit ondwards.
Thanks,

Josef

Josef Bacik (3):
  btrfs-progs: cleanup dirty buffers on transaction abort
  btrfs-progs: properly cleanup aborted transactions in check
  btrfs-progs: add extent buffer leak detection to make test

 check/main.c                |  10 +++-
 kernel-shared/transaction.c |  45 ++++++++-------
 tests/common                | 108 ++++++++++++++++++++----------------
 3 files changed, 95 insertions(+), 68 deletions(-)

-- 
2.41.0

[PATCH 1/3] btrfs-progs: cleanup dirty buffers on transaction abort

[Josef Bacik]

When adding the extent buffer leak detection I started getting failures
on some of the fuzz tests.  This is because we don't clean up dirty
buffers for aborted transactions, we just leave them dirty and thus we
leak them.  Fix this up by making btrfs_commit_transaction() on an
aborted transaction properly cleanup the dirty buffers that exist in the
system.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 kernel-shared/transaction.c | 45 +++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/kernel-shared/transaction.c b/kernel-shared/transaction.c
index fcd8e6e0..01f08f0f 100644
--- a/kernel-shared/transaction.c
+++ b/kernel-shared/transaction.c
@@ -132,6 +132,25 @@ int commit_tree_roots(struct btrfs_trans_handle *trans,
 	return 0;
 }
 
+static void clean_dirty_buffers(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct extent_io_tree *tree = &fs_info->dirty_buffers;
+	struct extent_buffer *eb;
+	u64 start, end;
+
+	while (find_first_extent_bit(tree, 0, &start, &end, EXTENT_DIRTY,
+				     NULL) == 0) {
+		while (start <= end) {
+			eb = find_first_extent_buffer(fs_info, start);
+			BUG_ON(!eb || eb->start != start);
+			start += eb->len;
+			btrfs_clear_buffer_dirty(trans, eb);
+			free_extent_buffer(eb);
+		}
+	}
+}
+
 int __commit_transaction(struct btrfs_trans_handle *trans,
 				struct btrfs_root *root)
 {
@@ -174,23 +193,7 @@ cleanup:
 	 * Mark all remaining dirty ebs clean, as they have no chance to be written
 	 * back anymore.
 	 */
-	while (1) {
-		int find_ret;
-
-		find_ret = find_first_extent_bit(tree, 0, &start, &end,
-						 EXTENT_DIRTY, NULL);
-
-		if (find_ret)
-			break;
-
-		while (start <= end) {
-			eb = find_first_extent_buffer(fs_info, start);
-			BUG_ON(!eb || eb->start != start);
-			start += eb->len;
-			btrfs_clear_buffer_dirty(trans, eb);
-			free_extent_buffer(eb);
-		}
-	}
+	clean_dirty_buffers(trans);
 	return ret;
 }
 
@@ -202,8 +205,11 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_space_info *sinfo;
 
-	if (trans->fs_info->transaction_aborted)
-		return -EROFS;
+	if (trans->fs_info->transaction_aborted) {
+		ret = -EROFS;
+		goto error;
+	}
+
 	/*
 	 * Flush all accumulated delayed refs so that root-tree updates are
 	 * consistent
@@ -277,6 +283,7 @@ commit_tree:
 	return ret;
 error:
 	btrfs_abort_transaction(trans, ret);
+	clean_dirty_buffers(trans);
 	btrfs_destroy_delayed_refs(trans);
 	free(trans);
 	return ret;
-- 
2.41.0

[PATCH 2/3] btrfs-progs: properly cleanup aborted transactions in check

[Josef Bacik]

There are several places that we call btrfs_abort_transaction() in a
failure case, but never call btrfs_commit_transaction().  This leaks the
trans handle and the associated extent buffers and such.  Fix all these
sites by making sure we call btrfs_commit_transaction() after we call
btrfs_abort_transaction() to make sure all the appropriate cleanup is
done.  This gets rid of the leaked extent buffer errors.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 check/main.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/check/main.c b/check/main.c
index c99092a2..1d5f570a 100644
--- a/check/main.c
+++ b/check/main.c
@@ -3114,6 +3114,7 @@ static int check_inode_recs(struct btrfs_root *root,
 			ret = btrfs_make_root_dir(trans, root, root_dirid);
 			if (ret < 0) {
 				btrfs_abort_transaction(trans, ret);
+				btrfs_commit_transaction(trans, root);
 				return ret;
 			}
 
@@ -8011,8 +8012,10 @@ static int repair_extent_item_generation(struct extent_record *rec)
 	rec->generation = new_gen;
 out:
 	btrfs_release_path(&path);
-	if (ret < 0)
+	if (ret < 0) {
 		btrfs_abort_transaction(trans, ret);
+		btrfs_commit_transaction(trans, extent_root);
+	}
 	return ret;
 }
 
@@ -8223,8 +8226,11 @@ repair_abort:
 			}
 
 			ret = btrfs_fix_block_accounting(trans);
-			if (ret)
+			if (ret) {
+				btrfs_abort_transaction(trans, ret);
+				btrfs_commit_transaction(trans, root);
 				goto repair_abort;
+			}
 			ret = btrfs_commit_transaction(trans, root);
 			if (ret)
 				goto repair_abort;
-- 
2.41.0

[PATCH 3/3] btrfs-progs: add extent buffer leak detection to make test

[Josef Bacik]

I introduced a regression where we were leaking extent buffers, and this
resulted in the CI failing because we were spewing these errors.

Instead of waiting for fstests to catch my mistakes, check every command
output for leak messages, and fail the test if we detect any of these
messages.  I've made this generic enough that we could check for other
debug messages in the future.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 tests/common | 108 +++++++++++++++++++++++++++++----------------------
 1 file changed, 61 insertions(+), 47 deletions(-)

diff --git a/tests/common b/tests/common
index 602a4122..607ad747 100644
--- a/tests/common
+++ b/tests/common
@@ -160,6 +160,18 @@ _is_target_command()
 	return 1
 }
 
+# Check to see if there's any debug messages that may mean we have a problem.
+_check_output()
+{
+	local results="$1"
+
+	if grep -q "extent buffer leak" "$results"; then
+		_fail "extent buffer leak reported"
+		return 1
+	fi
+	return 0
+}
+
 # Expanding extra commands/options for current command string
 # This function is responsible for inserting the following things:
 # - @INSTRUMENT before 'btrfs'  commands
@@ -206,6 +218,48 @@ expand_command()
 	done
 }
 
+# This is the helper for the run_check variants.
+# The first argument is the run_check type
+# The second argument is the run_check type that will get logged to tty
+# The third argument is wether we want the output echo'ed
+# The rest of the arguments are the command
+_run_check()
+{
+	local header_type
+	local test_log_type
+	local do_stdout
+	local tmp_output
+
+	run_type="$1"
+	shift
+
+	test_log_type="$1"
+	shift
+
+	do_stdout="$1"
+	shift
+
+	tmp_output=$(mktemp --tmpdir btrfs-progs-leak-detect.XXXXXX)
+
+	expand_command "$@"
+	echo "====== RUN $run_type ${cmd_array[@]}" >> "$RESULTS" 2>&1
+	if [[ $TEST_LOG =~ tty ]]; then echo "$test_log_type: ${cmd_array[@]}" \
+		> /dev/tty; fi
+	"${cmd_array[@]}" > "$tmp_output" 2>&1
+	ret=$?
+
+	cat "$tmp_output" >> "$RESULTS"
+	[ "$do_stdout" = true ] && cat "$tmp_output"
+
+	if ! _check_output "$tmp_output"; then
+		_fail "bad output"
+		rm "$tmp_output"
+		return 1
+	fi
+	rm "$tmp_output"
+	return "$ret"
+}
+
 # Argument passing magic:
 # the command passed to run_* helpers is inspected, if there's 'btrfs command'
 # found and there are defined additional arguments, they're inserted just after
@@ -216,11 +270,7 @@ expand_command()
 
 run_check()
 {
-	expand_command "$@"
-	echo "====== RUN CHECK ${cmd_array[@]}" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD: ${cmd_array[@]}" > /dev/tty; fi
-
-	"${cmd_array[@]}" >> "$RESULTS" 2>&1 || _fail "failed: ${cmd_array[@]}"
+	_run_check "CHECK" "CMD" "false" "$@" || _fail "failed: ${cmd_array[@]}"
 }
 
 # same as run_check but the stderr+stdout output is duplicated on stdout and
@@ -230,12 +280,8 @@ run_check()
 #	filter the output, as INSTRUMENT can easily pollute the output.
 run_check_stdout()
 {
-	expand_command "$@"
-	echo "====== RUN CHECK ${cmd_array[@]}" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(stdout): ${cmd_array[@]}" \
-		> /dev/tty; fi
-	"${cmd_array[@]}" 2>&1 | tee -a "$RESULTS"
-	if [ ${PIPESTATUS[0]} -ne 0 ]; then
+	_run_check "CHECK" "CMD(stdout)" "true" "$@"
+	if [ $? -ne 0 ]; then
 		_fail "failed: $@"
 	fi
 }
@@ -245,11 +291,7 @@ run_check_stdout()
 # output is logged
 run_mayfail()
 {
-	expand_command "$@"
-	echo "====== RUN MAYFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mayfail): ${cmd_array[@]}" \
-		> /dev/tty; fi
-	"${cmd_array[@]}" >> "$RESULTS" 2>&1
+	_run_check "MAYFAIL" "CMD(mayfail)" "false" "$@"
 	ret=$?
 	if [ $ret != 0 ]; then
 		echo "failed (ignored, ret=$ret): $@" >> "$RESULTS"
@@ -271,19 +313,8 @@ run_mayfail()
 # store the output to a variable for further processing.
 run_mayfail_stdout()
 {
-	tmp_output=$(mktemp --tmpdir btrfs-progs-mayfail-stdout.XXXXXX)
-
-	expand_command "$@"
-	echo "====== RUN MAYFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mayfail): ${cmd_array[@]}" \
-		> /dev/tty; fi
-	"${cmd_array[@]}" 2>&1 > "$tmp_output"
+	_run_check "MAYFAIL" "CMD(mayfail)" "true" "$@"
 	ret=$?
-
-	cat "$tmp_output" >> "$RESULTS"
-	cat "$tmp_output"
-	rm -- "$tmp_output"
-
 	if [ "$ret" != 0 ]; then
 		echo "failed (ignored, ret=$ret): $@" >> "$RESULTS"
 		if [ "$ret" == 139 ]; then
@@ -312,12 +343,7 @@ run_mustfail()
 		exit 1
 	fi
 
-
-	expand_command "$@"
-	echo "====== RUN MUSTFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mustfail): ${cmd_array[@]}" \
-		> /dev/tty; fi
-	"${cmd_array[@]}" >> "$RESULTS" 2>&1
+	_run_check "MUSTFAIL" "CMD(mustfail)" "false" "$@"
 	if [ $? != 0 ]; then
 		echo "failed (expected): $@" >> "$RESULTS"
 		return 0
@@ -337,9 +363,6 @@ run_mustfail_stdout()
 {
 	local msg
 	local ret
-	local tmp_output
-
-	tmp_output=$(mktemp --tmpdir btrfs-progs-mustfail-stdout.XXXXXX)
 
 	msg="$1"
 	shift
@@ -349,17 +372,8 @@ run_mustfail_stdout()
 		exit 1
 	fi
 
-	expand_command "$@"
-	echo "====== RUN MUSTFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mustfail): ${cmd_array[@]}" \
-		> /dev/tty; fi
-	"${cmd_array[@]}" 2>&1 > "$tmp_output"
+	_run_check "MUSTFAIL" "CMD(mustfail)" "true" "$@"
 	ret=$?
-
-	cat "$tmp_output" >> "$RESULTS"
-	cat "$tmp_output"
-	rm "$tmp_output"
-
 	if [ "$ret" != 0 ]; then
 		echo "failed (expected): $@" >> "$RESULTS"
 		return 0
-- 
2.41.0

Re: [PATCH 3/3] btrfs-progs: add extent buffer leak detection to make test

[David Sterba]

On Tue, Sep 05, 2023 at 04:21:53PM -0400, Josef Bacik wrote:
> I introduced a regression where we were leaking extent buffers, and this
> resulted in the CI failing because we were spewing these errors.
> 
> Instead of waiting for fstests to catch my mistakes, check every command
> output for leak messages, and fail the test if we detect any of these
> messages.  I've made this generic enough that we could check for other
> debug messages in the future.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

There already is a script to verify more than just the error leaks, I've
added it to all the test type running scripts so it would fail.
> ---
>  tests/common | 108 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 61 insertions(+), 47 deletions(-)
> 
> diff --git a/tests/common b/tests/common
> index 602a4122..607ad747 100644
> --- a/tests/common
> +++ b/tests/common
> @@ -160,6 +160,18 @@ _is_target_command()
>  	return 1
>  }
>  
> +# Check to see if there's any debug messages that may mean we have a problem.
> +_check_output()
> +{
> +	local results="$1"
> +
> +	if grep -q "extent buffer leak" "$results"; then
> +		_fail "extent buffer leak reported"

There's more than that we'd like to catch, see tests/scan-results.sh.

> +		return 1
> +	fi
> +	return 0
> +}
> +
>  # Expanding extra commands/options for current command string
>  # This function is responsible for inserting the following things:
>  # - @INSTRUMENT before 'btrfs'  commands

Re: [PATCH 1/3] btrfs-progs: cleanup dirty buffers on transaction abort

[Qu Wenruo]

On 2023/9/6 04:21, Josef Bacik wrote:
> When adding the extent buffer leak detection I started getting failures
> on some of the fuzz tests.  This is because we don't clean up dirty
> buffers for aborted transactions, we just leave them dirty and thus we
> leak them.  Fix this up by making btrfs_commit_transaction() on an
> aborted transaction properly cleanup the dirty buffers that exist in the
> system.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Shouldn't we call the new clean_dirty_buffers() inside
btrfs_abort_transaction()?

Thanks,
Qu

> ---
>   kernel-shared/transaction.c | 45 +++++++++++++++++++++----------------
>   1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/kernel-shared/transaction.c b/kernel-shared/transaction.c
> index fcd8e6e0..01f08f0f 100644
> --- a/kernel-shared/transaction.c
> +++ b/kernel-shared/transaction.c
> @@ -132,6 +132,25 @@ int commit_tree_roots(struct btrfs_trans_handle *trans,
>   	return 0;
>   }
>
> +static void clean_dirty_buffers(struct btrfs_trans_handle *trans)
> +{
> +	struct btrfs_fs_info *fs_info = trans->fs_info;
> +	struct extent_io_tree *tree = &fs_info->dirty_buffers;
> +	struct extent_buffer *eb;
> +	u64 start, end;
> +
> +	while (find_first_extent_bit(tree, 0, &start, &end, EXTENT_DIRTY,
> +				     NULL) == 0) {
> +		while (start <= end) {
> +			eb = find_first_extent_buffer(fs_info, start);
> +			BUG_ON(!eb || eb->start != start);
> +			start += eb->len;
> +			btrfs_clear_buffer_dirty(trans, eb);
> +			free_extent_buffer(eb);
> +		}
> +	}
> +}
> +
>   int __commit_transaction(struct btrfs_trans_handle *trans,
>   				struct btrfs_root *root)
>   {
> @@ -174,23 +193,7 @@ cleanup:
>   	 * Mark all remaining dirty ebs clean, as they have no chance to be written
>   	 * back anymore.
>   	 */
> -	while (1) {
> -		int find_ret;
> -
> -		find_ret = find_first_extent_bit(tree, 0, &start, &end,
> -						 EXTENT_DIRTY, NULL);
> -
> -		if (find_ret)
> -			break;
> -
> -		while (start <= end) {
> -			eb = find_first_extent_buffer(fs_info, start);
> -			BUG_ON(!eb || eb->start != start);
> -			start += eb->len;
> -			btrfs_clear_buffer_dirty(trans, eb);
> -			free_extent_buffer(eb);
> -		}
> -	}
> +	clean_dirty_buffers(trans);
>   	return ret;
>   }
>
> @@ -202,8 +205,11 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>   	struct btrfs_fs_info *fs_info = root->fs_info;
>   	struct btrfs_space_info *sinfo;
>
> -	if (trans->fs_info->transaction_aborted)
> -		return -EROFS;
> +	if (trans->fs_info->transaction_aborted) {
> +		ret = -EROFS;
> +		goto error;
> +	}
> +
>   	/*
>   	 * Flush all accumulated delayed refs so that root-tree updates are
>   	 * consistent
> @@ -277,6 +283,7 @@ commit_tree:
>   	return ret;
>   error:
>   	btrfs_abort_transaction(trans, ret);
> +	clean_dirty_buffers(trans);
>   	btrfs_destroy_delayed_refs(trans);
>   	free(trans);
>   	return ret;

Re: [PATCH 2/3] btrfs-progs: properly cleanup aborted transactions in check

[Qu Wenruo]

On 2023/9/6 04:21, Josef Bacik wrote:
> There are several places that we call btrfs_abort_transaction() in a
> failure case, but never call btrfs_commit_transaction().  This leaks the
> trans handle and the associated extent buffers and such.  Fix all these
> sites by making sure we call btrfs_commit_transaction() after we call
> btrfs_abort_transaction() to make sure all the appropriate cleanup is
> done.  This gets rid of the leaked extent buffer errors.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Although I'd say wouldn't it be better to make btrfs_abort_transaction()
more standalone?

It's pretty instinctive to think btrfs_abort_transaction() should handle
everything.

Thanks,
Qu
> ---
>   check/main.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/check/main.c b/check/main.c
> index c99092a2..1d5f570a 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -3114,6 +3114,7 @@ static int check_inode_recs(struct btrfs_root *root,
>   			ret = btrfs_make_root_dir(trans, root, root_dirid);
>   			if (ret < 0) {
>   				btrfs_abort_transaction(trans, ret);
> +				btrfs_commit_transaction(trans, root);
>   				return ret;
>   			}
>
> @@ -8011,8 +8012,10 @@ static int repair_extent_item_generation(struct extent_record *rec)
>   	rec->generation = new_gen;
>   out:
>   	btrfs_release_path(&path);
> -	if (ret < 0)
> +	if (ret < 0) {
>   		btrfs_abort_transaction(trans, ret);
> +		btrfs_commit_transaction(trans, extent_root);
> +	}
>   	return ret;
>   }
>
> @@ -8223,8 +8226,11 @@ repair_abort:
>   			}
>
>   			ret = btrfs_fix_block_accounting(trans);
> -			if (ret)
> +			if (ret) {
> +				btrfs_abort_transaction(trans, ret);
> +				btrfs_commit_transaction(trans, root);
>   				goto repair_abort;
> +			}
>   			ret = btrfs_commit_transaction(trans, root);
>   			if (ret)
>   				goto repair_abort;

Re: [PATCH 3/3] btrfs-progs: add extent buffer leak detection to make test

[Qu Wenruo]

On 2023/9/6 04:21, Josef Bacik wrote:
> I introduced a regression where we were leaking extent buffers, and this
> resulted in the CI failing because we were spewing these errors.
>
> Instead of waiting for fstests to catch my mistakes, check every command
> output for leak messages, and fail the test if we detect any of these
> messages.  I've made this generic enough that we could check for other
> debug messages in the future.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Another solution is to make debug build of btrfs-progs more noisy when
eb leak is detected.

Instead of a graceful exit (which is suitable for release build), a
noisy BUG_ON()/ASSERT() would definitely catch our attention, and
requires less work in the test framework.

Thanks,
Qu

> ---
>   tests/common | 108 +++++++++++++++++++++++++++++----------------------
>   1 file changed, 61 insertions(+), 47 deletions(-)
>
> diff --git a/tests/common b/tests/common
> index 602a4122..607ad747 100644
> --- a/tests/common
> +++ b/tests/common
> @@ -160,6 +160,18 @@ _is_target_command()
>   	return 1
>   }
>
> +# Check to see if there's any debug messages that may mean we have a problem.
> +_check_output()
> +{
> +	local results="$1"
> +
> +	if grep -q "extent buffer leak" "$results"; then
> +		_fail "extent buffer leak reported"
> +		return 1
> +	fi
> +	return 0
> +}
> +
>   # Expanding extra commands/options for current command string
>   # This function is responsible for inserting the following things:
>   # - @INSTRUMENT before 'btrfs'  commands
> @@ -206,6 +218,48 @@ expand_command()
>   	done
>   }
>
> +# This is the helper for the run_check variants.
> +# The first argument is the run_check type
> +# The second argument is the run_check type that will get logged to tty
> +# The third argument is wether we want the output echo'ed
> +# The rest of the arguments are the command
> +_run_check()
> +{
> +	local header_type
> +	local test_log_type
> +	local do_stdout
> +	local tmp_output
> +
> +	run_type="$1"
> +	shift
> +
> +	test_log_type="$1"
> +	shift
> +
> +	do_stdout="$1"
> +	shift
> +
> +	tmp_output=$(mktemp --tmpdir btrfs-progs-leak-detect.XXXXXX)
> +
> +	expand_command "$@"
> +	echo "====== RUN $run_type ${cmd_array[@]}" >> "$RESULTS" 2>&1
> +	if [[ $TEST_LOG =~ tty ]]; then echo "$test_log_type: ${cmd_array[@]}" \
> +		> /dev/tty; fi
> +	"${cmd_array[@]}" > "$tmp_output" 2>&1
> +	ret=$?
> +
> +	cat "$tmp_output" >> "$RESULTS"
> +	[ "$do_stdout" = true ] && cat "$tmp_output"
> +
> +	if ! _check_output "$tmp_output"; then
> +		_fail "bad output"
> +		rm "$tmp_output"
> +		return 1
> +	fi
> +	rm "$tmp_output"
> +	return "$ret"
> +}
> +
>   # Argument passing magic:
>   # the command passed to run_* helpers is inspected, if there's 'btrfs command'
>   # found and there are defined additional arguments, they're inserted just after
> @@ -216,11 +270,7 @@ expand_command()
>
>   run_check()
>   {
> -	expand_command "$@"
> -	echo "====== RUN CHECK ${cmd_array[@]}" >> "$RESULTS" 2>&1
> -	if [[ $TEST_LOG =~ tty ]]; then echo "CMD: ${cmd_array[@]}" > /dev/tty; fi
> -
> -	"${cmd_array[@]}" >> "$RESULTS" 2>&1 || _fail "failed: ${cmd_array[@]}"
> +	_run_check "CHECK" "CMD" "false" "$@" || _fail "failed: ${cmd_array[@]}"
>   }
>
>   # same as run_check but the stderr+stdout output is duplicated on stdout and
> @@ -230,12 +280,8 @@ run_check()
>   #	filter the output, as INSTRUMENT can easily pollute the output.
>   run_check_stdout()
>   {
> -	expand_command "$@"
> -	echo "====== RUN CHECK ${cmd_array[@]}" >> "$RESULTS" 2>&1
> -	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(stdout): ${cmd_array[@]}" \
> -		> /dev/tty; fi
> -	"${cmd_array[@]}" 2>&1 | tee -a "$RESULTS"
> -	if [ ${PIPESTATUS[0]} -ne 0 ]; then
> +	_run_check "CHECK" "CMD(stdout)" "true" "$@"
> +	if [ $? -ne 0 ]; then
>   		_fail "failed: $@"
>   	fi
>   }
> @@ -245,11 +291,7 @@ run_check_stdout()
>   # output is logged
>   run_mayfail()
>   {
> -	expand_command "$@"
> -	echo "====== RUN MAYFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
> -	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mayfail): ${cmd_array[@]}" \
> -		> /dev/tty; fi
> -	"${cmd_array[@]}" >> "$RESULTS" 2>&1
> +	_run_check "MAYFAIL" "CMD(mayfail)" "false" "$@"
>   	ret=$?
>   	if [ $ret != 0 ]; then
>   		echo "failed (ignored, ret=$ret): $@" >> "$RESULTS"
> @@ -271,19 +313,8 @@ run_mayfail()
>   # store the output to a variable for further processing.
>   run_mayfail_stdout()
>   {
> -	tmp_output=$(mktemp --tmpdir btrfs-progs-mayfail-stdout.XXXXXX)
> -
> -	expand_command "$@"
> -	echo "====== RUN MAYFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
> -	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mayfail): ${cmd_array[@]}" \
> -		> /dev/tty; fi
> -	"${cmd_array[@]}" 2>&1 > "$tmp_output"
> +	_run_check "MAYFAIL" "CMD(mayfail)" "true" "$@"
>   	ret=$?
> -
> -	cat "$tmp_output" >> "$RESULTS"
> -	cat "$tmp_output"
> -	rm -- "$tmp_output"
> -
>   	if [ "$ret" != 0 ]; then
>   		echo "failed (ignored, ret=$ret): $@" >> "$RESULTS"
>   		if [ "$ret" == 139 ]; then
> @@ -312,12 +343,7 @@ run_mustfail()
>   		exit 1
>   	fi
>
> -
> -	expand_command "$@"
> -	echo "====== RUN MUSTFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
> -	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mustfail): ${cmd_array[@]}" \
> -		> /dev/tty; fi
> -	"${cmd_array[@]}" >> "$RESULTS" 2>&1
> +	_run_check "MUSTFAIL" "CMD(mustfail)" "false" "$@"
>   	if [ $? != 0 ]; then
>   		echo "failed (expected): $@" >> "$RESULTS"
>   		return 0
> @@ -337,9 +363,6 @@ run_mustfail_stdout()
>   {
>   	local msg
>   	local ret
> -	local tmp_output
> -
> -	tmp_output=$(mktemp --tmpdir btrfs-progs-mustfail-stdout.XXXXXX)
>
>   	msg="$1"
>   	shift
> @@ -349,17 +372,8 @@ run_mustfail_stdout()
>   		exit 1
>   	fi
>
> -	expand_command "$@"
> -	echo "====== RUN MUSTFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
> -	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mustfail): ${cmd_array[@]}" \
> -		> /dev/tty; fi
> -	"${cmd_array[@]}" 2>&1 > "$tmp_output"
> +	_run_check "MUSTFAIL" "CMD(mustfail)" "true" "$@"
>   	ret=$?
> -
> -	cat "$tmp_output" >> "$RESULTS"
> -	cat "$tmp_output"
> -	rm "$tmp_output"
> -
>   	if [ "$ret" != 0 ]; then
>   		echo "failed (expected): $@" >> "$RESULTS"
>   		return 0

Re: [PATCH 2/3] btrfs-progs: properly cleanup aborted transactions in check

[Josef Bacik]

On Wed, Sep 06, 2023 at 06:55:45AM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/9/6 04:21, Josef Bacik wrote:
> > There are several places that we call btrfs_abort_transaction() in a
> > failure case, but never call btrfs_commit_transaction().  This leaks the
> > trans handle and the associated extent buffers and such.  Fix all these
> > sites by making sure we call btrfs_commit_transaction() after we call
> > btrfs_abort_transaction() to make sure all the appropriate cleanup is
> > done.  This gets rid of the leaked extent buffer errors.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Although I'd say wouldn't it be better to make btrfs_abort_transaction()
> more standalone?
> 
> It's pretty instinctive to think btrfs_abort_transaction() should handle
> everything.
> 

It doesn't handle everything in the kernel, we call abort but we still have to
call btrfs_end_transaction() to clean up the trans handle.  Thanks,

Josef

Re: [PATCH 3/3] btrfs-progs: add extent buffer leak detection to make test

[Josef Bacik]

On Tue, Sep 05, 2023 at 10:49:56PM +0200, David Sterba wrote:
> On Tue, Sep 05, 2023 at 04:21:53PM -0400, Josef Bacik wrote:
> > I introduced a regression where we were leaking extent buffers, and this
> > resulted in the CI failing because we were spewing these errors.
> > 
> > Instead of waiting for fstests to catch my mistakes, check every command
> > output for leak messages, and fail the test if we detect any of these
> > messages.  I've made this generic enough that we could check for other
> > debug messages in the future.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> There already is a script to verify more than just the error leaks, I've
> added it to all the test type running scripts so it would fail.
> > ---
> >  tests/common | 108 +++++++++++++++++++++++++++++----------------------
> >  1 file changed, 61 insertions(+), 47 deletions(-)
> > 
> > diff --git a/tests/common b/tests/common
> > index 602a4122..607ad747 100644
> > --- a/tests/common
> > +++ b/tests/common
> > @@ -160,6 +160,18 @@ _is_target_command()
> >  	return 1
> >  }
> >  
> > +# Check to see if there's any debug messages that may mean we have a problem.
> > +_check_output()
> > +{
> > +	local results="$1"
> > +
> > +	if grep -q "extent buffer leak" "$results"; then
> > +		_fail "extent buffer leak reported"
> 
> There's more than that we'd like to catch, see tests/scan-results.sh.

Yup I prefer your solution, you can ignore this patch.  Thanks,

Josef

Re: [PATCH 3/3] btrfs-progs: add extent buffer leak detection to make test

[David Sterba]

On Wed, Sep 06, 2023 at 06:57:55AM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/9/6 04:21, Josef Bacik wrote:
> > I introduced a regression where we were leaking extent buffers, and this
> > resulted in the CI failing because we were spewing these errors.
> >
> > Instead of waiting for fstests to catch my mistakes, check every command
> > output for leak messages, and fail the test if we detect any of these
> > messages.  I've made this generic enough that we could check for other
> > debug messages in the future.
> >
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> Another solution is to make debug build of btrfs-progs more noisy when
> eb leak is detected.
> 
> Instead of a graceful exit (which is suitable for release build), a
> noisy BUG_ON()/ASSERT() would definitely catch our attention, and
> requires less work in the test framework.

The eb leak checks are in the kernel-shared directory that does not
share much with the debugging helpers in the other code. Eg. the message
helpers, so we can't even grep for ERROR in the logs when there are no
supposed to be seen.

Re: [PATCH 0/3] btrfs-progs: add eb leak detection and fixes

[David Sterba]

On Tue, Sep 05, 2023 at 04:21:50PM -0400, Josef Bacik wrote:
> Hello,
> 
> I introduced an EB leak that we only discovered when we started running fstests
> with my code applied to the btrfs-progs devel branch.  We really want to detect
> this before we start using this code for fstests, so update all the run_check
> related helpers to use a helper that will check for extent buffer leaks and fail
> accordingly.  This will allow developers to discover they've introduced a
> problem when they run make test after their changes.
> 
> This functionality of course uncovered a few leaks that currently exist in
> btrfs-progs, so there are two fixes that precede the leak detection work in
> order to make sure we are clean from the leak detection commit ondwards.
> Thanks,
> 
> Josef
> 
> Josef Bacik (3):
>   btrfs-progs: cleanup dirty buffers on transaction abort
>   btrfs-progs: properly cleanup aborted transactions in check
>   btrfs-progs: add extent buffer leak detection to make test

1 and 2 added to devel, the leak detection is done in a different way,
thanks.