Re: [PATCH] fstests: delete btrfs/064 it makes no sense

[Anand Jain <anand.jain@oracle.com>]

On 30/9/20 1:26 am, Josef Bacik wrote:
> On 9/29/20 12:13 PM, Filipe Manana wrote:
>> On Tue, Sep 29, 2020 at 5:02 PM Josef Bacik <josef@toxicpanda.com> wrote:
>>>
>>> On 9/29/20 11:55 AM, Filipe Manana wrote:
>>>> On Tue, Sep 29, 2020 at 4:50 PM Anand Jain <anand.jain@oracle.com> 
>>>> wrote:
>>>>>
>>>>> btrfs/064 aimed to test balance and replace concurrency while the 
>>>>> stress
>>>>> test is running in the background.
>>>>>
>>>>> However, as the balance and the replace operation are mutually
>>>>> exclusive, so they can never run concurrently.
>>>>
>>>> And it's good to have a test that verifies that attempting to run them
>>>> concurrently doesn't cause any problems, like crashes, memory leaks or
>>>> some sort of filesystem corruption.
>>>>
>>>> For example btrfs/187, which I wrote sometime ago, tests that running
>>>> send, balance and deduplication in parallel doesn't result in crashes,
>>>> since in the past they were allowed to run concurrently.
>>>>
>>>> I see no point in removing the test, it's useful.
>>>
>>> My confusion was around whether this test was actually testing what we
>>> think it should be testing.  If this test was meant to make sure that
>>> replace works while we've got load on the fs, then clearly it's not
>>> doing what we think it's doing.
>>
>> Given that neither the test's description nor the changelog mention
>> that it expects device replace and balance to be able to run
>> concurrently,
>> that errors are explicitly ignored and redirected to $seqres.full, and
>> we don't do any sort of validation after device replace operations, it
>> makes it clear to me it's a stress test.
>>
> 
> Sure but I spent a while looking at it when it was failing being very 
> confused.  In my mind my snapshot-stress.sh is a stress test, because 
> its meant to run without errors.  The changelog and description are 
> sufficiently vague enough that it appeared that Eryu meant to write a 
> test that actually did a replace and balance at the same time.  The test 
> clearly isn't doing that, so we need to update the description so it's 
> clear that's what's going on.  And then I wanted to make sure that we do 
> in fact have a test that stresses replace in these scenarios, because I 
> want to make sure we actually test replace as well.
> 
> Not ripping it out is fine, but updating the description so I'm not 
> confused in a couple years when I trip over this again would be nice. 
> Thanks,
> 

As of now, we have the following balance concurrency tests.
-----
028 balance and unlink fsstress concurrency [1]
060 balance and subvol ops concurrency with fsstress [2]
061 balance and scrub concurrency with fsstress [2]
062 balance and defrag concurrency with fsstress [2]
063 balance and remount concurrency with fsstress [2]
064 balance and replace concurrency with fsstress  [2]
177 balance and resize concurrency
187 balance, send and dedupe concurrency
190 balance with qgroup

[1]
args=`_scale_fsstress_args -z \
         -f write=10 -f unlink=10 \
         -f creat=10 -f fsync=10 \
         -f fsync=10 -n 100000 -p 2 \
         -d $SCRATCH_MNT/stress_dir`

[2]
args=`_scale_fsstress_args -p 20 -n 100 $FSSTRESS_AVOID -d 
$SCRATCH_MNT/stressdir`
-----

064 shall test balance with fsstress in the background. The replace 
thread is kept out with the early check of BTRFS_FS_EXCL_OP in the kernel.
I am ok with the 064 headers updated, will send v2.


Also, it turns out that this test case helped to find a btrfs-progs bug.
Its patch [1] is sent to the ML.
   [1] btrfs-progs: fix return code for failed replace start

Thanks, Anand


> Josef