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

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

On 30/9/20 5:16 pm, Filipe Manana wrote:
> On Wed, Sep 30, 2020 at 5:14 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> 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
> 
> No, 177 does not test balance and resize concurrency.
> It tests balance when a swap file exists. And the resize happens
> (starts and ends) before setting the swap file and before doing the
> balance.

   You are right. Thanks for correcting.
-Anand

> 
> Thanks.
> 
> 
>> 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
>>
> 
>