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

[Filipe Manana <fdmanana@gmail.com>]

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.

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
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”