[PATCH 0/2] btrfs-progs: fsck: fix false warning on sprouted filesystems

[PATCH 0/2] btrfs-progs: fsck: fix false warning on sprouted filesystems

[Qu Wenruo]

During my attempt to utilize seed device at mkfs time, I found there
there is a small bug that btrfs check always reports devices size
related warning on the sprouted device.

It turns out we didn't iterate seed devices at all during btrfs check.

The first patch will fix the problem and enhance the warning output.
The second one will add a regression test for it.

Qu Wenruo (2):
  btrfs-progs: check: fix wrong total bytes check for seed device
  btrfs-progs: fsck-tests: check warning for seed and sprouted
    filesystems

 check/main.c                                  | 18 ++++---
 .../fsck-tests/057-seed-false-alerts/test.sh  | 51 +++++++++++++++++++
 2 files changed, 63 insertions(+), 6 deletions(-)
 create mode 100755 tests/fsck-tests/057-seed-false-alerts/test.sh

-- 
2.35.1

[PATCH 1/2] btrfs-progs: check: fix wrong total bytes check for seed device

[Qu Wenruo]

[BUG]
The following script can lead to false positive from btrfs check:

  mkfs.btrfs -f $dev1
  mount $dev1 $mnt
  btrfstune -S1 $dev1
  mount $dev1 $mnt
  btrfs dev add -f $dev2 $mnt
  umount $mnt

  # Now dev1 is seed, and dev2 is the rw fs.
  btrfs check $dev2
  ...
  [2/7] checking extents
  WARNING: minor unaligned/mismatch device size detected
  WARNING: recommended to use 'btrfs rescue fix-device-size' to fix it
  ...

This false positive only happens on $dev2, $dev1 is completely fine.

[CAUSE]
The warning is from is_super_size_valid(), in that function we verify
the super block total bytes (@super_bytes) is correct against the total
device bytes (@total_bytes).

However the when calculating @total_bytes, we only use devices in
current fs_devices, which only contains RW devices.

Thus all bytes from seed device are not taken into consideration, and
trigger the false positive.

[FIX]
Fix it by also iterating seed devices.

Since we're here, also output @total_bytes and @super_bytes when
outputting the warning message, to allow end users have a better idea on
what's going wrong.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/check/main.c b/check/main.c
index e6e85784d5ea..64fc6f2ebdb7 100644
--- a/check/main.c
+++ b/check/main.c
@@ -8550,13 +8550,17 @@ static int check_device_used(struct device_record *dev_rec,
  */
 static bool is_super_size_valid(void)
 {
-	struct btrfs_device *dev;
-	struct list_head *dev_list = &gfs_info->fs_devices->devices;
+	struct btrfs_fs_devices *fs_devices = gfs_info->fs_devices;
+	const u64 super_bytes = btrfs_super_total_bytes(gfs_info->super_copy);
 	u64 total_bytes = 0;
-	u64 super_bytes = btrfs_super_total_bytes(gfs_info->super_copy);
 
-	list_for_each_entry(dev, dev_list, dev_list)
-		total_bytes += dev->total_bytes;
+	while (fs_devices) {
+		struct btrfs_device *dev;
+
+		list_for_each_entry(dev, &fs_devices->devices, dev_list)
+			total_bytes += dev->total_bytes;
+		fs_devices = fs_devices->seed;
+	}
 
 	/* Important check, which can cause unmountable fs */
 	if (super_bytes < total_bytes) {
@@ -8579,7 +8583,9 @@ static bool is_super_size_valid(void)
 	if (!IS_ALIGNED(super_bytes, gfs_info->sectorsize) ||
 	    !IS_ALIGNED(total_bytes, gfs_info->sectorsize) ||
 	    super_bytes != total_bytes) {
-		warning("minor unaligned/mismatch device size detected");
+		warning("minor unaligned/mismatch device size detected:");
+		warning("  super block total bytes=%llu found total bytes=%llu",
+			super_bytes, total_bytes);
 		warning(
 		"recommended to use 'btrfs rescue fix-device-size' to fix it");
 	}
-- 
2.35.1

[PATCH 2/2] btrfs-progs: fsck-tests: check warning for seed and sprouted filesystems

[Qu Wenruo]

Previously we had a bug that btrfs check would report false warning for
a sprouted filesystem.

So this patch will add a new test case to make sure neither seed nor
and sprouted filesystem will cause such false warning.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../fsck-tests/057-seed-false-alerts/test.sh  | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100755 tests/fsck-tests/057-seed-false-alerts/test.sh

diff --git a/tests/fsck-tests/057-seed-false-alerts/test.sh b/tests/fsck-tests/057-seed-false-alerts/test.sh
new file mode 100755
index 000000000000..3a442c1202d0
--- /dev/null
+++ b/tests/fsck-tests/057-seed-false-alerts/test.sh
@@ -0,0 +1,51 @@
+#!/bin/bash
+#
+# Make sure "btrfs check" won't report false alerts on sprouted filesystems
+#
+
+source "$TEST_TOP/common"
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+check_prereq btrfstune
+check_global_prereq losetup
+
+setup_loopdevs 2
+prepare_loopdevs
+dev1=${loopdevs[1]}
+dev2=${loopdevs[2]}
+TEST_DEV=$dev1
+
+setup_root_helper
+
+run_check $SUDO_HELPERS "$TOP/mkfs.btrfs" -f $dev1
+run_check $SUDO_HELPERS "$TOP/btrfstune" -S1 $dev1
+run_check_mount_test_dev
+run_check $SUDO_HELPERS "$TOP/btrfs" device add -f $dev2 $TEST_MNT
+
+# Here we can not use umount helper, as it uses the seed device to do the
+# umount. We need to manually unmout using the mount point
+run_check $SUDO_HELPERS umount $TEST_MNT
+
+seed_output=$(_mktemp --tmpdir btrfs-progs-seed-check-stdout.XXXXXX)
+sprouted_output=$(_mktemp --tmpdir btrfs-progs-sprouted-check-stdout.XXXXXX)
+
+# The false alerts are just warnings, so we need to save and filter
+# the output
+run_check_stdout "$TOP/btrfs" check "$dev1" >> "$seed_output"
+run_check_stdout "$TOP/btrfs" check "$dev2" >> "$sprouted_output"
+
+# There should be no warning for both seed and sprouted fs
+if grep -q "WARNING" "$seed_output"; then
+	cleanup_loopdevs
+	rm -f -- "$seed_output" "$sprouted_output"
+	_fail "false alerts detected for seed fs"
+fi
+if grep -q "WARNING" "$sprouted_output"; then
+	cleanup_loopdevs
+	rm -f -- "$seed_output" "$sprouted_output"
+	_fail "false alerts detected for sprouted fs"
+fi
+
+cleanup_loopdevs
+rm -f -- "$seed_output" "$sprouted_output"
-- 
2.35.1

Re: [PATCH 1/2] btrfs-progs: check: fix wrong total bytes check for seed device

[Su Yue]

On 2022/4/17 15:30, Qu Wenruo wrote:
> [BUG]
> The following script can lead to false positive from btrfs check:
> 
>    mkfs.btrfs -f $dev1
>    mount $dev1 $mnt
>    btrfstune -S1 $dev1
>    mount $dev1 $mnt
>    btrfs dev add -f $dev2 $mnt
>    umount $mnt
> 
>    # Now dev1 is seed, and dev2 is the rw fs.
>    btrfs check $dev2
>    ...
>    [2/7] checking extents
>    WARNING: minor unaligned/mismatch device size detected
>    WARNING: recommended to use 'btrfs rescue fix-device-size' to fix it
>    ...
> 
> This false positive only happens on $dev2, $dev1 is completely fine.
> 
> [CAUSE]
> The warning is from is_super_size_valid(), in that function we verify
> the super block total bytes (@super_bytes) is correct against the total
> device bytes (@total_bytes).
> 
> However the when calculating @total_bytes, we only use devices in
> current fs_devices, which only contains RW devices.
> 
> Thus all bytes from seed device are not taken into consideration, and
> trigger the false positive.
> 
> [FIX]
> Fix it by also iterating seed devices.
> 
> Since we're here, also output @total_bytes and @super_bytes when
> outputting the warning message, to allow end users have a better idea on
> what's going wrong.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Su Yue <glass@fydeos.io>
> ---
>   check/main.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/check/main.c b/check/main.c
> index e6e85784d5ea..64fc6f2ebdb7 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -8550,13 +8550,17 @@ static int check_device_used(struct device_record *dev_rec,
>    */
>   static bool is_super_size_valid(void)
>   {
> -	struct btrfs_device *dev;
> -	struct list_head *dev_list = &gfs_info->fs_devices->devices;
> +	struct btrfs_fs_devices *fs_devices = gfs_info->fs_devices;
> +	const u64 super_bytes = btrfs_super_total_bytes(gfs_info->super_copy);
>   	u64 total_bytes = 0;
> -	u64 super_bytes = btrfs_super_total_bytes(gfs_info->super_copy);
>   
> -	list_for_each_entry(dev, dev_list, dev_list)
> -		total_bytes += dev->total_bytes;
> +	while (fs_devices) {
> +		struct btrfs_device *dev;
> +
> +		list_for_each_entry(dev, &fs_devices->devices, dev_list)
> +			total_bytes += dev->total_bytes;
> +		fs_devices = fs_devices->seed;
> +	}
>   
>   	/* Important check, which can cause unmountable fs */
>   	if (super_bytes < total_bytes) {
> @@ -8579,7 +8583,9 @@ static bool is_super_size_valid(void)
>   	if (!IS_ALIGNED(super_bytes, gfs_info->sectorsize) ||
>   	    !IS_ALIGNED(total_bytes, gfs_info->sectorsize) ||
>   	    super_bytes != total_bytes) {
> -		warning("minor unaligned/mismatch device size detected");
> +		warning("minor unaligned/mismatch device size detected:");
> +		warning("  super block total bytes=%llu found total bytes=%llu",
> +			super_bytes, total_bytes);
>   		warning(
>   		"recommended to use 'btrfs rescue fix-device-size' to fix it");
>   	}

Re: [PATCH 1/2] btrfs-progs: check: fix wrong total bytes check for seed device

[David Sterba]

On Sun, Apr 17, 2022 at 03:30:35PM +0800, Qu Wenruo wrote:
> @@ -8579,7 +8583,9 @@ static bool is_super_size_valid(void)
>  	if (!IS_ALIGNED(super_bytes, gfs_info->sectorsize) ||
>  	    !IS_ALIGNED(total_bytes, gfs_info->sectorsize) ||
>  	    super_bytes != total_bytes) {
> -		warning("minor unaligned/mismatch device size detected");
> +		warning("minor unaligned/mismatch device size detected:");
> +		warning("  super block total bytes=%llu found total bytes=%llu",
> +			super_bytes, total_bytes);

Minor thing, the message in warning() should be split the string and
indent, not add another call, that's how it it is done now. If we decide
to do some kind of auto split or reformat then we can do it inside the
helper.

>  		warning(
>  		"recommended to use 'btrfs rescue fix-device-size' to fix it");
>  	}
> -- 
> 2.35.1

Re: [PATCH 2/2] btrfs-progs: fsck-tests: check warning for seed and sprouted filesystems

[David Sterba]

On Sun, Apr 17, 2022 at 03:30:36PM +0800, Qu Wenruo wrote:
> Previously we had a bug that btrfs check would report false warning for
> a sprouted filesystem.
> 
> So this patch will add a new test case to make sure neither seed nor
> and sprouted filesystem will cause such false warning.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  .../fsck-tests/057-seed-false-alerts/test.sh  | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100755 tests/fsck-tests/057-seed-false-alerts/test.sh
> 
> diff --git a/tests/fsck-tests/057-seed-false-alerts/test.sh b/tests/fsck-tests/057-seed-false-alerts/test.sh
> new file mode 100755
> index 000000000000..3a442c1202d0
> --- /dev/null
> +++ b/tests/fsck-tests/057-seed-false-alerts/test.sh
> @@ -0,0 +1,51 @@
> +#!/bin/bash
> +#
> +# Make sure "btrfs check" won't report false alerts on sprouted filesystems
> +#
> +
> +source "$TEST_TOP/common"
> +
> +check_prereq btrfs
> +check_prereq mkfs.btrfs
> +check_prereq btrfstune
> +check_global_prereq losetup
> +
> +setup_loopdevs 2
> +prepare_loopdevs
> +dev1=${loopdevs[1]}
> +dev2=${loopdevs[2]}
> +TEST_DEV=$dev1
> +
> +setup_root_helper
> +
> +run_check $SUDO_HELPERS "$TOP/mkfs.btrfs" -f $dev1
             ^^^^^^^^^^^^^

It's $SUDO_HELPER, otherwise it does not work, also please quote all
shell variables, everywhere.

> +run_check $SUDO_HELPERS "$TOP/btrfstune" -S1 $dev1
> +run_check_mount_test_dev
> +run_check $SUDO_HELPERS "$TOP/btrfs" device add -f $dev2 $TEST_MNT
> +
> +# Here we can not use umount helper, as it uses the seed device to do the
> +# umount. We need to manually unmout using the mount point
> +run_check $SUDO_HELPERS umount $TEST_MNT
> +
> +seed_output=$(_mktemp --tmpdir btrfs-progs-seed-check-stdout.XXXXXX)

This won't work as intended, there's a helper _mktemp_dir that creates a
temporary directory, otherwise --tmpdir is interpreted as the direcotry
name.

Re: [PATCH 0/2] btrfs-progs: fsck: fix false warning on sprouted filesystems

[David Sterba]

On Sun, Apr 17, 2022 at 03:30:34PM +0800, Qu Wenruo wrote:
> During my attempt to utilize seed device at mkfs time, I found there
> there is a small bug that btrfs check always reports devices size
> related warning on the sprouted device.
> 
> It turns out we didn't iterate seed devices at all during btrfs check.
> 
> The first patch will fix the problem and enhance the warning output.
> The second one will add a regression test for it.
> 
> Qu Wenruo (2):
>   btrfs-progs: check: fix wrong total bytes check for seed device
>   btrfs-progs: fsck-tests: check warning for seed and sprouted
>     filesystems

With the test fixups added to devel, thanks.

Re: [PATCH 2/2] btrfs-progs: fsck-tests: check warning for seed and sprouted filesystems

[David Sterba]

On Tue, Apr 26, 2022 at 06:44:04AM +0800, Qu Wenruo wrote:
> > This won't work as intended, there's a helper _mktemp_dir that creates a
> > temporary directory, otherwise --tmpdir is interpreted as the direcotry
> > name.
> 
> My bad, it should be mktemp without the "_" prefix, just like what we
> did in all the run* helpers.
> 
> And I'm not expected to create a directory, thus I don't need to
> _mktemp_dir.

Right, I figured it later and dropped the _dir name and verified it
works.

Re: [PATCH 2/2] btrfs-progs: fsck-tests: check warning for seed and sprouted filesystems

[Qu Wenruo]

On 2022/4/26 01:04, David Sterba wrote:
> On Sun, Apr 17, 2022 at 03:30:36PM +0800, Qu Wenruo wrote:
>> Previously we had a bug that btrfs check would report false warning for
>> a sprouted filesystem.
>>
>> So this patch will add a new test case to make sure neither seed nor
>> and sprouted filesystem will cause such false warning.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   .../fsck-tests/057-seed-false-alerts/test.sh  | 51 +++++++++++++++++++
>>   1 file changed, 51 insertions(+)
>>   create mode 100755 tests/fsck-tests/057-seed-false-alerts/test.sh
>>
>> diff --git a/tests/fsck-tests/057-seed-false-alerts/test.sh b/tests/fsck-tests/057-seed-false-alerts/test.sh
>> new file mode 100755
>> index 000000000000..3a442c1202d0
>> --- /dev/null
>> +++ b/tests/fsck-tests/057-seed-false-alerts/test.sh
>> @@ -0,0 +1,51 @@
>> +#!/bin/bash
>> +#
>> +# Make sure "btrfs check" won't report false alerts on sprouted filesystems
>> +#
>> +
>> +source "$TEST_TOP/common"
>> +
>> +check_prereq btrfs
>> +check_prereq mkfs.btrfs
>> +check_prereq btrfstune
>> +check_global_prereq losetup
>> +
>> +setup_loopdevs 2
>> +prepare_loopdevs
>> +dev1=${loopdevs[1]}
>> +dev2=${loopdevs[2]}
>> +TEST_DEV=$dev1
>> +
>> +setup_root_helper
>> +
>> +run_check $SUDO_HELPERS "$TOP/mkfs.btrfs" -f $dev1
>               ^^^^^^^^^^^^^
>
> It's $SUDO_HELPER, otherwise it does not work, also please quote all
> shell variables, everywhere.
>
>> +run_check $SUDO_HELPERS "$TOP/btrfstune" -S1 $dev1
>> +run_check_mount_test_dev
>> +run_check $SUDO_HELPERS "$TOP/btrfs" device add -f $dev2 $TEST_MNT
>> +
>> +# Here we can not use umount helper, as it uses the seed device to do the
>> +# umount. We need to manually unmout using the mount point
>> +run_check $SUDO_HELPERS umount $TEST_MNT
>> +
>> +seed_output=$(_mktemp --tmpdir btrfs-progs-seed-check-stdout.XXXXXX)
>
> This won't work as intended, there's a helper _mktemp_dir that creates a
> temporary directory, otherwise --tmpdir is interpreted as the direcotry
> name.

My bad, it should be mktemp without the "_" prefix, just like what we
did in all the run* helpers.

And I'm not expected to create a directory, thus I don't need to
_mktemp_dir.

Thanks,
Qu

Re: [PATCH 2/2] btrfs-progs: fsck-tests: check warning for seed and sprouted filesystems

[Qu Wenruo]

On 2022/4/26 06:44, Qu Wenruo wrote:
>
>
> On 2022/4/26 01:04, David Sterba wrote:
>> On Sun, Apr 17, 2022 at 03:30:36PM +0800, Qu Wenruo wrote:
>>> Previously we had a bug that btrfs check would report false warning for
>>> a sprouted filesystem.
>>>
>>> So this patch will add a new test case to make sure neither seed nor
>>> and sprouted filesystem will cause such false warning.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   .../fsck-tests/057-seed-false-alerts/test.sh  | 51 +++++++++++++++++++
>>>   1 file changed, 51 insertions(+)
>>>   create mode 100755 tests/fsck-tests/057-seed-false-alerts/test.sh
>>>
>>> diff --git a/tests/fsck-tests/057-seed-false-alerts/test.sh
>>> b/tests/fsck-tests/057-seed-false-alerts/test.sh
>>> new file mode 100755
>>> index 000000000000..3a442c1202d0
>>> --- /dev/null
>>> +++ b/tests/fsck-tests/057-seed-false-alerts/test.sh
>>> @@ -0,0 +1,51 @@
>>> +#!/bin/bash
>>> +#
>>> +# Make sure "btrfs check" won't report false alerts on sprouted
>>> filesystems
>>> +#
>>> +
>>> +source "$TEST_TOP/common"
>>> +
>>> +check_prereq btrfs
>>> +check_prereq mkfs.btrfs
>>> +check_prereq btrfstune
>>> +check_global_prereq losetup
>>> +
>>> +setup_loopdevs 2
>>> +prepare_loopdevs
>>> +dev1=${loopdevs[1]}
>>> +dev2=${loopdevs[2]}
>>> +TEST_DEV=$dev1
>>> +
>>> +setup_root_helper
>>> +
>>> +run_check $SUDO_HELPERS "$TOP/mkfs.btrfs" -f $dev1
>>               ^^^^^^^^^^^^^
>>
>> It's $SUDO_HELPER, otherwise it does not work, also please quote all
>> shell variables, everywhere.

Surprise surprise, it's from my vim's context based completion.

It turns out that fsck/056 has the same problem...

Thanks,
Qu

>>
>>> +run_check $SUDO_HELPERS "$TOP/btrfstune" -S1 $dev1
>>> +run_check_mount_test_dev
>>> +run_check $SUDO_HELPERS "$TOP/btrfs" device add -f $dev2 $TEST_MNT
>>> +
>>> +# Here we can not use umount helper, as it uses the seed device to
>>> do the
>>> +# umount. We need to manually unmout using the mount point
>>> +run_check $SUDO_HELPERS umount $TEST_MNT
>>> +
>>> +seed_output=$(_mktemp --tmpdir btrfs-progs-seed-check-stdout.XXXXXX)
>>
>> This won't work as intended, there's a helper _mktemp_dir that creates a
>> temporary directory, otherwise --tmpdir is interpreted as the direcotry
>> name.
>
> My bad, it should be mktemp without the "_" prefix, just like what we
> did in all the run* helpers.
>
> And I'm not expected to create a directory, thus I don't need to
> _mktemp_dir.
>
> Thanks,
> Qu

Re: [PATCH 2/2] btrfs-progs: fsck-tests: check warning for seed and sprouted filesystems

[David Sterba]

On Tue, Apr 26, 2022 at 07:06:00AM +0800, Qu Wenruo wrote:
> >>> +prepare_loopdevs
> >>> +dev1=${loopdevs[1]}
> >>> +dev2=${loopdevs[2]}
> >>> +TEST_DEV=$dev1
> >>> +
> >>> +setup_root_helper
> >>> +
> >>> +run_check $SUDO_HELPERS "$TOP/mkfs.btrfs" -f $dev1
> >>               ^^^^^^^^^^^^^
> >>
> >> It's $SUDO_HELPER, otherwise it does not work, also please quote all
> >> shell variables, everywhere.
> 
> Surprise surprise, it's from my vim's context based completion.
> 
> It turns out that fsck/056 has the same problem...

I see, fixed as well.