[PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Qu Wenruo]

Although btrfsck test case support pure image dump(tar.xz), it is still
too large for some images, e.g, a small 64M image with about 3 levels
(level 0~2) metadata will produce about 2.6M after xz zip, which is too
large for a single binary commit.

However btrfs-image -c9 will works much finer, the above image with
btrfs-image dump will only be less than 200K, which is quite reasonable.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/fsck-tests.sh | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/tests/fsck-tests.sh b/tests/fsck-tests.sh
index 8987d04..007e5b0 100644
--- a/tests/fsck-tests.sh
+++ b/tests/fsck-tests.sh
@@ -22,16 +22,38 @@ run_check()
 	"$@" >> $RESULT 2>&1 || _fail "failed: $@"
 }
 
+# For complicated fsck repair case,
+# where even repairing is OK, it may still report problem before or after
+# reparing since the repair needs several loops to repair all the problems
+# but report checks it before all repair loops done
+run_check_no_fail()
+{
+	echo "############### $@" >> $RESULT 2>&1
+	"$@" >> $RESULT 2>&1
+}
+
 rm -f $RESULT
 
 # test rely on corrupting blocks tool
 run_check make btrfs-corrupt-block
 
+# Supported test image formats:
+# 1) btrfs-image dump(.img files)
 # Some broken filesystem images are kept as .img files, created by the tool
-# btrfs-image, and others are kept as .tar.xz files that contain raw filesystem
+# btrfs-image
+#
+# 2) binary image dump only(only test.img in .tar.xz)
+# Some are kept as .tar.xz files that contain raw filesystem
 # image (the backing file of a loop device, as a sparse file). The reason for
 # keeping some as tarballs of raw images is that for these cases btrfs-image
 # isn't able to preserve all the (bad) filesystem structure for some reason.
+# This provides great flexibility at the cost of large file size.
+#
+# 3) script generated dump(generate_image.sh + needed things in .tar.gz)
+# The image is generated by the generate_image.sh script alone the needed
+# files in the tarball, normally a quite small btrfs-image dump.
+# This one combines the advatange of relative small btrfs-image and the
+# flexibility to support corrupted image.
 for i in $(find $here/tests/fsck-tests -name '*.img' -o -name '*.tar.xz' | sort)
 do
 	echo "     [TEST]    $(basename $i)"
@@ -39,16 +61,24 @@ do
 
 	extension=${i#*.}
 
+	if [ -f generate_image.sh ]; then
+		rm generate_image.sh
+	fi
+
 	if [ $extension == "img" ]; then
 		run_check $here/btrfs-image -r $i test.img
 	else
 		run_check tar xJf $i
 	fi
 
+	if [ -x generate_image.sh ]; then
+		./generate_image.sh
+	fi
+
 	$here/btrfsck test.img >> $RESULT 2>&1
 	[ $? -eq 0 ] && _fail "btrfsck should have detected corruption"
 
-	run_check $here/btrfsck --repair test.img
+	run_check_no_fail $here/btrfsck --repair test.img
 	run_check $here/btrfsck test.img
 done
 
-- 
2.1.3

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Qu Wenruo]

It seems the second patch, which about 225K including a btrfs-image 
dump, can't pass the ml's size limit.

To David:
I created the pull request to your github repo:
https://github.com/kdave/btrfs-progs/pull/3

Thanks,
Qu

-------- Original Message --------
Subject: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt 
script fsck test case.
From: Qu Wenruo <quwenruo@cn.fujitsu.com>
To: <linux-btrfs@vger.kernel.org>
Date: 2014年12月15日 11:54
> Although btrfsck test case support pure image dump(tar.xz), it is still
> too large for some images, e.g, a small 64M image with about 3 levels
> (level 0~2) metadata will produce about 2.6M after xz zip, which is too
> large for a single binary commit.
>
> However btrfs-image -c9 will works much finer, the above image with
> btrfs-image dump will only be less than 200K, which is quite reasonable.
>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>   tests/fsck-tests.sh | 34 ++++++++++++++++++++++++++++++++--
>   1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/tests/fsck-tests.sh b/tests/fsck-tests.sh
> index 8987d04..007e5b0 100644
> --- a/tests/fsck-tests.sh
> +++ b/tests/fsck-tests.sh
> @@ -22,16 +22,38 @@ run_check()
>   	"$@" >> $RESULT 2>&1 || _fail "failed: $@"
>   }
>   
> +# For complicated fsck repair case,
> +# where even repairing is OK, it may still report problem before or after
> +# reparing since the repair needs several loops to repair all the problems
> +# but report checks it before all repair loops done
> +run_check_no_fail()
> +{
> +	echo "############### $@" >> $RESULT 2>&1
> +	"$@" >> $RESULT 2>&1
> +}
> +
>   rm -f $RESULT
>   
>   # test rely on corrupting blocks tool
>   run_check make btrfs-corrupt-block
>   
> +# Supported test image formats:
> +# 1) btrfs-image dump(.img files)
>   # Some broken filesystem images are kept as .img files, created by the tool
> -# btrfs-image, and others are kept as .tar.xz files that contain raw filesystem
> +# btrfs-image
> +#
> +# 2) binary image dump only(only test.img in .tar.xz)
> +# Some are kept as .tar.xz files that contain raw filesystem
>   # image (the backing file of a loop device, as a sparse file). The reason for
>   # keeping some as tarballs of raw images is that for these cases btrfs-image
>   # isn't able to preserve all the (bad) filesystem structure for some reason.
> +# This provides great flexibility at the cost of large file size.
> +#
> +# 3) script generated dump(generate_image.sh + needed things in .tar.gz)
> +# The image is generated by the generate_image.sh script alone the needed
> +# files in the tarball, normally a quite small btrfs-image dump.
> +# This one combines the advatange of relative small btrfs-image and the
> +# flexibility to support corrupted image.
>   for i in $(find $here/tests/fsck-tests -name '*.img' -o -name '*.tar.xz' | sort)
>   do
>   	echo "     [TEST]    $(basename $i)"
> @@ -39,16 +61,24 @@ do
>   
>   	extension=${i#*.}
>   
> +	if [ -f generate_image.sh ]; then
> +		rm generate_image.sh
> +	fi
> +
>   	if [ $extension == "img" ]; then
>   		run_check $here/btrfs-image -r $i test.img
>   	else
>   		run_check tar xJf $i
>   	fi
>   
> +	if [ -x generate_image.sh ]; then
> +		./generate_image.sh
> +	fi
> +
>   	$here/btrfsck test.img >> $RESULT 2>&1
>   	[ $? -eq 0 ] && _fail "btrfsck should have detected corruption"
>   
> -	run_check $here/btrfsck --repair test.img
> +	run_check_no_fail $here/btrfsck --repair test.img
>   	run_check $here/btrfsck test.img
>   done
>   

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[David Sterba]

On Mon, Dec 15, 2014 at 02:09:02PM +0800, Qu Wenruo wrote:
> It seems the second patch, which about 225K including a btrfs-image 
> dump, can't pass the ml's size limit.

I've cherry-picked the commit with the test image.

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Filipe David Manana]

On Mon, Dec 15, 2014 at 3:54 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
> Although btrfsck test case support pure image dump(tar.xz), it is still
> too large for some images, e.g, a small 64M image with about 3 levels
> (level 0~2) metadata will produce about 2.6M after xz zip, which is too
> large for a single binary commit.
>
> However btrfs-image -c9 will works much finer, the above image with
> btrfs-image dump will only be less than 200K, which is quite reasonable.
>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>  tests/fsck-tests.sh | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/tests/fsck-tests.sh b/tests/fsck-tests.sh
> index 8987d04..007e5b0 100644
> --- a/tests/fsck-tests.sh
> +++ b/tests/fsck-tests.sh
> @@ -22,16 +22,38 @@ run_check()
>         "$@" >> $RESULT 2>&1 || _fail "failed: $@"
>  }
>
> +# For complicated fsck repair case,
> +# where even repairing is OK, it may still report problem before or after
> +# reparing since the repair needs several loops to repair all the problems
> +# but report checks it before all repair loops done
> +run_check_no_fail()
> +{
> +       echo "############### $@" >> $RESULT 2>&1
> +       "$@" >> $RESULT 2>&1
> +}

I'm confused with this function, why it's needed and the respective comment.
So I can interpret it as either:

1) The several loops means fsck --repair does multiple passages
internally to fix some issues?
 If this is the case, we (user or script) only need to call fsck
--repair once, which should exit with status 0 if it was able to fix
all the issues, right? If so, then we should check that fsck --repair
exits with status 0, removing the need for this new function.

2) The several loops means a user or script must call fsck --repair
multiple times to fix all the issues? If this is the case then you're
only calling this function once, for a single fsck --repair, in the
code below, which confuses me and it makes this new function redundant
too.

Thanks

> +
>  rm -f $RESULT
>
>  # test rely on corrupting blocks tool
>  run_check make btrfs-corrupt-block
>
> +# Supported test image formats:
> +# 1) btrfs-image dump(.img files)
>  # Some broken filesystem images are kept as .img files, created by the tool
> -# btrfs-image, and others are kept as .tar.xz files that contain raw filesystem
> +# btrfs-image
> +#
> +# 2) binary image dump only(only test.img in .tar.xz)
> +# Some are kept as .tar.xz files that contain raw filesystem
>  # image (the backing file of a loop device, as a sparse file). The reason for
>  # keeping some as tarballs of raw images is that for these cases btrfs-image
>  # isn't able to preserve all the (bad) filesystem structure for some reason.
> +# This provides great flexibility at the cost of large file size.
> +#
> +# 3) script generated dump(generate_image.sh + needed things in .tar.gz)
> +# The image is generated by the generate_image.sh script alone the needed
> +# files in the tarball, normally a quite small btrfs-image dump.
> +# This one combines the advatange of relative small btrfs-image and the
> +# flexibility to support corrupted image.
>  for i in $(find $here/tests/fsck-tests -name '*.img' -o -name '*.tar.xz' | sort)
>  do
>         echo "     [TEST]    $(basename $i)"
> @@ -39,16 +61,24 @@ do
>
>         extension=${i#*.}
>
> +       if [ -f generate_image.sh ]; then
> +               rm generate_image.sh
> +       fi
> +
>         if [ $extension == "img" ]; then
>                 run_check $here/btrfs-image -r $i test.img
>         else
>                 run_check tar xJf $i
>         fi
>
> +       if [ -x generate_image.sh ]; then
> +               ./generate_image.sh
> +       fi
> +
>         $here/btrfsck test.img >> $RESULT 2>&1
>         [ $? -eq 0 ] && _fail "btrfsck should have detected corruption"
>
> -       run_check $here/btrfsck --repair test.img
> +       run_check_no_fail $here/btrfsck --repair test.img
>         run_check $here/btrfsck test.img
>  done
>
> --
> 2.1.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + 
corrupt script fsck test case.
From: Filipe David Manana <fdmanana@gmail.com>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年12月15日 17:00
> On Mon, Dec 15, 2014 at 3:54 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>> Although btrfsck test case support pure image dump(tar.xz), it is still
>> too large for some images, e.g, a small 64M image with about 3 levels
>> (level 0~2) metadata will produce about 2.6M after xz zip, which is too
>> large for a single binary commit.
>>
>> However btrfs-image -c9 will works much finer, the above image with
>> btrfs-image dump will only be less than 200K, which is quite reasonable.
>>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>   tests/fsck-tests.sh | 34 ++++++++++++++++++++++++++++++++--
>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/fsck-tests.sh b/tests/fsck-tests.sh
>> index 8987d04..007e5b0 100644
>> --- a/tests/fsck-tests.sh
>> +++ b/tests/fsck-tests.sh
>> @@ -22,16 +22,38 @@ run_check()
>>          "$@" >> $RESULT 2>&1 || _fail "failed: $@"
>>   }
>>
>> +# For complicated fsck repair case,
>> +# where even repairing is OK, it may still report problem before or after
>> +# reparing since the repair needs several loops to repair all the problems
>> +# but report checks it before all repair loops done
>> +run_check_no_fail()
>> +{
>> +       echo "############### $@" >> $RESULT 2>&1
>> +       "$@" >> $RESULT 2>&1
>> +}
> I'm confused with this function, why it's needed and the respective comment.
> So I can interpret it as either:
>
> 1) The several loops means fsck --repair does multiple passages
> internally to fix some issues?
>   If this is the case, we (user or script) only need to call fsck
> --repair once, which should exit with status 0 if it was able to fix
> all the issues, right? If so, then we should check that fsck --repair
> exits with status 0, removing the need for this new function.
Sorry for the poor explain.

The problem is, there is some check cases before we doing repair and 
these check result is bad so
btrfsck thinks there is err even it will be repaired later.

So The result is, especially on corrupted-leaf case, btrfsck --repair 
will fix all the problems but
still return 1, and the next btrfsck without --repair will return 0.

I think there are still  a lot of other cases causing things like 
this(multiple different errors combines together)
so just discard the return value of btrfsck --repair and focus on the 
second btrfsck return value.

Thanks,
Qu
>
> 2) The several loops means a user or script must call fsck --repair
> multiple times to fix all the issues? If this is the case then you're
> only calling this function once, for a single fsck --repair, in the
> code below, which confuses me and it makes this new function redundant
> too.
>
> Thanks
>
>> +
>>   rm -f $RESULT
>>
>>   # test rely on corrupting blocks tool
>>   run_check make btrfs-corrupt-block
>>
>> +# Supported test image formats:
>> +# 1) btrfs-image dump(.img files)
>>   # Some broken filesystem images are kept as .img files, created by the tool
>> -# btrfs-image, and others are kept as .tar.xz files that contain raw filesystem
>> +# btrfs-image
>> +#
>> +# 2) binary image dump only(only test.img in .tar.xz)
>> +# Some are kept as .tar.xz files that contain raw filesystem
>>   # image (the backing file of a loop device, as a sparse file). The reason for
>>   # keeping some as tarballs of raw images is that for these cases btrfs-image
>>   # isn't able to preserve all the (bad) filesystem structure for some reason.
>> +# This provides great flexibility at the cost of large file size.
>> +#
>> +# 3) script generated dump(generate_image.sh + needed things in .tar.gz)
>> +# The image is generated by the generate_image.sh script alone the needed
>> +# files in the tarball, normally a quite small btrfs-image dump.
>> +# This one combines the advatange of relative small btrfs-image and the
>> +# flexibility to support corrupted image.
>>   for i in $(find $here/tests/fsck-tests -name '*.img' -o -name '*.tar.xz' | sort)
>>   do
>>          echo "     [TEST]    $(basename $i)"
>> @@ -39,16 +61,24 @@ do
>>
>>          extension=${i#*.}
>>
>> +       if [ -f generate_image.sh ]; then
>> +               rm generate_image.sh
>> +       fi
>> +
>>          if [ $extension == "img" ]; then
>>                  run_check $here/btrfs-image -r $i test.img
>>          else
>>                  run_check tar xJf $i
>>          fi
>>
>> +       if [ -x generate_image.sh ]; then
>> +               ./generate_image.sh
>> +       fi
>> +
>>          $here/btrfsck test.img >> $RESULT 2>&1
>>          [ $? -eq 0 ] && _fail "btrfsck should have detected corruption"
>>
>> -       run_check $here/btrfsck --repair test.img
>> +       run_check_no_fail $here/btrfsck --repair test.img
>>          run_check $here/btrfsck test.img
>>   done
>>
>> --
>> 2.1.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Filipe David Manana]

On Mon, Dec 15, 2014 at 9:40 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>
> -------- Original Message --------
> Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt
> script fsck test case.
> From: Filipe David Manana <fdmanana@gmail.com>
> To: Qu Wenruo <quwenruo@cn.fujitsu.com>
> Date: 2014年12月15日 17:00
>>
>> On Mon, Dec 15, 2014 at 3:54 AM, Qu Wenruo <quwenruo@cn.fujitsu.com>
>> wrote:
>>>
>>> Although btrfsck test case support pure image dump(tar.xz), it is still
>>> too large for some images, e.g, a small 64M image with about 3 levels
>>> (level 0~2) metadata will produce about 2.6M after xz zip, which is too
>>> large for a single binary commit.
>>>
>>> However btrfs-image -c9 will works much finer, the above image with
>>> btrfs-image dump will only be less than 200K, which is quite reasonable.
>>>
>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>> ---
>>>   tests/fsck-tests.sh | 34 ++++++++++++++++++++++++++++++++--
>>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/fsck-tests.sh b/tests/fsck-tests.sh
>>> index 8987d04..007e5b0 100644
>>> --- a/tests/fsck-tests.sh
>>> +++ b/tests/fsck-tests.sh
>>> @@ -22,16 +22,38 @@ run_check()
>>>          "$@" >> $RESULT 2>&1 || _fail "failed: $@"
>>>   }
>>>
>>> +# For complicated fsck repair case,
>>> +# where even repairing is OK, it may still report problem before or
>>> after
>>> +# reparing since the repair needs several loops to repair all the
>>> problems
>>> +# but report checks it before all repair loops done
>>> +run_check_no_fail()
>>> +{
>>> +       echo "############### $@" >> $RESULT 2>&1
>>> +       "$@" >> $RESULT 2>&1
>>> +}
>>
>> I'm confused with this function, why it's needed and the respective
>> comment.
>> So I can interpret it as either:
>>
>> 1) The several loops means fsck --repair does multiple passages
>> internally to fix some issues?
>>   If this is the case, we (user or script) only need to call fsck
>> --repair once, which should exit with status 0 if it was able to fix
>> all the issues, right? If so, then we should check that fsck --repair
>> exits with status 0, removing the need for this new function.
>
> Sorry for the poor explain.
>
> The problem is, there is some check cases before we doing repair and these
> check result is bad so
> btrfsck thinks there is err even it will be repaired later.
>
> So The result is, especially on corrupted-leaf case, btrfsck --repair will
> fix all the problems but
> still return 1, and the next btrfsck without --repair will return 0.

That seems wrong to me. If --repair was able to fix all problems it
should exit with status 0.
If a script is running fsck --repair it would incorrectly assume
--repair failed.

>
> I think there are still  a lot of other cases causing things like
> this(multiple different errors combines together)
> so just discard the return value of btrfsck --repair and focus on the second
> btrfsck return value.
>
> Thanks,
> Qu
>
>>
>> 2) The several loops means a user or script must call fsck --repair
>> multiple times to fix all the issues? If this is the case then you're
>> only calling this function once, for a single fsck --repair, in the
>> code below, which confuses me and it makes this new function redundant
>> too.
>>
>> Thanks
>>
>>> +
>>>   rm -f $RESULT
>>>
>>>   # test rely on corrupting blocks tool
>>>   run_check make btrfs-corrupt-block
>>>
>>> +# Supported test image formats:
>>> +# 1) btrfs-image dump(.img files)
>>>   # Some broken filesystem images are kept as .img files, created by the
>>> tool
>>> -# btrfs-image, and others are kept as .tar.xz files that contain raw
>>> filesystem
>>> +# btrfs-image
>>> +#
>>> +# 2) binary image dump only(only test.img in .tar.xz)
>>> +# Some are kept as .tar.xz files that contain raw filesystem
>>>   # image (the backing file of a loop device, as a sparse file). The
>>> reason for
>>>   # keeping some as tarballs of raw images is that for these cases
>>> btrfs-image
>>>   # isn't able to preserve all the (bad) filesystem structure for some
>>> reason.
>>> +# This provides great flexibility at the cost of large file size.
>>> +#
>>> +# 3) script generated dump(generate_image.sh + needed things in .tar.gz)
>>> +# The image is generated by the generate_image.sh script alone the
>>> needed
>>> +# files in the tarball, normally a quite small btrfs-image dump.
>>> +# This one combines the advatange of relative small btrfs-image and the
>>> +# flexibility to support corrupted image.
>>>   for i in $(find $here/tests/fsck-tests -name '*.img' -o -name
>>> '*.tar.xz' | sort)
>>>   do
>>>          echo "     [TEST]    $(basename $i)"
>>> @@ -39,16 +61,24 @@ do
>>>
>>>          extension=${i#*.}
>>>
>>> +       if [ -f generate_image.sh ]; then
>>> +               rm generate_image.sh
>>> +       fi
>>> +
>>>          if [ $extension == "img" ]; then
>>>                  run_check $here/btrfs-image -r $i test.img
>>>          else
>>>                  run_check tar xJf $i
>>>          fi
>>>
>>> +       if [ -x generate_image.sh ]; then
>>> +               ./generate_image.sh
>>> +       fi
>>> +
>>>          $here/btrfsck test.img >> $RESULT 2>&1
>>>          [ $? -eq 0 ] && _fail "btrfsck should have detected corruption"
>>>
>>> -       run_check $here/btrfsck --repair test.img
>>> +       run_check_no_fail $here/btrfsck --repair test.img
>>>          run_check $here/btrfsck test.img
>>>   done
>>>
>>> --
>>> 2.1.3
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + 
corrupt script fsck test case.
From: Filipe David Manana <fdmanana@gmail.com>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年12月15日 17:43
> On Mon, Dec 15, 2014 at 9:40 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>> -------- Original Message --------
>> Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt
>> script fsck test case.
>> From: Filipe David Manana <fdmanana@gmail.com>
>> To: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> Date: 2014年12月15日 17:00
>>> On Mon, Dec 15, 2014 at 3:54 AM, Qu Wenruo <quwenruo@cn.fujitsu.com>
>>> wrote:
>>>> Although btrfsck test case support pure image dump(tar.xz), it is still
>>>> too large for some images, e.g, a small 64M image with about 3 levels
>>>> (level 0~2) metadata will produce about 2.6M after xz zip, which is too
>>>> large for a single binary commit.
>>>>
>>>> However btrfs-image -c9 will works much finer, the above image with
>>>> btrfs-image dump will only be less than 200K, which is quite reasonable.
>>>>
>>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>>> ---
>>>>    tests/fsck-tests.sh | 34 ++++++++++++++++++++++++++++++++--
>>>>    1 file changed, 32 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/fsck-tests.sh b/tests/fsck-tests.sh
>>>> index 8987d04..007e5b0 100644
>>>> --- a/tests/fsck-tests.sh
>>>> +++ b/tests/fsck-tests.sh
>>>> @@ -22,16 +22,38 @@ run_check()
>>>>           "$@" >> $RESULT 2>&1 || _fail "failed: $@"
>>>>    }
>>>>
>>>> +# For complicated fsck repair case,
>>>> +# where even repairing is OK, it may still report problem before or
>>>> after
>>>> +# reparing since the repair needs several loops to repair all the
>>>> problems
>>>> +# but report checks it before all repair loops done
>>>> +run_check_no_fail()
>>>> +{
>>>> +       echo "############### $@" >> $RESULT 2>&1
>>>> +       "$@" >> $RESULT 2>&1
>>>> +}
>>> I'm confused with this function, why it's needed and the respective
>>> comment.
>>> So I can interpret it as either:
>>>
>>> 1) The several loops means fsck --repair does multiple passages
>>> internally to fix some issues?
>>>    If this is the case, we (user or script) only need to call fsck
>>> --repair once, which should exit with status 0 if it was able to fix
>>> all the issues, right? If so, then we should check that fsck --repair
>>> exits with status 0, removing the need for this new function.
>> Sorry for the poor explain.
>>
>> The problem is, there is some check cases before we doing repair and these
>> check result is bad so
>> btrfsck thinks there is err even it will be repaired later.
>>
>> So The result is, especially on corrupted-leaf case, btrfsck --repair will
>> fix all the problems but
>> still return 1, and the next btrfsck without --repair will return 0.
> That seems wrong to me. If --repair was able to fix all problems it
> should exit with status 0.
> If a script is running fsck --repair it would incorrectly assume
> --repair failed.
That's right, I'll look into it and try to fix the return value things 
before I send the v2 test case.

Thanks,
Qu

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Filipe David Manana]

On Mon, Dec 15, 2014 at 3:54 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
> Although btrfsck test case support pure image dump(tar.xz), it is still
> too large for some images, e.g, a small 64M image with about 3 levels
> (level 0~2) metadata will produce about 2.6M after xz zip, which is too
> large for a single binary commit.
>
> However btrfs-image -c9 will works much finer, the above image with
> btrfs-image dump will only be less than 200K, which is quite reasonable.
>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>  tests/fsck-tests.sh | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/tests/fsck-tests.sh b/tests/fsck-tests.sh
> index 8987d04..007e5b0 100644
> --- a/tests/fsck-tests.sh
> +++ b/tests/fsck-tests.sh
> @@ -22,16 +22,38 @@ run_check()
>         "$@" >> $RESULT 2>&1 || _fail "failed: $@"
>  }
>
> +# For complicated fsck repair case,
> +# where even repairing is OK, it may still report problem before or after
> +# reparing since the repair needs several loops to repair all the problems
> +# but report checks it before all repair loops done
> +run_check_no_fail()
> +{
> +       echo "############### $@" >> $RESULT 2>&1
> +       "$@" >> $RESULT 2>&1
> +}
> +
>  rm -f $RESULT
>
>  # test rely on corrupting blocks tool
>  run_check make btrfs-corrupt-block
>
> +# Supported test image formats:
> +# 1) btrfs-image dump(.img files)
>  # Some broken filesystem images are kept as .img files, created by the tool
> -# btrfs-image, and others are kept as .tar.xz files that contain raw filesystem
> +# btrfs-image
> +#
> +# 2) binary image dump only(only test.img in .tar.xz)
> +# Some are kept as .tar.xz files that contain raw filesystem
>  # image (the backing file of a loop device, as a sparse file). The reason for
>  # keeping some as tarballs of raw images is that for these cases btrfs-image
>  # isn't able to preserve all the (bad) filesystem structure for some reason.
> +# This provides great flexibility at the cost of large file size.
> +#
> +# 3) script generated dump(generate_image.sh + needed things in .tar.gz)
> +# The image is generated by the generate_image.sh script alone the needed
> +# files in the tarball, normally a quite small btrfs-image dump.
> +# This one combines the advatange of relative small btrfs-image and the
> +# flexibility to support corrupted image.
>  for i in $(find $here/tests/fsck-tests -name '*.img' -o -name '*.tar.xz' | sort)
>  do
>         echo "     [TEST]    $(basename $i)"
> @@ -39,16 +61,24 @@ do
>
>         extension=${i#*.}
>
> +       if [ -f generate_image.sh ]; then
> +               rm generate_image.sh
> +       fi
> +
>         if [ $extension == "img" ]; then
>                 run_check $here/btrfs-image -r $i test.img
>         else
>                 run_check tar xJf $i
>         fi
>
> +       if [ -x generate_image.sh ]; then
> +               ./generate_image.sh
> +       fi
> +
>         $here/btrfsck test.img >> $RESULT 2>&1
>         [ $? -eq 0 ] && _fail "btrfsck should have detected corruption"
>
> -       run_check $here/btrfsck --repair test.img
> +       run_check_no_fail $here/btrfsck --repair test.img
>         run_check $here/btrfsck test.img
>  done

So another thing I would like to see is doing a more comprehensive
verification that the repair code worked as expected. Currently we
only check that a readonly fsck, after running fsck --repair, returns
0.

For the improvements you've been doing, it's equally important to
verify that --repair recovered the inodes, links, etc to the
lost+found directory (or whatever is the directory's name).

So perhaps adding a verify.sh script to the tarball for example?

Thanks

>
> --
> 2.1.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Filipe David Manana]

On Mon, Dec 15, 2014 at 9:36 AM, Filipe David Manana <fdmanana@gmail.com> wrote:
> On Mon, Dec 15, 2014 at 3:54 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>> Although btrfsck test case support pure image dump(tar.xz), it is still
>> too large for some images, e.g, a small 64M image with about 3 levels
>> (level 0~2) metadata will produce about 2.6M after xz zip, which is too
>> large for a single binary commit.
>>
>> However btrfs-image -c9 will works much finer, the above image with
>> btrfs-image dump will only be less than 200K, which is quite reasonable.
>>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>  tests/fsck-tests.sh | 34 ++++++++++++++++++++++++++++++++--
>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/fsck-tests.sh b/tests/fsck-tests.sh
>> index 8987d04..007e5b0 100644
>> --- a/tests/fsck-tests.sh
>> +++ b/tests/fsck-tests.sh
>> @@ -22,16 +22,38 @@ run_check()
>>         "$@" >> $RESULT 2>&1 || _fail "failed: $@"
>>  }
>>
>> +# For complicated fsck repair case,
>> +# where even repairing is OK, it may still report problem before or after
>> +# reparing since the repair needs several loops to repair all the problems
>> +# but report checks it before all repair loops done
>> +run_check_no_fail()
>> +{
>> +       echo "############### $@" >> $RESULT 2>&1
>> +       "$@" >> $RESULT 2>&1
>> +}
>> +
>>  rm -f $RESULT
>>
>>  # test rely on corrupting blocks tool
>>  run_check make btrfs-corrupt-block
>>
>> +# Supported test image formats:
>> +# 1) btrfs-image dump(.img files)
>>  # Some broken filesystem images are kept as .img files, created by the tool
>> -# btrfs-image, and others are kept as .tar.xz files that contain raw filesystem
>> +# btrfs-image
>> +#
>> +# 2) binary image dump only(only test.img in .tar.xz)
>> +# Some are kept as .tar.xz files that contain raw filesystem
>>  # image (the backing file of a loop device, as a sparse file). The reason for
>>  # keeping some as tarballs of raw images is that for these cases btrfs-image
>>  # isn't able to preserve all the (bad) filesystem structure for some reason.
>> +# This provides great flexibility at the cost of large file size.
>> +#
>> +# 3) script generated dump(generate_image.sh + needed things in .tar.gz)
>> +# The image is generated by the generate_image.sh script alone the needed
>> +# files in the tarball, normally a quite small btrfs-image dump.
>> +# This one combines the advatange of relative small btrfs-image and the
>> +# flexibility to support corrupted image.
>>  for i in $(find $here/tests/fsck-tests -name '*.img' -o -name '*.tar.xz' | sort)
>>  do
>>         echo "     [TEST]    $(basename $i)"
>> @@ -39,16 +61,24 @@ do
>>
>>         extension=${i#*.}
>>
>> +       if [ -f generate_image.sh ]; then
>> +               rm generate_image.sh
>> +       fi
>> +
>>         if [ $extension == "img" ]; then
>>                 run_check $here/btrfs-image -r $i test.img
>>         else
>>                 run_check tar xJf $i
>>         fi
>>
>> +       if [ -x generate_image.sh ]; then
>> +               ./generate_image.sh
>> +       fi
>> +
>>         $here/btrfsck test.img >> $RESULT 2>&1
>>         [ $? -eq 0 ] && _fail "btrfsck should have detected corruption"
>>
>> -       run_check $here/btrfsck --repair test.img
>> +       run_check_no_fail $here/btrfsck --repair test.img
>>         run_check $here/btrfsck test.img
>>  done
>
> So another thing I would like to see is doing a more comprehensive
> verification that the repair code worked as expected. Currently we
> only check that a readonly fsck, after running fsck --repair, returns
> 0.
>
> For the improvements you've been doing, it's equally important to
> verify that --repair recovered the inodes, links, etc to the
> lost+found directory (or whatever is the directory's name).
>
> So perhaps adding a verify.sh script to the tarball for example?

Or, forgot before, it might be better to do such verification/test in
xfstests since we can create the fs and use the new btrfs-progs
programs to corrupt leafs/nodes. xfstests has a lot of infrastructure
already and probably run by a lot more people (compared to the fsck
tests of btrfs-progs).

>
> Thanks
>
>>
>> --
>> 2.1.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Filipe David Manana,
>
> "Reasonable men adapt themselves to the world.
>  Unreasonable men adapt the world to themselves.
>  That's why all progress depends on unreasonable men."



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[David Sterba]

On Mon, Dec 15, 2014 at 10:13:45AM +0000, Filipe David Manana wrote:
> >
> > So another thing I would like to see is doing a more comprehensive
> > verification that the repair code worked as expected. Currently we
> > only check that a readonly fsck, after running fsck --repair, returns
> > 0.
> >
> > For the improvements you've been doing, it's equally important to
> > verify that --repair recovered the inodes, links, etc to the
> > lost+found directory (or whatever is the directory's name).
> >
> > So perhaps adding a verify.sh script to the tarball for example?
> 
> Or, forgot before, it might be better to do such verification/test in
> xfstests since we can create the fs and use the new btrfs-progs
> programs to corrupt leafs/nodes. xfstests has a lot of infrastructure
> already and probably run by a lot more people (compared to the fsck
> tests of btrfs-progs).

I'm thinking about the best way how to integrate that, but it seems that
there will be always some level of code or infrastructure duplication
(or other hassle).

btrfs-corrupt-block is not installed by default (make install) and it's
not a type of utility I'd consider for default installations. The tests
would be skipped in absence of the utility, so there will be test
environments where "install xfstests, install btrfspprogs" will not add
the desired test coverage. Solvable by packaging the extra progs.

Adding corrupt-block into xfsprogs is infeasible (IMO too much code from
btrfs-progs to be added).

I don't know how much infrastructure code we'd have to either write or
copy from fstests, but I think it would not be that much. Ideally we
could write the tests within btrfs-progs and then submit them to fstests
once they're considered reliable. If we keep the same "syntax" of the
tests, provide stubs where applicable, the code duplication in test
itself would be zero. We'd only have to write the stubs in btrfs-progs
and probably extend fstests to provide helpers for preparing/unpacking
the images.

Also, collecting the crafted binary images may bloat the git repo
nicely, even if we use xz.

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + 
corrupt script fsck test case.
From: David Sterba <dsterba@suse.cz>
To: Filipe David Manana <fdmanana@gmail.com>
Date: 2014年12月16日 02:19
> On Mon, Dec 15, 2014 at 10:13:45AM +0000, Filipe David Manana wrote:
>>> So another thing I would like to see is doing a more comprehensive
>>> verification that the repair code worked as expected. Currently we
>>> only check that a readonly fsck, after running fsck --repair, returns
>>> 0.
>>>
>>> For the improvements you've been doing, it's equally important to
>>> verify that --repair recovered the inodes, links, etc to the
>>> lost+found directory (or whatever is the directory's name).
>>>
>>> So perhaps adding a verify.sh script to the tarball for example?
>> Or, forgot before, it might be better to do such verification/test in
>> xfstests since we can create the fs and use the new btrfs-progs
>> programs to corrupt leafs/nodes. xfstests has a lot of infrastructure
>> already and probably run by a lot more people (compared to the fsck
>> tests of btrfs-progs).
> I'm thinking about the best way how to integrate that, but it seems that
> there will be always some level of code or infrastructure duplication
> (or other hassle).
>
> btrfs-corrupt-block is not installed by default (make install) and it's
> not a type of utility I'd consider for default installations. The tests
> would be skipped in absence of the utility, so there will be test
> environments where "install xfstests, install btrfspprogs" will not add
> the desired test coverage. Solvable by packaging the extra progs.
>
> Adding corrupt-block into xfsprogs is infeasible (IMO too much code from
> btrfs-progs to be added).
>
> I don't know how much infrastructure code we'd have to either write or
> copy from fstests, but I think it would not be that much. Ideally we
> could write the tests within btrfs-progs and then submit them to fstests
> once they're considered reliable. If we keep the same "syntax" of the
> tests, provide stubs where applicable, the code duplication in test
> itself would be zero. We'd only have to write the stubs in btrfs-progs
> and probably extend fstests to provide helpers for preparing/unpacking
> the images.
In my wildest idea, if we have a good enough btrfs debugger(maybe even 
stronger than debugfs), which can
do almost everything from read key/item to corrupt given structure, then 
we can resolve them all.
No binary image since corruption can be done by it and verify can also 
done by it.
(OK, it's just a daydream)

But IMHO, isn't xfstests designed to mainly detect kernel defeats?
I don't see any fsck tool test case in it.

Thanks,
Qu
>
> Also, collecting the crafted binary images may bloat the git repo
> nicely, even if we use xz.

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Filipe David Manana]

On Tue, Dec 16, 2014 at 1:35 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>
> -------- Original Message --------
> Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt
> script fsck test case.
> From: David Sterba <dsterba@suse.cz>
> To: Filipe David Manana <fdmanana@gmail.com>
> Date: 2014年12月16日 02:19
>>
>> On Mon, Dec 15, 2014 at 10:13:45AM +0000, Filipe David Manana wrote:
>>>>
>>>> So another thing I would like to see is doing a more comprehensive
>>>> verification that the repair code worked as expected. Currently we
>>>> only check that a readonly fsck, after running fsck --repair, returns
>>>> 0.
>>>>
>>>> For the improvements you've been doing, it's equally important to
>>>> verify that --repair recovered the inodes, links, etc to the
>>>> lost+found directory (or whatever is the directory's name).
>>>>
>>>> So perhaps adding a verify.sh script to the tarball for example?
>>>
>>> Or, forgot before, it might be better to do such verification/test in
>>> xfstests since we can create the fs and use the new btrfs-progs
>>> programs to corrupt leafs/nodes. xfstests has a lot of infrastructure
>>> already and probably run by a lot more people (compared to the fsck
>>> tests of btrfs-progs).
>>
>> I'm thinking about the best way how to integrate that, but it seems that
>> there will be always some level of code or infrastructure duplication
>> (or other hassle).
>>
>> btrfs-corrupt-block is not installed by default (make install) and it's
>> not a type of utility I'd consider for default installations. The tests
>> would be skipped in absence of the utility, so there will be test
>> environments where "install xfstests, install btrfspprogs" will not add
>> the desired test coverage. Solvable by packaging the extra progs.
>>
>> Adding corrupt-block into xfsprogs is infeasible (IMO too much code from
>> btrfs-progs to be added).
>>
>> I don't know how much infrastructure code we'd have to either write or
>> copy from fstests, but I think it would not be that much. Ideally we
>> could write the tests within btrfs-progs and then submit them to fstests
>> once they're considered reliable. If we keep the same "syntax" of the
>> tests, provide stubs where applicable, the code duplication in test
>> itself would be zero. We'd only have to write the stubs in btrfs-progs
>> and probably extend fstests to provide helpers for preparing/unpacking
>> the images.
>
> In my wildest idea, if we have a good enough btrfs debugger(maybe even
> stronger than debugfs), which can
> do almost everything from read key/item to corrupt given structure, then we
> can resolve them all.
> No binary image since corruption can be done by it and verify can also done
> by it.
> (OK, it's just a daydream)
>
> But IMHO, isn't xfstests designed to mainly detect kernel defeats?
> I don't see any fsck tool test case in it.

I don't think xfstests is specific to test the kernel implementation
of filesystems. I believe it includes user space code too, but I might
be wrong so I'm CCing fstests and Dave to get an authoritative answer.

And I don't see a big problem with btrfs-corrupt not being built by
default when running "make" on btrfs-progs. We can make the xfstest
not run and print an informative message if the btrfs-corrupt program
isn't available - several xfstests do this, they require some
executable which isn't either in the xfstests nor xfsprogs
repositories - for example generic/241 which requires 'dbench' and
generic/299 which requires 'fio'.

Sure it would be ideal not requiring people to go into btrfs-progs,
type "make btrfs-corrupt" and put in a $PATH directory. But that's
certainly much better than not having automated tests, and I think
even with that extra "work" developers at least would be willing to do
it in their test machines. I also have a slight preference to get all
tests in the same place (xfstests) rather than in multiple
repositories (btrfs-progs, xfstests).

thanks

>
> Thanks,
> Qu
>
>>
>> Also, collecting the crafted binary images may bloat the git repo
>> nicely, even if we use xz.



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Dave Chinner]

[ Sorry to take some time to get to this, it got caught by a spam
filter and I only just noticed. ]

On Tue, Dec 16, 2014 at 02:08:53PM +0000, Filipe David Manana wrote:
> On Tue, Dec 16, 2014 at 1:35 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
> >
> > -------- Original Message --------
> > Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt
> > script fsck test case.
> > From: David Sterba <dsterba@suse.cz>
> > To: Filipe David Manana <fdmanana@gmail.com>
> > Date: 2014年12月16日 02:19
> >>
> >> On Mon, Dec 15, 2014 at 10:13:45AM +0000, Filipe David Manana wrote:
> >>>>
> >>>> So another thing I would like to see is doing a more comprehensive
> >>>> verification that the repair code worked as expected. Currently we
> >>>> only check that a readonly fsck, after running fsck --repair, returns
> >>>> 0.
> >>>>
> >>>> For the improvements you've been doing, it's equally important to
> >>>> verify that --repair recovered the inodes, links, etc to the
> >>>> lost+found directory (or whatever is the directory's name).
> >>>>
> >>>> So perhaps adding a verify.sh script to the tarball for example?
> >>>
> >>> Or, forgot before, it might be better to do such verification/test in
> >>> xfstests since we can create the fs and use the new btrfs-progs
> >>> programs to corrupt leafs/nodes. xfstests has a lot of infrastructure
> >>> already and probably run by a lot more people (compared to the fsck
> >>> tests of btrfs-progs).
> >>
> >> I'm thinking about the best way how to integrate that, but it seems that
> >> there will be always some level of code or infrastructure duplication
> >> (or other hassle).
> >>
> >> btrfs-corrupt-block is not installed by default (make install) and it's
> >> not a type of utility I'd consider for default installations. The tests
> >> would be skipped in absence of the utility, so there will be test
> >> environments where "install xfstests, install btrfspprogs" will not add
> >> the desired test coverage. Solvable by packaging the extra progs.
> >>
> >> Adding corrupt-block into xfsprogs is infeasible (IMO too much code from
> >> btrfs-progs to be added).
> >>
> >> I don't know how much infrastructure code we'd have to either write or
> >> copy from fstests, but I think it would not be that much. Ideally we
> >> could write the tests within btrfs-progs and then submit them to fstests
> >> once they're considered reliable. If we keep the same "syntax" of the
> >> tests, provide stubs where applicable, the code duplication in test
> >> itself would be zero. We'd only have to write the stubs in btrfs-progs
> >> and probably extend fstests to provide helpers for preparing/unpacking
> >> the images.
> >
> > In my wildest idea, if we have a good enough btrfs debugger(maybe even
> > stronger than debugfs), which can
> > do almost everything from read key/item to corrupt given structure, then we
> > can resolve them all.
> > No binary image since corruption can be done by it and verify can also done
> > by it.
> > (OK, it's just a daydream)
> >
> > But IMHO, isn't xfstests designed to mainly detect kernel defeats?
> > I don't see any fsck tool test case in it.
> 
> I don't think xfstests is specific to test the kernel implementation
> of filesystems. I believe it includes user space code too, but I might
> be wrong so I'm CCing fstests and Dave to get an authoritative answer.

We use fstests to test everything we ship for XFS - kernel and
userspace. i.e. we have tests that corrupt filesystems with xfs_db
and then test that xfs_repair can fix them, and once fixed the
filesystem can be mounted and used by the kernel...

i.e. fstests is for testing both the kernel code and the utilities
used to manipulate filesystems.

> And I don't see a big problem with btrfs-corrupt not being built by
> default when running "make" on btrfs-progs. We can make the xfstest
> not run and print an informative message if the btrfs-corrupt program
> isn't available - several xfstests do this, they require some
> executable which isn't either in the xfstests nor xfsprogs
> repositories - for example generic/241 which requires 'dbench' and
> generic/299 which requires 'fio'.

_require_btrfs_corrupt_prog()

Just like we do with lots of other optional userspace tools that are
required for various tests to run.

> I also have a slight preference to get all
> tests in the same place (xfstests) rather than in multiple
> repositories (btrfs-progs, xfstests).

Definitely my preference as well.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + 
corrupt script fsck test case.
From: Dave Chinner <david@fromorbit.com>
To: Filipe David Manana <fdmanana@gmail.com>
Date: 2014年12月24日 08:03
> [ Sorry to take some time to get to this, it got caught by a spam
> filter and I only just noticed. ]
>
> On Tue, Dec 16, 2014 at 02:08:53PM +0000, Filipe David Manana wrote:
>> On Tue, Dec 16, 2014 at 1:35 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>>> -------- Original Message --------
>>> Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt
>>> script fsck test case.
>>> From: David Sterba <dsterba@suse.cz>
>>> To: Filipe David Manana <fdmanana@gmail.com>
>>> Date: 2014年12月16日 02:19
>>>> On Mon, Dec 15, 2014 at 10:13:45AM +0000, Filipe David Manana wrote:
>>>>>> So another thing I would like to see is doing a more comprehensive
>>>>>> verification that the repair code worked as expected. Currently we
>>>>>> only check that a readonly fsck, after running fsck --repair, returns
>>>>>> 0.
>>>>>>
>>>>>> For the improvements you've been doing, it's equally important to
>>>>>> verify that --repair recovered the inodes, links, etc to the
>>>>>> lost+found directory (or whatever is the directory's name).
>>>>>>
>>>>>> So perhaps adding a verify.sh script to the tarball for example?
>>>>> Or, forgot before, it might be better to do such verification/test in
>>>>> xfstests since we can create the fs and use the new btrfs-progs
>>>>> programs to corrupt leafs/nodes. xfstests has a lot of infrastructure
>>>>> already and probably run by a lot more people (compared to the fsck
>>>>> tests of btrfs-progs).
>>>> I'm thinking about the best way how to integrate that, but it seems that
>>>> there will be always some level of code or infrastructure duplication
>>>> (or other hassle).
>>>>
>>>> btrfs-corrupt-block is not installed by default (make install) and it's
>>>> not a type of utility I'd consider for default installations. The tests
>>>> would be skipped in absence of the utility, so there will be test
>>>> environments where "install xfstests, install btrfspprogs" will not add
>>>> the desired test coverage. Solvable by packaging the extra progs.
>>>>
>>>> Adding corrupt-block into xfsprogs is infeasible (IMO too much code from
>>>> btrfs-progs to be added).
>>>>
>>>> I don't know how much infrastructure code we'd have to either write or
>>>> copy from fstests, but I think it would not be that much. Ideally we
>>>> could write the tests within btrfs-progs and then submit them to fstests
>>>> once they're considered reliable. If we keep the same "syntax" of the
>>>> tests, provide stubs where applicable, the code duplication in test
>>>> itself would be zero. We'd only have to write the stubs in btrfs-progs
>>>> and probably extend fstests to provide helpers for preparing/unpacking
>>>> the images.
>>> In my wildest idea, if we have a good enough btrfs debugger(maybe even
>>> stronger than debugfs), which can
>>> do almost everything from read key/item to corrupt given structure, then we
>>> can resolve them all.
>>> No binary image since corruption can be done by it and verify can also done
>>> by it.
>>> (OK, it's just a daydream)
>>>
>>> But IMHO, isn't xfstests designed to mainly detect kernel defeats?
>>> I don't see any fsck tool test case in it.
>> I don't think xfstests is specific to test the kernel implementation
>> of filesystems. I believe it includes user space code too, but I might
>> be wrong so I'm CCing fstests and Dave to get an authoritative answer.
> We use fstests to test everything we ship for XFS - kernel and
> userspace. i.e. we have tests that corrupt filesystems with xfs_db
> and then test that xfs_repair can fix them, and once fixed the
> filesystem can be mounted and used by the kernel...
>
> i.e. fstests is for testing both the kernel code and the utilities
> used to manipulate filesystems.
That's great.

But what will happen if some btrfs cases need binary(still huge even 
compressed) or
btrfs-image dump(some existing dumps are already several MB)?
Will it be OK for fstests?

Or should we wait until btrfs-progs has a good debug tools like xfs_db 
or debugfs and use
them to generate the corrupted image like xfs testcases do?

Thanks,
Qu

>
>> And I don't see a big problem with btrfs-corrupt not being built by
>> default when running "make" on btrfs-progs. We can make the xfstest
>> not run and print an informative message if the btrfs-corrupt program
>> isn't available - several xfstests do this, they require some
>> executable which isn't either in the xfstests nor xfsprogs
>> repositories - for example generic/241 which requires 'dbench' and
>> generic/299 which requires 'fio'.
> _require_btrfs_corrupt_prog()
>
> Just like we do with lots of other optional userspace tools that are
> required for various tests to run.
>
>> I also have a slight preference to get all
>> tests in the same place (xfstests) rather than in multiple
>> repositories (btrfs-progs, xfstests).
> Definitely my preference as well.
>
> Cheers,
>
> Dave.

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Dave Chinner]

On Wed, Dec 24, 2014 at 10:56:04AM +0800, Qu Wenruo wrote:
> 
> -------- Original Message --------
> Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image +
> corrupt script fsck test case.
> From: Dave Chinner <david@fromorbit.com>
> To: Filipe David Manana <fdmanana@gmail.com>
> Date: 2014年12月24日 08:03
> >[ Sorry to take some time to get to this, it got caught by a spam
> >filter and I only just noticed. ]
> >
> >On Tue, Dec 16, 2014 at 02:08:53PM +0000, Filipe David Manana wrote:
> >>On Tue, Dec 16, 2014 at 1:35 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
> >>>-------- Original Message --------
> >>>Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt
> >>>script fsck test case.
> >>>From: David Sterba <dsterba@suse.cz>
> >>>To: Filipe David Manana <fdmanana@gmail.com>
> >>>Date: 2014年12月16日 02:19
> >>>>On Mon, Dec 15, 2014 at 10:13:45AM +0000, Filipe David Manana wrote:
> >>>>>>So another thing I would like to see is doing a more comprehensive
> >>>>>>verification that the repair code worked as expected. Currently we
> >>>>>>only check that a readonly fsck, after running fsck --repair, returns
> >>>>>>0.
> >>>>>>
> >>>>>>For the improvements you've been doing, it's equally important to
> >>>>>>verify that --repair recovered the inodes, links, etc to the
> >>>>>>lost+found directory (or whatever is the directory's name).
> >>>>>>
> >>>>>>So perhaps adding a verify.sh script to the tarball for example?
> >>>>>Or, forgot before, it might be better to do such verification/test in
> >>>>>xfstests since we can create the fs and use the new btrfs-progs
> >>>>>programs to corrupt leafs/nodes. xfstests has a lot of infrastructure
> >>>>>already and probably run by a lot more people (compared to the fsck
> >>>>>tests of btrfs-progs).
> >>>>I'm thinking about the best way how to integrate that, but it seems that
> >>>>there will be always some level of code or infrastructure duplication
> >>>>(or other hassle).
> >>>>
> >>>>btrfs-corrupt-block is not installed by default (make install) and it's
> >>>>not a type of utility I'd consider for default installations. The tests
> >>>>would be skipped in absence of the utility, so there will be test
> >>>>environments where "install xfstests, install btrfspprogs" will not add
> >>>>the desired test coverage. Solvable by packaging the extra progs.
> >>>>
> >>>>Adding corrupt-block into xfsprogs is infeasible (IMO too much code from
> >>>>btrfs-progs to be added).
> >>>>
> >>>>I don't know how much infrastructure code we'd have to either write or
> >>>>copy from fstests, but I think it would not be that much. Ideally we
> >>>>could write the tests within btrfs-progs and then submit them to fstests
> >>>>once they're considered reliable. If we keep the same "syntax" of the
> >>>>tests, provide stubs where applicable, the code duplication in test
> >>>>itself would be zero. We'd only have to write the stubs in btrfs-progs
> >>>>and probably extend fstests to provide helpers for preparing/unpacking
> >>>>the images.
> >>>In my wildest idea, if we have a good enough btrfs debugger(maybe even
> >>>stronger than debugfs), which can
> >>>do almost everything from read key/item to corrupt given structure, then we
> >>>can resolve them all.
> >>>No binary image since corruption can be done by it and verify can also done
> >>>by it.
> >>>(OK, it's just a daydream)
> >>>
> >>>But IMHO, isn't xfstests designed to mainly detect kernel defeats?
> >>>I don't see any fsck tool test case in it.
> >>I don't think xfstests is specific to test the kernel implementation
> >>of filesystems. I believe it includes user space code too, but I might
> >>be wrong so I'm CCing fstests and Dave to get an authoritative answer.
> >We use fstests to test everything we ship for XFS - kernel and
> >userspace. i.e. we have tests that corrupt filesystems with xfs_db
> >and then test that xfs_repair can fix them, and once fixed the
> >filesystem can be mounted and used by the kernel...
> >
> >i.e. fstests is for testing both the kernel code and the utilities
> >used to manipulate filesystems.
> That's great.
> 
> But what will happen if some btrfs cases need binary(still huge even
> compressed) or
> btrfs-image dump(some existing dumps are already several MB)?
> Will it be OK for fstests?

No. Random filesystem images don't make a regression test. They are
just different encoding of the game of whack-a-mole. Corruption
recovery tests work best with simple, obvious corruptions. This
makes it easy to make sure correct detection and recovery behaviour
works without any other complications.

e.g. overwriting the primary superblock with zeros is a simple test
that every filesystem recovery tool should be able to handle.

> Or should we wait until btrfs-progs has a good debug tools like
> xfs_db or debugfs and use
> them to generate the corrupted image like xfs testcases do?

You don't need a tool like xfs_db to do repeatable structure
corruption - you can do this with dd by writing to known offsets.

xfs_db just makes that really easy by having a filesystem format
parser that means finding the offset we want to write to is
trivial....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[David Sterba]

On Mon, Dec 15, 2014 at 09:36:51AM +0000, Filipe David Manana wrote:
> So another thing I would like to see is doing a more comprehensive
> verification that the repair code worked as expected. Currently we
> only check that a readonly fsck, after running fsck --repair, returns
> 0.
> 
> For the improvements you've been doing, it's equally important to
> verify that --repair recovered the inodes, links, etc to the
> lost+found directory (or whatever is the directory's name).
> 
> So perhaps adding a verify.sh script to the tarball for example?

A verifier script would be good, but I'd rather not put it into the
tarball. We might want to edit it, do cleanups etc, this would require
to regenerate the image each time and the changes would be hard to
review.

We can use the base image name and add -verify.sh suffix instead, eg.
007-bad_root_items_fs_skinny.tar.xz and
007-bad_root_items_fs_skinny-verify.sh

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + 
corrupt script fsck test case.
From: David Sterba <dsterba@suse.cz>
To: Filipe David Manana <fdmanana@gmail.com>
Date: 2014年12月16日 01:35
> On Mon, Dec 15, 2014 at 09:36:51AM +0000, Filipe David Manana wrote:
>> So another thing I would like to see is doing a more comprehensive
>> verification that the repair code worked as expected. Currently we
>> only check that a readonly fsck, after running fsck --repair, returns
>> 0.
>>
>> For the improvements you've been doing, it's equally important to
>> verify that --repair recovered the inodes, links, etc to the
>> lost+found directory (or whatever is the directory's name).
>>
>> So perhaps adding a verify.sh script to the tarball for example?
> A verifier script would be good, but I'd rather not put it into the
> tarball. We might want to edit it, do cleanups etc, this would require
> to regenerate the image each time and the changes would be hard to
> review.
>
> We can use the base image name and add -verify.sh suffix instead, eg.
> 007-bad_root_items_fs_skinny.tar.xz and
> 007-bad_root_items_fs_skinny-verify.sh
>
>
I'd like to add verify script too, especially when it is put out of the 
tarball.

But to the leaf-corruption case, it seems a little overkilled for me.

1) The object of leaf-corrupt recover is not to salvage data.
Although most of the patches are trying its best to salvage as much data 
as possible ,
from ino to file type or even later extent data, but in fact, the 
patchset's main object is to make the metadata
of the btrfs consistent. The data recovery is just a optional addition.
(Original, it's designed to delete every inode whose metadata is lost in 
a corrupted leaf)
So the second btrfsck's return value instead of the contents in 
lost+found is the important.

2) The recovery is *lossy*, verify would better be called on *lossless* 
recovery
Leaf-corruption is based on the btree recovery, which will introduce 
data loss(at least a leaf),
so we can't ensure anything.
And in some case, repair_inode_backref() will even repair backref before 
nlink repair,
which may introduce some randomness
(if a inode_item is not corrupted in a leaf, then a backref maybe 
repaired without move it to lost+found dir)
So for *lossy* repair, I prefer not to add verify script.

I generally agree to add verify script support, but only for lossless 
recovery case.

Thanks,
Qu

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Filipe David Manana]

On Tue, Dec 16, 2014 at 12:58 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>
> -------- Original Message --------
> Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt
> script fsck test case.
> From: David Sterba <dsterba@suse.cz>
> To: Filipe David Manana <fdmanana@gmail.com>
> Date: 2014年12月16日 01:35
>>
>> On Mon, Dec 15, 2014 at 09:36:51AM +0000, Filipe David Manana wrote:
>>>
>>> So another thing I would like to see is doing a more comprehensive
>>> verification that the repair code worked as expected. Currently we
>>> only check that a readonly fsck, after running fsck --repair, returns
>>> 0.
>>>
>>> For the improvements you've been doing, it's equally important to
>>> verify that --repair recovered the inodes, links, etc to the
>>> lost+found directory (or whatever is the directory's name).
>>>
>>> So perhaps adding a verify.sh script to the tarball for example?
>>
>> A verifier script would be good, but I'd rather not put it into the
>> tarball. We might want to edit it, do cleanups etc, this would require
>> to regenerate the image each time and the changes would be hard to
>> review.
>>
>> We can use the base image name and add -verify.sh suffix instead, eg.
>> 007-bad_root_items_fs_skinny.tar.xz and
>> 007-bad_root_items_fs_skinny-verify.sh
>>
>>
> I'd like to add verify script too, especially when it is put out of the
> tarball.
>
> But to the leaf-corruption case, it seems a little overkilled for me.
>
> 1) The object of leaf-corrupt recover is not to salvage data.
> Although most of the patches are trying its best to salvage as much data as
> possible ,
> from ino to file type or even later extent data, but in fact, the patchset's
> main object is to make the metadata
> of the btrfs consistent. The data recovery is just a optional addition.
> (Original, it's designed to delete every inode whose metadata is lost in a
> corrupted leaf)
> So the second btrfsck's return value instead of the contents in lost+found
> is the important.
>
> 2) The recovery is *lossy*, verify would better be called on *lossless*
> recovery
> Leaf-corruption is based on the btree recovery, which will introduce data
> loss(at least a leaf),
> so we can't ensure anything.
> And in some case, repair_inode_backref() will even repair backref before
> nlink repair,
> which may introduce some randomness
> (if a inode_item is not corrupted in a leaf, then a backref maybe repaired
> without move it to lost+found dir)
> So for *lossy* repair, I prefer not to add verify script.

>From the moment we have code that accomplishes something, it doesn't
matter if it was part of a primary or secondary goal of a patch, nor
if it does full or partial recovery. If we have code that does
something (intentionally) we should always try to have tests for it -
if we don't care about what the code does exactly, then we probably
shouldn't have it in the first place.
Otherwise code will break more easily with future changes. Having
manual tests done on each release (or ideally after each btrfs-progs
or fsck at least) is error prone...

>
> I generally agree to add verify script support, but only for lossless
> recovery case.
>
> Thanks,
> Qu



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt script fsck test case.

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + 
corrupt script fsck test case.
From: Filipe David Manana <fdmanana@gmail.com>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年12月16日 21:55
> On Tue, Dec 16, 2014 at 12:58 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>> -------- Original Message --------
>> Subject: Re: [PATCH 1/2] btrfs-progs: Add support for btrfs-image + corrupt
>> script fsck test case.
>> From: David Sterba <dsterba@suse.cz>
>> To: Filipe David Manana <fdmanana@gmail.com>
>> Date: 2014年12月16日 01:35
>>> On Mon, Dec 15, 2014 at 09:36:51AM +0000, Filipe David Manana wrote:
>>>> So another thing I would like to see is doing a more comprehensive
>>>> verification that the repair code worked as expected. Currently we
>>>> only check that a readonly fsck, after running fsck --repair, returns
>>>> 0.
>>>>
>>>> For the improvements you've been doing, it's equally important to
>>>> verify that --repair recovered the inodes, links, etc to the
>>>> lost+found directory (or whatever is the directory's name).
>>>>
>>>> So perhaps adding a verify.sh script to the tarball for example?
>>> A verifier script would be good, but I'd rather not put it into the
>>> tarball. We might want to edit it, do cleanups etc, this would require
>>> to regenerate the image each time and the changes would be hard to
>>> review.
>>>
>>> We can use the base image name and add -verify.sh suffix instead, eg.
>>> 007-bad_root_items_fs_skinny.tar.xz and
>>> 007-bad_root_items_fs_skinny-verify.sh
>>>
>>>
>> I'd like to add verify script too, especially when it is put out of the
>> tarball.
>>
>> But to the leaf-corruption case, it seems a little overkilled for me.
>>
>> 1) The object of leaf-corrupt recover is not to salvage data.
>> Although most of the patches are trying its best to salvage as much data as
>> possible ,
>> from ino to file type or even later extent data, but in fact, the patchset's
>> main object is to make the metadata
>> of the btrfs consistent. The data recovery is just a optional addition.
>> (Original, it's designed to delete every inode whose metadata is lost in a
>> corrupted leaf)
>> So the second btrfsck's return value instead of the contents in lost+found
>> is the important.
>>
>> 2) The recovery is *lossy*, verify would better be called on *lossless*
>> recovery
>> Leaf-corruption is based on the btree recovery, which will introduce data
>> loss(at least a leaf),
>> so we can't ensure anything.
>> And in some case, repair_inode_backref() will even repair backref before
>> nlink repair,
>> which may introduce some randomness
>> (if a inode_item is not corrupted in a leaf, then a backref maybe repaired
>> without move it to lost+found dir)
>> So for *lossy* repair, I prefer not to add verify script.
>  From the moment we have code that accomplishes something, it doesn't
> matter if it was part of a primary or secondary goal of a patch, nor
> if it does full or partial recovery. If we have code that does
> something (intentionally) we should always try to have tests for it -
> if we don't care about what the code does exactly, then we probably
> shouldn't have it in the first place.
First please let me make it clear when you mention the verify script, 
what it really means.
Which case in the leaf-corruption recovery do you mean?

1) Generic script verifying everything or part of the inodes in original 
image is recovered.
If you mean this *GENERIC* one, that's impractical for leaf-corruption 
recovery and any other
lossy recovery.

2) Specific script only for this specific damaged image.
This one is suitable for lossy recovery case but it may be restrict 
verify script, it only tells
what result it should be after recovery for the specific image. And it 
may be only a reminder
for new patches modifying the existing recovery codes.

If you mean 1), IMHO it's not practical.
If you mean 2), I am OK to implement the verify script but I doubt about 
the necessarily.

Thanks,
Qu
> Otherwise code will break more easily with future changes. Having
> manual tests done on each release (or ideally after each btrfs-progs
> or fsck at least) is error prone...
>
>> I generally agree to add verify script support, but only for lossless
>> recovery case.
>>
>> Thanks,
>> Qu
>
>