[PATCH] fstests: btrfs/185 update for single device pseudo device-scan

[PATCH] fstests: btrfs/185 update for single device pseudo device-scan

[Anand Jain]

As we are obliterating the need for the device scan for the single device,
which will return success if the basic superblock verification passes,
even for the duplicate device of the mounted filesystem, drop the check
for the return code in this testcase and continue to verify if the device
path of the mounted filesystem remains unaltered after the scan.

Also, if the test fails, it leaves the local non-standard mount point
remained mounted, leading to further test cases failing. Call unmount
in _cleanup().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/btrfs/185 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/btrfs/185 b/tests/btrfs/185
index ba0200617e69..c7b8d2d46951 100755
--- a/tests/btrfs/185
+++ b/tests/btrfs/185
@@ -15,6 +15,7 @@ mnt=$TEST_DIR/$seq.mnt
 # Override the default cleanup function.
 _cleanup()
 {
+	$UMOUNT_PROG $mnt > /dev/null 2>&1
 	rm -rf $mnt > /dev/null 2>&1
 	cd /
 	rm -f $tmp.*
@@ -51,9 +52,9 @@ for sb_bytenr in 65536 67108864; do
 	echo ..:$? >> $seqres.full
 done
 
-# Original device is mounted, scan of its clone should fail
+# Original device is mounted, scan of its clone must not alter the
+# filesystem device path
 $BTRFS_UTIL_PROG device scan $device_2 >> $seqres.full 2>&1
-[[ $? != 1 ]] && _fail "cloned device scan should fail"
 
 [[ $(findmnt $mnt | grep -v TARGET | $AWK_PROG '{print $2}') != $device_1 ]] && \
 						_fail "mounted device changed"
-- 
2.39.3

Re: [PATCH] fstests: btrfs/185 update for single device pseudo device-scan

[Boris Burkov]

On Thu, Sep 07, 2023 at 12:24:43AM +0800, Anand Jain wrote:
> As we are obliterating the need for the device scan for the single device,
> which will return success if the basic superblock verification passes,
> even for the duplicate device of the mounted filesystem, drop the check
> for the return code in this testcase and continue to verify if the device
> path of the mounted filesystem remains unaltered after the scan.
> 
> Also, if the test fails, it leaves the local non-standard mount point
> remained mounted, leading to further test cases failing. Call unmount
> in _cleanup().

This was also affecting my setup, thanks for the fix!

> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  tests/btrfs/185 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/btrfs/185 b/tests/btrfs/185
> index ba0200617e69..c7b8d2d46951 100755
> --- a/tests/btrfs/185
> +++ b/tests/btrfs/185
> @@ -15,6 +15,7 @@ mnt=$TEST_DIR/$seq.mnt
>  # Override the default cleanup function.
>  _cleanup()
>  {
> +	$UMOUNT_PROG $mnt > /dev/null 2>&1

Do you mean to umount before calling rm -rf on it? That seems.. risky.

>  	rm -rf $mnt > /dev/null 2>&1
>  	cd /
>  	rm -f $tmp.*
> @@ -51,9 +52,9 @@ for sb_bytenr in 65536 67108864; do
>  	echo ..:$? >> $seqres.full
>  done
>  
> -# Original device is mounted, scan of its clone should fail
> +# Original device is mounted, scan of its clone must not alter the
> +# filesystem device path
>  $BTRFS_UTIL_PROG device scan $device_2 >> $seqres.full 2>&1
> -[[ $? != 1 ]] && _fail "cloned device scan should fail"
>  
>  [[ $(findmnt $mnt | grep -v TARGET | $AWK_PROG '{print $2}') != $device_1 ]] && \
>  						_fail "mounted device changed"
> -- 
> 2.39.3
> 

Re: [PATCH] fstests: btrfs/185 update for single device pseudo device-scan

[Anand Jain]

On 12/09/2023 02:32, Boris Burkov wrote:
> On Thu, Sep 07, 2023 at 12:24:43AM +0800, Anand Jain wrote:
>> As we are obliterating the need for the device scan for the single device,
>> which will return success if the basic superblock verification passes,
>> even for the duplicate device of the mounted filesystem, drop the check
>> for the return code in this testcase and continue to verify if the device
>> path of the mounted filesystem remains unaltered after the scan.
>>
>> Also, if the test fails, it leaves the local non-standard mount point
>> remained mounted, leading to further test cases failing. Call unmount
>> in _cleanup().
> 
> This was also affecting my setup, thanks for the fix!

Hmm, it shouldn't, unless commit d41f57d15a90 ("brfs: scan but don't
  register device on single device filesystem") is already in the kernel
  you are testing. Do you have the logs?


> 
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   tests/btrfs/185 | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/btrfs/185 b/tests/btrfs/185
>> index ba0200617e69..c7b8d2d46951 100755
>> --- a/tests/btrfs/185
>> +++ b/tests/btrfs/185
>> @@ -15,6 +15,7 @@ mnt=$TEST_DIR/$seq.mnt
>>   # Override the default cleanup function.
>>   _cleanup()
>>   {



>> +	$UMOUNT_PROG $mnt > /dev/null 2>&1
> 
> Do you mean to umount before calling rm -rf on it? That seems.. risky.
> 
>>   	rm -rf $mnt > /dev/null 2>&1

mnt is a special mount point. Removing the special mnt directory after 
unmounting it is correct..


>>   	cd /
>>   	rm -f $tmp.*
>> @@ -51,9 +52,9 @@ for sb_bytenr in 65536 67108864; do
>>   	echo ..:$? >> $seqres.full
>>   done
>>   
>> -# Original device is mounted, scan of its clone should fail
>> +# Original device is mounted, scan of its clone must not alter the
>> +# filesystem device path
>>   $BTRFS_UTIL_PROG device scan $device_2 >> $seqres.full 2>&1
>> -[[ $? != 1 ]] && _fail "cloned device scan should fail"
>>   
>>   [[ $(findmnt $mnt | grep -v TARGET | $AWK_PROG '{print $2}') != $device_1 ]] && \
>>   						_fail "mounted device changed"
>> -- 
>> 2.39.3
>>

Re: [PATCH] fstests: btrfs/185 update for single device pseudo device-scan

[Boris Burkov]

On Tue, Sep 12, 2023 at 05:07:49PM +0800, Anand Jain wrote:
> 
> 
> On 12/09/2023 02:32, Boris Burkov wrote:
> > On Thu, Sep 07, 2023 at 12:24:43AM +0800, Anand Jain wrote:
> > > As we are obliterating the need for the device scan for the single device,
> > > which will return success if the basic superblock verification passes,
> > > even for the duplicate device of the mounted filesystem, drop the check
> > > for the return code in this testcase and continue to verify if the device
> > > path of the mounted filesystem remains unaltered after the scan.
> > > 
> > > Also, if the test fails, it leaves the local non-standard mount point
> > > remained mounted, leading to further test cases failing. Call unmount
> > > in _cleanup().
> > 
> > This was also affecting my setup, thanks for the fix!
> 
> Hmm, it shouldn't, unless commit d41f57d15a90 ("brfs: scan but don't
>  register device on single device filesystem") is already in the kernel
>  you are testing. Do you have the logs?

I was testing on top of misc-next and that patch was indeed present.

> 
> 
> > 
> > > 
> > > Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Boris Burkov <boris@bur.io>

> > > ---
> > >   tests/btrfs/185 | 5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/btrfs/185 b/tests/btrfs/185
> > > index ba0200617e69..c7b8d2d46951 100755
> > > --- a/tests/btrfs/185
> > > +++ b/tests/btrfs/185
> > > @@ -15,6 +15,7 @@ mnt=$TEST_DIR/$seq.mnt
> > >   # Override the default cleanup function.
> > >   _cleanup()
> > >   {
> 
> 
> 
> > > +	$UMOUNT_PROG $mnt > /dev/null 2>&1
> > 
> > Do you mean to umount before calling rm -rf on it? That seems.. risky.
> > 
> > >   	rm -rf $mnt > /dev/null 2>&1
> 
> mnt is a special mount point. Removing the special mnt directory after
> unmounting it is correct..

D'oh, you're totally right, my bad!

> 
> 
> > >   	cd /
> > >   	rm -f $tmp.*
> > > @@ -51,9 +52,9 @@ for sb_bytenr in 65536 67108864; do
> > >   	echo ..:$? >> $seqres.full
> > >   done
> > > -# Original device is mounted, scan of its clone should fail
> > > +# Original device is mounted, scan of its clone must not alter the
> > > +# filesystem device path
> > >   $BTRFS_UTIL_PROG device scan $device_2 >> $seqres.full 2>&1
> > > -[[ $? != 1 ]] && _fail "cloned device scan should fail"
> > >   [[ $(findmnt $mnt | grep -v TARGET | $AWK_PROG '{print $2}') != $device_1 ]] && \
> > >   						_fail "mounted device changed"
> > > -- 
> > > 2.39.3
> > > 

Re: [PATCH] fstests: btrfs/185 update for single device pseudo device-scan

[Boris Burkov]

On Thu, Sep 07, 2023 at 12:24:43AM +0800, Anand Jain wrote:
> As we are obliterating the need for the device scan for the single device,
> which will return success if the basic superblock verification passes,
> even for the duplicate device of the mounted filesystem, drop the check
> for the return code in this testcase and continue to verify if the device
> path of the mounted filesystem remains unaltered after the scan.
> 
> Also, if the test fails, it leaves the local non-standard mount point
> remained mounted, leading to further test cases failing. Call unmount
> in _cleanup().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  tests/btrfs/185 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/btrfs/185 b/tests/btrfs/185
> index ba0200617e69..c7b8d2d46951 100755
> --- a/tests/btrfs/185
> +++ b/tests/btrfs/185
> @@ -15,6 +15,7 @@ mnt=$TEST_DIR/$seq.mnt
>  # Override the default cleanup function.
>  _cleanup()
>  {
> +	$UMOUNT_PROG $mnt > /dev/null 2>&1
>  	rm -rf $mnt > /dev/null 2>&1
>  	cd /
>  	rm -f $tmp.*

Also, do we need to do the scratch_dev_pool_put on cleanup? Do we have
to worry about having not actually called scratch_dev_pool_get?

> @@ -51,9 +52,9 @@ for sb_bytenr in 65536 67108864; do
>  	echo ..:$? >> $seqres.full
>  done
>  
> -# Original device is mounted, scan of its clone should fail
> +# Original device is mounted, scan of its clone must not alter the
> +# filesystem device path
>  $BTRFS_UTIL_PROG device scan $device_2 >> $seqres.full 2>&1
> -[[ $? != 1 ]] && _fail "cloned device scan should fail"
>  
>  [[ $(findmnt $mnt | grep -v TARGET | $AWK_PROG '{print $2}') != $device_1 ]] && \
>  						_fail "mounted device changed"
> -- 
> 2.39.3
> 

Re: [PATCH] fstests: btrfs/185 update for single device pseudo device-scan

[Anand Jain]

On 12/09/2023 02:35, Boris Burkov wrote:
> On Thu, Sep 07, 2023 at 12:24:43AM +0800, Anand Jain wrote:
>> As we are obliterating the need for the device scan for the single device,
>> which will return success if the basic superblock verification passes,
>> even for the duplicate device of the mounted filesystem, drop the check
>> for the return code in this testcase and continue to verify if the device
>> path of the mounted filesystem remains unaltered after the scan.
>>
>> Also, if the test fails, it leaves the local non-standard mount point
>> remained mounted, leading to further test cases failing. Call unmount
>> in _cleanup().
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   tests/btrfs/185 | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/btrfs/185 b/tests/btrfs/185
>> index ba0200617e69..c7b8d2d46951 100755
>> --- a/tests/btrfs/185
>> +++ b/tests/btrfs/185
>> @@ -15,6 +15,7 @@ mnt=$TEST_DIR/$seq.mnt
>>   # Override the default cleanup function.
>>   _cleanup()
>>   {
>> +	$UMOUNT_PROG $mnt > /dev/null 2>&1
>>   	rm -rf $mnt > /dev/null 2>&1
>>   	cd /
>>   	rm -f $tmp.*
> 
> Also, do we need to do the scratch_dev_pool_put on cleanup? Do we have
> to worry about having not actually called scratch_dev_pool_get?

No need, actually, the config gets called at the beginning of the test
cases, so SCRATCH_DEV_POOL gets overwritten.


> 
>> @@ -51,9 +52,9 @@ for sb_bytenr in 65536 67108864; do
>>   	echo ..:$? >> $seqres.full
>>   done
>>   
>> -# Original device is mounted, scan of its clone should fail
>> +# Original device is mounted, scan of its clone must not alter the
>> +# filesystem device path
>>   $BTRFS_UTIL_PROG device scan $device_2 >> $seqres.full 2>&1
>> -[[ $? != 1 ]] && _fail "cloned device scan should fail"
>>   
>>   [[ $(findmnt $mnt | grep -v TARGET | $AWK_PROG '{print $2}') != $device_1 ]] && \
>>   						_fail "mounted device changed"
>> -- 
>> 2.39.3
>>

Re: [PATCH] fstests: btrfs/185 update for single device pseudo device-scan

[Zorro Lang]

On Thu, Sep 07, 2023 at 12:24:43AM +0800, Anand Jain wrote:
> As we are obliterating the need for the device scan for the single device,
> which will return success if the basic superblock verification passes,
> even for the duplicate device of the mounted filesystem, drop the check
> for the return code in this testcase and continue to verify if the device
> path of the mounted filesystem remains unaltered after the scan.
> 
> Also, if the test fails, it leaves the local non-standard mount point
> remained mounted, leading to further test cases failing. Call unmount
> in _cleanup().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---

Make sense to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

>  tests/btrfs/185 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/btrfs/185 b/tests/btrfs/185
> index ba0200617e69..c7b8d2d46951 100755
> --- a/tests/btrfs/185
> +++ b/tests/btrfs/185
> @@ -15,6 +15,7 @@ mnt=$TEST_DIR/$seq.mnt
>  # Override the default cleanup function.
>  _cleanup()
>  {
> +	$UMOUNT_PROG $mnt > /dev/null 2>&1
>  	rm -rf $mnt > /dev/null 2>&1
>  	cd /
>  	rm -f $tmp.*
> @@ -51,9 +52,9 @@ for sb_bytenr in 65536 67108864; do
>  	echo ..:$? >> $seqres.full
>  done
>  
> -# Original device is mounted, scan of its clone should fail
> +# Original device is mounted, scan of its clone must not alter the
> +# filesystem device path
>  $BTRFS_UTIL_PROG device scan $device_2 >> $seqres.full 2>&1
> -[[ $? != 1 ]] && _fail "cloned device scan should fail"
>  
>  [[ $(findmnt $mnt | grep -v TARGET | $AWK_PROG '{print $2}') != $device_1 ]] && \
>  						_fail "mounted device changed"
> -- 
> 2.39.3
>