[PATCH RFC] fstests: generic/077: fix populate fs use _fill_fs()

[PATCH RFC] fstests: generic/077: fix populate fs use _fill_fs()

[Anand Jain]

Test case generic/077 uses files under /lib or /usr to fill SCRATCH_MNT.
If /usr or /lib is below 256mb then test fails to run, or if these dirs
are too large it takes a long time for the cp to finish. On my machine
it takes 645sec.

This patch propose to use the common/populate function _fill_fs() to
write files into the target directory instead. However I am not too
sure about the motivation of this test case in the first place, and
why does it wanted to cp /usr or /lib, and why fs should become full?
Any idea? Thanks.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/generic/077     | 24 +++++-------------------
 tests/generic/077.out |  3 +--
 2 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/tests/generic/077 b/tests/generic/077
index d11b49c6ff15..d8e5551f1925 100755
--- a/tests/generic/077
+++ b/tests/generic/077
@@ -14,18 +14,6 @@ here=`pwd`
 tmp=/tmp/$$
 status=1
 
-# Something w/ enough data to fill 256M of fs...
-filler=""
-[ -d /lib/modules ] && \
-	[ $(( $(du -h -m /lib/modules | tail -1| cut -f1) * 2 )) -ge 256 ] && \
-	filler=/lib/modules
-
-# fall back in case /lib/modules doesn't exist or smaller
-[[ -z $filler ]] && \
-	[ -d /usr ] && \
-	[ $(( $(du -h -m /usr | tail -1| cut -f1) * 2 )) -ge 256 ] && \
-	filler=/usr
-
 _cleanup()
 {
 	cd /
@@ -38,13 +26,12 @@ trap "_cleanup; rm -f $tmp.*; exit \$status" 0 1 2 3 15
 . ./common/rc
 . ./common/filter
 . ./common/attr
+. ./common/populate
 
 # real QA test starts here
 _supported_fs generic
 _supported_os Linux
 
-[ ! -d $filler ] && _notrun "No directory at least 256MB to source files from"
-
 _require_scratch
 _require_attrs
 _require_acls
@@ -64,11 +51,10 @@ mkdir $SCRATCH_MNT/subdir
 echo "*** set default ACL"
 setfacl -R -dm u:fsgqa:rwx,g::rwx,o::r-x,m::rwx $SCRATCH_MNT/subdir
 
-echo "*** populate filesystem, pass #1" | tee -a $seqres.full
-cp -rf $filler $SCRATCH_MNT/subdir >$seqres.full 2>&1
-
-echo "*** populate filesystem, pass #2" | tee -a $seqres.full
-cp -rf $filler $SCRATCH_MNT/subdir >$seqres.full 2>&1
+blksz="$(_get_block_size $SCRATCH_MNT/subdir)"
+echo "*** populate filesystem" | tee -a $seqres.full
+echo "*** fill_fs $fs_size $SCRATCH_MNT/subdir $blksz 0" >> $seqres.full
+_fill_fs $fs_size $SCRATCH_MNT/subdir $blksz 0 >> $seqres.full 2>&1
 
 _check_scratch_fs
 
diff --git a/tests/generic/077.out b/tests/generic/077.out
index eae7226ab29c..9c143c902a2c 100644
--- a/tests/generic/077.out
+++ b/tests/generic/077.out
@@ -1,7 +1,6 @@
 QA output created by 077
 *** create filesystem
 *** set default ACL
-*** populate filesystem, pass #1
-*** populate filesystem, pass #2
+*** populate filesystem
 *** all done
 *** unmount
-- 
2.17.1

Re: [PATCH RFC] fstests: generic/077: fix populate fs use _fill_fs()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2323 bytes --]



On 2019/4/12 下午1:24, Anand Jain wrote:
> Test case generic/077 uses files under /lib or /usr to fill SCRATCH_MNT.
> If /usr or /lib is below 256mb then test fails to run, or if these dirs
> are too large it takes a long time for the cp to finish. On my machine
> it takes 645sec.

Wait for a minute, ths fs is only 256M sized, if it takes you over 10
minutes, there must be something else wrong.

> 
> This patch propose to use the common/populate function _fill_fs() to
> write files into the target directory instead. However I am not too
> sure about the motivation of this test case in the first place, and
> why does it wanted to cp /usr or /lib,

To my eyes, it's using /usr or /lib just for it's mostly full/contains a
lot of files for most systems.

> and why fs should become full?

Maybe to hit certain ENOSPC corner case for ACL/xattr, just my guess.

> Any idea? Thanks.

Use populate_fs is definitely the right move.

However I have some concern below.

[snip]
>  echo "*** set default ACL"
>  setfacl -R -dm u:fsgqa:rwx,g::rwx,o::r-x,m::rwx $SCRATCH_MNT/subdir
>  
> -echo "*** populate filesystem, pass #1" | tee -a $seqres.full
> -cp -rf $filler $SCRATCH_MNT/subdir >$seqres.full 2>&1
> -
> -echo "*** populate filesystem, pass #2" | tee -a $seqres.full
> -cp -rf $filler $SCRATCH_MNT/subdir >$seqres.full 2>&1
> +blksz="$(_get_block_size $SCRATCH_MNT/subdir)"
> +echo "*** populate filesystem" | tee -a $seqres.full
> +echo "*** fill_fs $fs_size $SCRATCH_MNT/subdir $blksz 0" >> $seqres.full
> +_fill_fs $fs_size $SCRATCH_MNT/subdir $blksz 0 >> $seqres.full 2>&1

Unlike the original behavior, which do 2 passes and the 2nd pass will
overwrite previous files.

While after your modification, it's no longer the case.

At least we should try to replay the 1st run to mimic the original behavior.

Thanks,
Qu

>  
>  _check_scratch_fs
>  
> diff --git a/tests/generic/077.out b/tests/generic/077.out
> index eae7226ab29c..9c143c902a2c 100644
> --- a/tests/generic/077.out
> +++ b/tests/generic/077.out
> @@ -1,7 +1,6 @@
>  QA output created by 077
>  *** create filesystem
>  *** set default ACL
> -*** populate filesystem, pass #1
> -*** populate filesystem, pass #2
> +*** populate filesystem
>  *** all done
>  *** unmount
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC] fstests: generic/077: fix populate fs use _fill_fs()

[Nikolay Borisov]

On 12.04.19 г. 10:15 ч., Qu Wenruo wrote:
> 
> 
> On 2019/4/12 下午1:24, Anand Jain wrote:
>> Test case generic/077 uses files under /lib or /usr to fill SCRATCH_MNT.
>> If /usr or /lib is below 256mb then test fails to run, or if these dirs
>> are too large it takes a long time for the cp to finish. On my machine
>> it takes 645sec.
> 
> Wait for a minute, ths fs is only 256M sized, if it takes you over 10
> minutes, there must be something else wrong.

I've had the same issue, the problem is that once the fs is full cp will
continue happily trying to copy stuff and will just be failing with ENOSPC

> 
>>
>> This patch propose to use the common/populate function _fill_fs() to
>> write files into the target directory instead. However I am not too
>> sure about the motivation of this test case in the first place, and
>> why does it wanted to cp /usr or /lib,
> 
> To my eyes, it's using /usr or /lib just for it's mostly full/contains a
> lot of files for most systems.
> 
>> and why fs should become full?
> 
> Maybe to hit certain ENOSPC corner case for ACL/xattr, just my guess.

nod, I've used that test to test certain patches (latest this test was
useful is when testing Anad's xattr/acl patches)

> 
>> Any idea? Thanks.
> 
> Use populate_fs is definitely the right move.
> 
> However I have some concern below.
> 
> [snip]
>>  echo "*** set default ACL"
>>  setfacl -R -dm u:fsgqa:rwx,g::rwx,o::r-x,m::rwx $SCRATCH_MNT/subdir
>>  
>> -echo "*** populate filesystem, pass #1" | tee -a $seqres.full
>> -cp -rf $filler $SCRATCH_MNT/subdir >$seqres.full 2>&1
>> -
>> -echo "*** populate filesystem, pass #2" | tee -a $seqres.full
>> -cp -rf $filler $SCRATCH_MNT/subdir >$seqres.full 2>&1
>> +blksz="$(_get_block_size $SCRATCH_MNT/subdir)"
>> +echo "*** populate filesystem" | tee -a $seqres.full
>> +echo "*** fill_fs $fs_size $SCRATCH_MNT/subdir $blksz 0" >> $seqres.full
>> +_fill_fs $fs_size $SCRATCH_MNT/subdir $blksz 0 >> $seqres.full 2>&1
> 
> Unlike the original behavior, which do 2 passes and the 2nd pass will
> overwrite previous files.
> 
> While after your modification, it's no longer the case.
> 
> At least we should try to replay the 1st run to mimic the original behavior.

Looking at the test shouldn't the "set default acl" be done after the fs
is completely full?

> 
> Thanks,
> Qu
> 
>>  
>>  _check_scratch_fs
>>  
>> diff --git a/tests/generic/077.out b/tests/generic/077.out
>> index eae7226ab29c..9c143c902a2c 100644
>> --- a/tests/generic/077.out
>> +++ b/tests/generic/077.out
>> @@ -1,7 +1,6 @@
>>  QA output created by 077
>>  *** create filesystem
>>  *** set default ACL
>> -*** populate filesystem, pass #1
>> -*** populate filesystem, pass #2
>> +*** populate filesystem
>>  *** all done
>>  *** unmount
>>
> 

Re: [PATCH RFC] fstests: generic/077: fix populate fs use _fill_fs()

[Anand Jain]

On 12/4/19 3:27 PM, Nikolay Borisov wrote:
> 
> 
> On 12.04.19 г. 10:15 ч., Qu Wenruo wrote:
>>
>>
>> On 2019/4/12 下午1:24, Anand Jain wrote:
>>> Test case generic/077 uses files under /lib or /usr to fill SCRATCH_MNT.
>>> If /usr or /lib is below 256mb then test fails to run, or if these dirs
>>> are too large it takes a long time for the cp to finish. On my machine
>>> it takes 645sec.
>>
>> Wait for a minute, ths fs is only 256M sized, if it takes you over 10
>> minutes, there must be something else wrong.
> 
> I've had the same issue, the problem is that once the fs is full cp will
> continue happily trying to copy stuff and will just be failing with ENOSPC
> 
>>
>>>
>>> This patch propose to use the common/populate function _fill_fs() to
>>> write files into the target directory instead. However I am not too
>>> sure about the motivation of this test case in the first place, and
>>> why does it wanted to cp /usr or /lib,
>>
>> To my eyes, it's using /usr or /lib just for it's mostly full/contains a
>> lot of files for most systems.
>>
>>> and why fs should become full?
>>
>> Maybe to hit certain ENOSPC corner case for ACL/xattr, just my guess.
> 
> nod, I've used that test to test certain patches (latest this test was
> useful is when testing Anad's xattr/acl patches)
> 
>>
>>> Any idea? Thanks.
>>
>> Use populate_fs is definitely the right move.
>>
>> However I have some concern below.
>>
>> [snip]
>>>   echo "*** set default ACL"
>>>   setfacl -R -dm u:fsgqa:rwx,g::rwx,o::r-x,m::rwx $SCRATCH_MNT/subdir
>>>   
>>> -echo "*** populate filesystem, pass #1" | tee -a $seqres.full
>>> -cp -rf $filler $SCRATCH_MNT/subdir >$seqres.full 2>&1
>>> -
>>> -echo "*** populate filesystem, pass #2" | tee -a $seqres.full
>>> -cp -rf $filler $SCRATCH_MNT/subdir >$seqres.full 2>&1
>>> +blksz="$(_get_block_size $SCRATCH_MNT/subdir)"
>>> +echo "*** populate filesystem" | tee -a $seqres.full
>>> +echo "*** fill_fs $fs_size $SCRATCH_MNT/subdir $blksz 0" >> $seqres.full
>>> +_fill_fs $fs_size $SCRATCH_MNT/subdir $blksz 0 >> $seqres.full 2>&1
>>
>> Unlike the original behavior, which do 2 passes and the 2nd pass will
>> overwrite previous files.
>>
>> While after your modification, it's no longer the case.
>>
>> At least we should try to replay the 1st run to mimic the original behavior.
> 
> Looking at the test shouldn't the "set default acl" be done after the fs
> is completely full?

  Even by using a large /usr, the cp (2 iterations) was never been able
  to make the metadata full, (same with __full_fs() as well).

  After cp -r /usr <256mb fs> reported ENOSPC.
  stat -f -c "%f" /btrfs
   16488

Thanks, Anand

>>
>> Thanks,
>> Qu
>>
>>>   
>>>   _check_scratch_fs
>>>   
>>> diff --git a/tests/generic/077.out b/tests/generic/077.out
>>> index eae7226ab29c..9c143c902a2c 100644
>>> --- a/tests/generic/077.out
>>> +++ b/tests/generic/077.out
>>> @@ -1,7 +1,6 @@
>>>   QA output created by 077
>>>   *** create filesystem
>>>   *** set default ACL
>>> -*** populate filesystem, pass #1
>>> -*** populate filesystem, pass #2
>>> +*** populate filesystem
>>>   *** all done
>>>   *** unmount
>>>
>>

Re: [PATCH RFC] fstests: generic/077: fix populate fs use _fill_fs()

[Anand Jain]

On 12/4/19 3:15 PM, Qu Wenruo wrote:
> 
> 
> On 2019/4/12 下午1:24, Anand Jain wrote:
>> Test case generic/077 uses files under /lib or /usr to fill SCRATCH_MNT.
>> If /usr or /lib is below 256mb then test fails to run, or if these dirs
>> are too large it takes a long time for the cp to finish. On my machine
>> it takes 645sec.
> 
> Wait for a minute, ths fs is only 256M sized, if it takes you over 10
> minutes, there must be something else wrong.

  What's happening is cp -r fails (ENOSPC) for each file, so the
  cp -r runs for a long time.

>>
>> This patch propose to use the common/populate function _fill_fs() to
>> write files into the target directory instead. However I am not too
>> sure about the motivation of this test case in the first place, and
>> why does it wanted to cp /usr or /lib,
> 
> To my eyes, it's using /usr or /lib just for it's mostly full/contains a
> lot of files for most systems.

  A lot of files. At the same time its not consistent across systems.

>> and why fs should become full?
> 
> Maybe to hit certain ENOSPC corner case for ACL/xattr, just my guess.

ok.

>> Any idea? Thanks.
> 
> Use populate_fs is definitely the right move.
> 
> However I have some concern below.
> 
> [snip]
>>   echo "*** set default ACL"
>>   setfacl -R -dm u:fsgqa:rwx,g::rwx,o::r-x,m::rwx $SCRATCH_MNT/subdir
>>   
>> -echo "*** populate filesystem, pass #1" | tee -a $seqres.full
>> -cp -rf $filler $SCRATCH_MNT/subdir >$seqres.full 2>&1
>> -
>> -echo "*** populate filesystem, pass #2" | tee -a $seqres.full
>> -cp -rf $filler $SCRATCH_MNT/subdir >$seqres.full 2>&1
>> +blksz="$(_get_block_size $SCRATCH_MNT/subdir)"
>> +echo "*** populate filesystem" | tee -a $seqres.full
>> +echo "*** fill_fs $fs_size $SCRATCH_MNT/subdir $blksz 0" >> $seqres.full
>> +_fill_fs $fs_size $SCRATCH_MNT/subdir $blksz 0 >> $seqres.full 2>&1
> 
> Unlike the original behavior, which do 2 passes and the 2nd pass will
> overwrite previous files.
> 
> While after your modification, it's no longer the case.
> 
> At least we should try to replay the 1st run to mimic the original behavior.

  yep.

Thanks, Anand

> Thanks,
> Qu
> 
>>   
>>   _check_scratch_fs
>>   
>> diff --git a/tests/generic/077.out b/tests/generic/077.out
>> index eae7226ab29c..9c143c902a2c 100644
>> --- a/tests/generic/077.out
>> +++ b/tests/generic/077.out
>> @@ -1,7 +1,6 @@
>>   QA output created by 077
>>   *** create filesystem
>>   *** set default ACL
>> -*** populate filesystem, pass #1
>> -*** populate filesystem, pass #2
>> +*** populate filesystem
>>   *** all done
>>   *** unmount
>>
>