[PATCH 0/6] btrfs-progs: Fixes for valgrind errors during fsck-tests

[PATCH 0/6] btrfs-progs: Fixes for valgrind errors during fsck-tests

[Qu Wenruo]

This patchset can be fetched from github:
https://github.com/adam900710/btrfs-progs/tree/valgrind_fixes

Inspired by that long-existing-but-I-can't-reproduce v5.1 bug, I will
never trust D=asan/D=uban anymore, and run valgrind on all fsck-tests.

The patchset is the result from the latest valgrind runs.

The first patch is to make "make INSTRUMENT=valgrind test-fsck" run
smoothly without false alerts due to mount/umount failure with valgrind.

The rest are the fixes for the error reports.

There are 2 more error reports, which I tend to believe they are just
false alerts.
Both are from fsck/012:
==122119== Syscall param pwrite64(buf) points to uninitialised byte(s)
==122119==    at 0x49F303A: pwrite (in /usr/lib/libpthread-2.31.so)
==122119==    by 0x1A5CA1: write_extent_to_disk (extent_io.c:816)
==122119==    by 0x1B2523: write_and_map_eb (disk-io.c:512)
==122119==    by 0x1B26C3: write_tree_block (disk-io.c:545)
==122119==    by 0x1D481B: __commit_transaction (transaction.c:148)
==122119==    by 0x1D4A9B: btrfs_commit_transaction (transaction.c:213)
==122119==    by 0x16360D: fixup_extent_refs (main.c:7662)
==122119==    by 0x16449F: check_extent_refs (main.c:8033)
==122119==    by 0x166199: check_chunks_and_extents (main.c:8786)
==122119==    by 0x166441: do_check_chunks_and_extents (main.c:8842)
==122119==    by 0x169D13: cmd_check (main.c:10324)
==122119==    by 0x11CDC6: cmd_execute (commands.h:125)
==122119==  Address 0x4e8aeb0 is 128 bytes inside a block of size 4,224 alloc'd
==122119==    at 0x483BB65: calloc (vg_replace_malloc.c:762)
==122119==    by 0x1A54C5: __alloc_extent_buffer (extent_io.c:609)
==122119==    by 0x1A5AED: alloc_extent_buffer (extent_io.c:753)
==122119==    by 0x1B1A26: btrfs_find_create_tree_block (disk-io.c:222)
==122119==    by 0x1BD4BE: btrfs_alloc_free_block (extent-tree.c:2538)
==122119==    by 0x1A8CFF: __btrfs_cow_block (ctree.c:322)
==122119==    by 0x1A91E2: btrfs_cow_block (ctree.c:415)
==122119==    by 0x1AB188: btrfs_search_slot (ctree.c:1185)
==122119==    by 0x160BBC: delete_extent_records (main.c:6652)
==122119==    by 0x16343F: fixup_extent_refs (main.c:7629)
==122119==    by 0x16449F: check_extent_refs (main.c:8033)
==122119==    by 0x166199: check_chunks_and_extents (main.c:8786)
==122119==

These 128 bytes are just the extent buffer in-memory header, which is
properly initialized, and we don't write that in-memory header on to
disk.

==122119== Conditional jump or move depends on uninitialised value(s)
==122119==    at 0x1B63A9: warning_trace (kerncompat.h:102)
==122119==    by 0x1BBF74: __free_extent (extent-tree.c:2042)
==122119==    by 0x1C0501: run_delayed_tree_ref (extent-tree.c:3758)
==122119==    by 0x1C058D: run_one_delayed_ref (extent-tree.c:3778)
==122119==    by 0x1C079F: btrfs_run_delayed_refs (extent-tree.c:3862)
==122119==    by 0x1D4903: btrfs_commit_transaction (transaction.c:172)
==122119==    by 0x1581AC: repair_btree (main.c:3508)
==122119==    by 0x158A76: check_fs_root (main.c:3671)
==122119==    by 0x158EAB: check_fs_roots (main.c:3771)
==122119==    by 0x159291: do_check_fs_roots (main.c:3888)
==122119==    by 0x169EFB: cmd_check (main.c:10364)
==122119==    by 0x11CDC6: cmd_execute (commands.h:125)
==122119==

I checked all related members, from the delayed tree ref to related
members, can't really find out what's wrong, as all memory looks
initialized without problem.

With this patchset applied (along with that fix for v5.1), fsck tests
all passes without valgrind error except mentioned fsck/012 above.

Qu Wenruo (6):
  btrfs-progs: tests/common: Don't call INSTRUMENT on mount command
  btrfs-progs: check/original: Fix uninitialized stack memory access for
    deal_root_from_list()
  btrfs-progs: check/original: Fix uninitialized memory for newly
    allocated data_backref
  btrfs-progs: check/original: Fix uninitialized return value from
    btrfs_write_dirty_block_groups()
  btrfs-progs: check/original: Fix uninitialized extent buffer contents
  btrfs-progs: extent-tree: Fix wrong post order rb tree cleanup for
    block groups

 check/main.c  | 4 ++--
 extent-tree.c | 3 +--
 extent_io.c   | 1 +
 tests/common  | 8 +++++++-
 4 files changed, 11 insertions(+), 5 deletions(-)

-- 
2.25.2

[PATCH 1/6] btrfs-progs: tests/common: Don't call INSTRUMENT on mount command

[Qu Wenruo]

[BUG]
With INSTRUMENT=valgrind set, some fsck tests will fail, e.g. fsck/013:
  ====== RUN CHECK mount -t btrfs -o loop /home/adam/btrfs/btrfs-progs/tests//test.img /home/adam/btrfs/btrfs-progs/tests//mnt
  ==114106==
  ==114106== Warning: Can't execute setuid/setgid/setcap executable: /usr/bin/mount
  ==114106== Possible workaround: remove --trace-children=yes, if in effect
  ==114106==
  valgrind: /usr/bin/mount: Permission denied
  failed: mount -t btrfs -o loop /home/adam/btrfs/btrfs-progs/tests//test.img /home/adam/btrfs/btrfs-progs/tests//mnt
  test failed for case 013-extent-tree-rebuild

[CAUSE]
Just as stated by valgrind itself, it can't handle program with
setuid/setgid/setcap.

Thankfully in our case it's mount and we don't really care about it at
all.

[FIX]
Although we could use complex skip pattern to skip mount in valgrind, we
don't really want to run valgrind on mount or sudo command anyway.

So here we do extra check if we're running mount command. And if that's
the case, just skip $INSTRUMENT command.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/common | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/common b/tests/common
index e04ceeb6ccbe..71661e950db0 100644
--- a/tests/common
+++ b/tests/common
@@ -154,7 +154,13 @@ run_check()
 	set -- "${@:1:$(($ins-1))}" $spec "${@: $ins}"
 	echo "====== RUN CHECK $@" >> "$RESULTS" 2>&1
 	if [[ $TEST_LOG =~ tty ]]; then echo "CMD: $@" > /dev/tty; fi
-	if [ "$1" = 'root_helper' ]; then
+
+	# If the command is `root_helper` or mount/umount, don't call INSTRUMENT
+	# as most profiling tool like valgrind can't handle setuid/setgid/setcap
+	# which mount normally has.
+	# And since mount/umount is only called with run_check(), we don't need
+	# to do the same check on other run_*() functions.
+	if [ "$1" = 'root_helper' -o "$1" = 'mount' -o "$1" = 'umount' ]; then
 		"$@" >> "$RESULTS" 2>&1 || _fail "failed: $@"
 	else
 		$INSTRUMENT "$@" >> "$RESULTS" 2>&1 || _fail "failed: $@"
-- 
2.25.2

[PATCH 2/6] btrfs-progs: check/original: Fix uninitialized stack memory access for deal_root_from_list()

[Qu Wenruo]

[BUG]
With valgrind, fsck/002 test with original mode would report the
following valgrind error:
  ==90600== Conditional jump or move depends on uninitialised value(s)
  ==90600==    at 0x15C280: pick_next_pending (main.c:4949)
  ==90600==    by 0x15F3CF: run_next_block (main.c:6175)
  ==90600==    by 0x1655CC: deal_root_from_list (main.c:8486)
  ==90600==    by 0x1660C7: check_chunks_and_extents (main.c:8762)
  ==90600==    by 0x166439: do_check_chunks_and_extents (main.c:8842)
  ==90600==    by 0x169D0B: cmd_check (main.c:10324)
  ==90600==    by 0x11CDC6: cmd_execute (commands.h:125)
  ==90600==    by 0x11D712: main (btrfs.c:386)

[CAUSE]
The problem happens like this:
deal_root_from_list(@list is empty)
|- stack @last is not initialized
|- while(!list_empty(list)) {} is skipped
|- run_next_block(&last);
   |- pick_next_pending(*last);
      |- node_start = last;

Since the stack @last is not initialized in deal_root_from_list(), the
final node_start = last assignment would just fetch the garbage from
stack.

[FIX]
Fix the problem by initializing @last to 0, as that's exactly what the
first while loop did.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index b56255bc10a8..d8181249e394 100644
--- a/check/main.c
+++ b/check/main.c
@@ -8442,7 +8442,7 @@ static int deal_root_from_list(struct list_head *list,
 			       struct device_extent_tree *dev_extent_cache)
 {
 	int ret = 0;
-	u64 last;
+	u64 last = 0;
 
 	while (!list_empty(list)) {
 		struct root_item_record *rec;
-- 
2.25.2

[PATCH 3/6] btrfs-progs: check/original: Fix uninitialized memory for newly allocated data_backref

[Qu Wenruo]

[BUG]
Valgrind reports the following error for fsck/002 (which only supports
original mode):
  ==97088== Conditional jump or move depends on uninitialised value(s)
  ==97088==    at 0x15BFF6: add_data_backref (main.c:4884)
  ==97088==    by 0x16025C: run_next_block (main.c:6452)
  ==97088==    by 0x165539: deal_root_from_list (main.c:8471)
  ==97088==    by 0x166040: check_chunks_and_extents (main.c:8753)
  ==97088==    by 0x166441: do_check_chunks_and_extents (main.c:8842)
  ==97088==    by 0x169D13: cmd_check (main.c:10324)
  ==97088==    by 0x11CDC6: cmd_execute (commands.h:125)
  ==97088==    by 0x11D712: main (btrfs.c:386)

[CAUSE]
In alloc_data_backref(), only ref->node is set to 0.
While ref->disk_bytenr is not initialized at all.

And then in add_data_backref(), if @back is a newly allocated data
backref, we use the garbage from back->disk_bytenr to determine if we
should reset them.

[FIX]
Fix it by initialize the whole data_backref structure in
alloc_data_backref().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index d8181249e394..37c5b35a36bd 100644
--- a/check/main.c
+++ b/check/main.c
@@ -4516,7 +4516,7 @@ static struct data_backref *alloc_data_backref(struct extent_record *rec,
 
 	if (!ref)
 		return NULL;
-	memset(&ref->node, 0, sizeof(ref->node));
+	memset(ref, 0, sizeof(*ref));
 	ref->node.is_data = 1;
 
 	if (parent > 0) {
-- 
2.25.2

[PATCH 4/6] btrfs-progs: check/original: Fix uninitialized return value from btrfs_write_dirty_block_groups()

[Qu Wenruo]

[BUG]
Valgrind reports the following error for fsck/007, which is only
repairable for original mode:
  ==97599== Conditional jump or move depends on uninitialised value(s)
  ==97599==    at 0x1D4A42: btrfs_commit_transaction (transaction.c:207)
  ==97599==    by 0x16475C: check_extent_refs (main.c:8097)
  ==97599==    by 0x166199: check_chunks_and_extents (main.c:8786)
  ==97599==    by 0x166441: do_check_chunks_and_extents (main.c:8842)
  ==97599==    by 0x169D13: cmd_check (main.c:10324)
  ==97599==    by 0x11CDC6: cmd_execute (commands.h:125)
  ==97599==    by 0x11D712: main (btrfs.c:386)
  ==97599==

[CAUSE]
If btrfs_write_dirty_block_groups() get called with no block group
dirtied (no dirty extents created), the return value of it is
uninitialized, as the stack @ret is not initialized at all.

[FIX]
Initialize @ret to 0 for btrfs_write_dirty_block_groups() as if there is
no dirty block groups, we do nothing and shouldn't fail.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 extent-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extent-tree.c b/extent-tree.c
index dc4b052c1666..f0cb9faa4da6 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -1564,7 +1564,7 @@ int btrfs_write_dirty_block_groups(struct btrfs_trans_handle *trans)
 {
 	struct btrfs_block_group_cache *cache;
 	struct btrfs_path *path;
-	int ret;
+	int ret = 0;
 
 	path = btrfs_alloc_path();
 	if (!path)
-- 
2.25.2

[PATCH 5/6] btrfs-progs: check/original: Fix uninitialized extent buffer contents

[Qu Wenruo]

[BUG]
Valgrind reports the following error for fsck/012:
  adding new tree backref on start 4206592 len 4096 parent 0 root 5
  ==100735== Syscall param pwrite64(buf) points to uninitialised byte(s)
  ==100735==    at 0x49F303A: pwrite (in /usr/lib/libpthread-2.31.so)
  ==100735==    by 0x1A5C85: write_extent_to_disk (extent_io.c:815)
  ==100735==    by 0x1B2507: write_and_map_eb (disk-io.c:512)
  ==100735==    by 0x1B26A7: write_tree_block (disk-io.c:545)
  ==100735==    by 0x1D4822: __commit_transaction (transaction.c:148)
  ==100735==    by 0x1D4AA2: btrfs_commit_transaction (transaction.c:213)
  ==100735==    by 0x16360D: fixup_extent_refs (main.c:7662)
  ==100735==    by 0x16449F: check_extent_refs (main.c:8033)
  ==100735==    by 0x166199: check_chunks_and_extents (main.c:8786)
  ==100735==    by 0x166441: do_check_chunks_and_extents (main.c:8842)
  ==100735==    by 0x169D13: cmd_check (main.c:10324)
  ==100735==    by 0x11CDC6: cmd_execute (commands.h:125)
  ==100735==  Address 0x4e8aeb0 is 128 bytes inside a block of size 4,224 alloc'd
  ==100735==    at 0x483BB65: calloc (vg_replace_malloc.c:762)
  ==100735==    by 0x1A54C5: __alloc_extent_buffer (extent_io.c:609)
  ==100735==    by 0x1A5AD1: alloc_extent_buffer (extent_io.c:752)
  ==100735==    by 0x1B1A0A: btrfs_find_create_tree_block (disk-io.c:222)
  ==100735==    by 0x1BD4A2: btrfs_alloc_free_block (extent-tree.c:2538)
  ==100735==    by 0x1A8CE3: __btrfs_cow_block (ctree.c:322)
  ==100735==    by 0x1A91C6: btrfs_cow_block (ctree.c:415)
  ==100735==    by 0x1AB16C: btrfs_search_slot (ctree.c:1185)
  ==100735==    by 0x160BBC: delete_extent_records (main.c:6652)
  ==100735==    by 0x16343F: fixup_extent_refs (main.c:7629)
  ==100735==    by 0x16449F: check_extent_refs (main.c:8033)
  ==100735==    by 0x166199: check_chunks_and_extents (main.c:8786)
  ==100735==

[CAUSE]
For new extent buffer allocated, we don't initialize its content.

This is not a major concern, at all.
For the above report, the reported range is inside the unused part of
the extent buffer, thus won't cause anything.

Regular btrfs_cow_block() will cover all the used ranges of one extent
buffer.

[FIX]
But still, since kernel initialize the extent buffer with 0, it won't
hurt to do extra initialized to make valgrind happy.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 extent_io.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/extent_io.c b/extent_io.c
index f11917a4c6fc..4b5acb1aabf0 100644
--- a/extent_io.c
+++ b/extent_io.c
@@ -622,6 +622,7 @@ static struct extent_buffer *__alloc_extent_buffer(struct btrfs_fs_info *info,
 	eb->tree = &info->extent_cache;
 	INIT_LIST_HEAD(&eb->recow);
 	INIT_LIST_HEAD(&eb->lru);
+	memset_extent_buffer(eb, 0, 0, blocksize);
 
 	return eb;
 }
-- 
2.25.2

[PATCH 6/6] btrfs-progs: extent-tree: Fix wrong post order rb tree cleanup for block groups

[Qu Wenruo]

[BUG]
Valgrind reports memory leak for fsck/012, even after the image is
repair, the memory leak can still be reproduced.
  ==107060== HEAP SUMMARY:
  ==107060==     in use at exit: 176 bytes in 1 blocks
  ==107060==   total heap usage: 10,647 allocs, 10,646 frees, 3,000,654 bytes allocated
  ==107060==
  ==107060== 176 bytes in 1 blocks are definitely lost in loss record 1 of 1
  ==107060==    at 0x483BB65: calloc (vg_replace_malloc.c:762)
  ==107060==    by 0x1BD953: read_one_block_group (extent-tree.c:2661)
  ==107060==    by 0x1BDBD8: btrfs_read_block_groups (extent-tree.c:2719)
  ==107060==    by 0x1B3A2C: btrfs_setup_all_roots (disk-io.c:1024)
  ==107060==    by 0x1B44CA: __open_ctree_fd (disk-io.c:1299)
  ==107060==    by 0x1B46C6: open_ctree_fs_info (disk-io.c:1345)
  ==107060==    by 0x16952E: cmd_check (main.c:10154)
  ==107060==    by 0x11CDC6: cmd_execute (commands.h:125)
  ==107060==    by 0x11D712: main (btrfs.c:386)
  ==107060==
  ==107060== LEAK SUMMARY:
  ==107060==    definitely lost: 176 bytes in 1 blocks
  ==107060==    indirectly lost: 0 bytes in 0 blocks
  ==107060==      possibly lost: 0 bytes in 0 blocks
  ==107060==    still reachable: 0 bytes in 0 blocks
  ==107060==         suppressed: 0 bytes in 0 blocks

[CAUSE]
In btrfs_free_block_groups(), we use
rbtree_postorder_for_each_entry_safe() to iterate all block group cache.

However since we're already doing post order iteration, we shouldn't
call rb_erase() during that iteration, as it would re-balance the tree,
and break the post order iteration.

This wrong rb_erase() call leads to above memory leak.

[FIX]
Kill that wrong rb_erase() call.

Fixes: b1bd3cd93fff ("btrfs-progs: reform block groups caches structure")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 extent-tree.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/extent-tree.c b/extent-tree.c
index f0cb9faa4da6..271a2714b889 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -2560,7 +2560,6 @@ int btrfs_free_block_groups(struct btrfs_fs_info *info)
 			     &info->block_group_cache_tree, cache_node) {
 		if (!list_empty(&cache->dirty_list))
 			list_del_init(&cache->dirty_list);
-		rb_erase(&cache->cache_node, &info->block_group_cache_tree);
 		RB_CLEAR_NODE(&cache->cache_node);
 		if (cache->free_space_ctl) {
 			btrfs_remove_free_space_cache(cache);
-- 
2.25.2

Re: [PATCH 0/6] btrfs-progs: Fixes for valgrind errors during fsck-tests

[David Sterba]

On Tue, Mar 24, 2020 at 06:53:09PM +0800, Qu Wenruo wrote:
> This patchset can be fetched from github:
> https://github.com/adam900710/btrfs-progs/tree/valgrind_fixes
> 
> Inspired by that long-existing-but-I-can't-reproduce v5.1 bug, I will
> never trust D=asan/D=uban anymore, and run valgrind on all fsck-tests.
> 
> The patchset is the result from the latest valgrind runs.
> 
> The first patch is to make "make INSTRUMENT=valgrind test-fsck" run
> smoothly without false alerts due to mount/umount failure with valgrind.

Thanks, that's great. In addition to that, all commands that use the
SUDO_HELPER/root_helper won't pass through valgrind. For maximum
coverage we might want to remove the helper from the subcommands of
'btrfs'. From a quick scan I found a lot of them and I'm not sure that
all are required. There's a lot of copy&paste in the tests, so that
would have to be cleaned up, or we leave it as it is and run the whole
tests under root.

> With this patchset applied (along with that fix for v5.1), fsck tests
> all passes without valgrind error except mentioned fsck/012 above.
> 
> Qu Wenruo (6):
>   btrfs-progs: tests/common: Don't call INSTRUMENT on mount command
>   btrfs-progs: check/original: Fix uninitialized stack memory access for
>     deal_root_from_list()
>   btrfs-progs: check/original: Fix uninitialized memory for newly
>     allocated data_backref
>   btrfs-progs: check/original: Fix uninitialized return value from
>     btrfs_write_dirty_block_groups()
>   btrfs-progs: check/original: Fix uninitialized extent buffer contents
>   btrfs-progs: extent-tree: Fix wrong post order rb tree cleanup for
>     block groups

Added to devel.

Re: [PATCH 0/6] btrfs-progs: Fixes for valgrind errors during fsck-tests

[Qu Wenruo]

On 2020/3/25 下午10:42, David Sterba wrote:
> On Tue, Mar 24, 2020 at 06:53:09PM +0800, Qu Wenruo wrote:
>> This patchset can be fetched from github:
>> https://github.com/adam900710/btrfs-progs/tree/valgrind_fixes
>>
>> Inspired by that long-existing-but-I-can't-reproduce v5.1 bug, I will
>> never trust D=asan/D=uban anymore, and run valgrind on all fsck-tests.
>>
>> The patchset is the result from the latest valgrind runs.
>>
>> The first patch is to make "make INSTRUMENT=valgrind test-fsck" run
>> smoothly without false alerts due to mount/umount failure with valgrind.
> 
> Thanks, that's great. In addition to that, all commands that use the
> SUDO_HELPER/root_helper won't pass through valgrind. For maximum
> coverage we might want to remove the helper from the subcommands of
> 'btrfs'. From a quick scan I found a lot of them and I'm not sure that
> all are required. There's a lot of copy&paste in the tests, so that
> would have to be cleaned up, or we leave it as it is and run the whole
> tests under root.

The root fix is, like what we did for lowmem mode, injecting valgrind to
proper location.

Currently I take a shortcut to reuse current infrastructure, but the
root fix would need to inject INSTRUMENT directly before
"btrfs/mkfs.btrfs/btrfs-convert", so that sudo_helper won't be a problem.

I would work on that if it's OK for you.

Thanks,
Qu

> 
>> With this patchset applied (along with that fix for v5.1), fsck tests
>> all passes without valgrind error except mentioned fsck/012 above.
>>
>> Qu Wenruo (6):
>>   btrfs-progs: tests/common: Don't call INSTRUMENT on mount command
>>   btrfs-progs: check/original: Fix uninitialized stack memory access for
>>     deal_root_from_list()
>>   btrfs-progs: check/original: Fix uninitialized memory for newly
>>     allocated data_backref
>>   btrfs-progs: check/original: Fix uninitialized return value from
>>     btrfs_write_dirty_block_groups()
>>   btrfs-progs: check/original: Fix uninitialized extent buffer contents
>>   btrfs-progs: extent-tree: Fix wrong post order rb tree cleanup for
>>     block groups
> 
> Added to devel.
> 

Re: [PATCH 0/6] btrfs-progs: Fixes for valgrind errors during fsck-tests

[David Sterba]

On Thu, Mar 26, 2020 at 08:59:16AM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/3/25 下午10:42, David Sterba wrote:
> > On Tue, Mar 24, 2020 at 06:53:09PM +0800, Qu Wenruo wrote:
> >> This patchset can be fetched from github:
> >> https://github.com/adam900710/btrfs-progs/tree/valgrind_fixes
> >>
> >> Inspired by that long-existing-but-I-can't-reproduce v5.1 bug, I will
> >> never trust D=asan/D=uban anymore, and run valgrind on all fsck-tests.
> >>
> >> The patchset is the result from the latest valgrind runs.
> >>
> >> The first patch is to make "make INSTRUMENT=valgrind test-fsck" run
> >> smoothly without false alerts due to mount/umount failure with valgrind.
> > 
> > Thanks, that's great. In addition to that, all commands that use the
> > SUDO_HELPER/root_helper won't pass through valgrind. For maximum
> > coverage we might want to remove the helper from the subcommands of
> > 'btrfs'. From a quick scan I found a lot of them and I'm not sure that
> > all are required. There's a lot of copy&paste in the tests, so that
> > would have to be cleaned up, or we leave it as it is and run the whole
> > tests under root.
> 
> The root fix is, like what we did for lowmem mode, injecting valgrind to
> proper location.
> 
> Currently I take a shortcut to reuse current infrastructure, but the
> root fix would need to inject INSTRUMENT directly before
> "btrfs/mkfs.btrfs/btrfs-convert", so that sudo_helper won't be a problem.

That's a great idea. For some reason I thought that valgrind refused to
work under root but that's not true. Injecting the instrumentation only
to the tools built from git is exactly what we want.