fstests: btrfs/153: Remove it from auto group
diff mbox series

Message ID 20200114125044.21594-1-wqu@suse.com
State New
Headers show
Series
  • fstests: btrfs/153: Remove it from auto group
Related show

Commit Message

Qu Wenruo Jan. 14, 2020, 12:50 p.m. UTC
This test case always fail after commit c6887cd11149 ("Btrfs: don't do
nocow check unless we have to").
As btrfs no longer checks nodatacow at buffered write time.

That commits brings in a big performance enhancement, as that check is
not cheap, but breaks qgroup, as write into preallocated space now needs
extra space.

There isn't yet a good solution (reverting that patch is not possible,
and only check nodatacow for quota enabled case is very bug prune due to
quite a lot code change).

We may solve it using the new ticketed space reservation facility, but
that won't come into fruit anytime soon.

So let's just remove that test case from 'auto' group, but still keep
the test case to inform we still have a lot of work to do.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/btrfs/group | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eryu Guan Feb. 1, 2020, 7:36 a.m. UTC | #1
On Tue, Jan 14, 2020 at 08:50:44PM +0800, Qu Wenruo wrote:
> This test case always fail after commit c6887cd11149 ("Btrfs: don't do
> nocow check unless we have to").
> As btrfs no longer checks nodatacow at buffered write time.
> 
> That commits brings in a big performance enhancement, as that check is
> not cheap, but breaks qgroup, as write into preallocated space now needs
> extra space.
> 
> There isn't yet a good solution (reverting that patch is not possible,
> and only check nodatacow for quota enabled case is very bug prune due to
> quite a lot code change).
> 
> We may solve it using the new ticketed space reservation facility, but
> that won't come into fruit anytime soon.
> 
> So let's just remove that test case from 'auto' group, but still keep
> the test case to inform we still have a lot of work to do.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

I'd like to see an ACK from btrfs folks. Thanks!

Eryu

> ---
>  tests/btrfs/group | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index 697b6a38ea00..3c554a194742 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -155,7 +155,7 @@
>  150 auto quick dangerous
>  151 auto quick volume
>  152 auto quick metadata qgroup send
> -153 auto quick qgroup limit
> +153 quick qgroup limit
>  154 auto quick volume
>  155 auto quick send
>  156 auto quick trim
> -- 
> 2.24.1
>
Amir Goldstein Feb. 1, 2020, 11:10 a.m. UTC | #2
On Sat, Feb 1, 2020 at 9:41 AM Eryu Guan <guaneryu@gmail.com> wrote:
>
> On Tue, Jan 14, 2020 at 08:50:44PM +0800, Qu Wenruo wrote:
> > This test case always fail after commit c6887cd11149 ("Btrfs: don't do
> > nocow check unless we have to").
> > As btrfs no longer checks nodatacow at buffered write time.
> >
> > That commits brings in a big performance enhancement, as that check is
> > not cheap, but breaks qgroup, as write into preallocated space now needs
> > extra space.
> >
> > There isn't yet a good solution (reverting that patch is not possible,
> > and only check nodatacow for quota enabled case is very bug prune due to
> > quite a lot code change).
> >
> > We may solve it using the new ticketed space reservation facility, but
> > that won't come into fruit anytime soon.
> >
> > So let's just remove that test case from 'auto' group, but still keep
> > the test case to inform we still have a lot of work to do.
> >
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> I'd like to see an ACK from btrfs folks. Thanks!
>
> Eryu
>
> > ---
> >  tests/btrfs/group | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/btrfs/group b/tests/btrfs/group
> > index 697b6a38ea00..3c554a194742 100644
> > --- a/tests/btrfs/group
> > +++ b/tests/btrfs/group
> > @@ -155,7 +155,7 @@
> >  150 auto quick dangerous
> >  151 auto quick volume
> >  152 auto quick metadata qgroup send
> > -153 auto quick qgroup limit
> > +153 quick qgroup limit

Hmm, if removing from auto it might make sense to also remove it
from quick, because people often use quick as a sanity regression group.

The issue at hand is a recurring pattern.
It is also been discussed recently about generic/484:
https://lore.kernel.org/fstests/20200131164619.GA13005@infradead.org/

I also handled something like this with:
fdb69864 overlay/061: remove from auto and quick groups

I suggest adding a group 'broken' to mark known/wontfix issues
then a default regression test could run -g auto -x broken
or -g quick -x broken for a quick regression sanity.

Thoughts?

Amir.
Qu Wenruo Feb. 1, 2020, 12:38 p.m. UTC | #3
On 2020/2/1 下午7:10, Amir Goldstein wrote:
> On Sat, Feb 1, 2020 at 9:41 AM Eryu Guan <guaneryu@gmail.com> wrote:
>>
>> On Tue, Jan 14, 2020 at 08:50:44PM +0800, Qu Wenruo wrote:
>>> This test case always fail after commit c6887cd11149 ("Btrfs: don't do
>>> nocow check unless we have to").
>>> As btrfs no longer checks nodatacow at buffered write time.
>>>
>>> That commits brings in a big performance enhancement, as that check is
>>> not cheap, but breaks qgroup, as write into preallocated space now needs
>>> extra space.
>>>
>>> There isn't yet a good solution (reverting that patch is not possible,
>>> and only check nodatacow for quota enabled case is very bug prune due to
>>> quite a lot code change).
>>>
>>> We may solve it using the new ticketed space reservation facility, but
>>> that won't come into fruit anytime soon.
>>>
>>> So let's just remove that test case from 'auto' group, but still keep
>>> the test case to inform we still have a lot of work to do.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>
>> I'd like to see an ACK from btrfs folks. Thanks!
>>
>> Eryu
>>
>>> ---
>>>  tests/btrfs/group | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/btrfs/group b/tests/btrfs/group
>>> index 697b6a38ea00..3c554a194742 100644
>>> --- a/tests/btrfs/group
>>> +++ b/tests/btrfs/group
>>> @@ -155,7 +155,7 @@
>>>  150 auto quick dangerous
>>>  151 auto quick volume
>>>  152 auto quick metadata qgroup send
>>> -153 auto quick qgroup limit
>>> +153 quick qgroup limit
> 
> Hmm, if removing from auto it might make sense to also remove it
> from quick, because people often use quick as a sanity regression group.

That's also one of my concern.

However recently I tend to run more same VMs on different ranges of
fstests to speed up the testing progress other than using 'quick' group.

Anyway this depends on the end users (QA and developers).

> 
> The issue at hand is a recurring pattern.
> It is also been discussed recently about generic/484:
> https://lore.kernel.org/fstests/20200131164619.GA13005@infradead.org/
> 
> I also handled something like this with:
> fdb69864 overlay/061: remove from auto and quick groups
> 
> I suggest adding a group 'broken' to mark known/wontfix issues
> then a default regression test could run -g auto -x broken
> or -g quick -x broken for a quick regression sanity.

That's much better.

Just one question, if a test is in both quick and broken group, will -g
quick -x broken still exclude it?

Thanks,
Qu

> 
> Thoughts?
> 
> Amir.
>
Amir Goldstein Feb. 1, 2020, 1:07 p.m. UTC | #4
On Sat, Feb 1, 2020 at 2:39 PM Qu Wenruo <wqu@suse.com> wrote:
>
>
>
> On 2020/2/1 下午7:10, Amir Goldstein wrote:
> > On Sat, Feb 1, 2020 at 9:41 AM Eryu Guan <guaneryu@gmail.com> wrote:
> >>
> >> On Tue, Jan 14, 2020 at 08:50:44PM +0800, Qu Wenruo wrote:
> >>> This test case always fail after commit c6887cd11149 ("Btrfs: don't do
> >>> nocow check unless we have to").
> >>> As btrfs no longer checks nodatacow at buffered write time.
> >>>
> >>> That commits brings in a big performance enhancement, as that check is
> >>> not cheap, but breaks qgroup, as write into preallocated space now needs
> >>> extra space.
> >>>
> >>> There isn't yet a good solution (reverting that patch is not possible,
> >>> and only check nodatacow for quota enabled case is very bug prune due to
> >>> quite a lot code change).
> >>>
> >>> We may solve it using the new ticketed space reservation facility, but
> >>> that won't come into fruit anytime soon.
> >>>
> >>> So let's just remove that test case from 'auto' group, but still keep
> >>> the test case to inform we still have a lot of work to do.
> >>>
> >>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >>
> >> I'd like to see an ACK from btrfs folks. Thanks!
> >>
> >> Eryu
> >>
> >>> ---
> >>>  tests/btrfs/group | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tests/btrfs/group b/tests/btrfs/group
> >>> index 697b6a38ea00..3c554a194742 100644
> >>> --- a/tests/btrfs/group
> >>> +++ b/tests/btrfs/group
> >>> @@ -155,7 +155,7 @@
> >>>  150 auto quick dangerous
> >>>  151 auto quick volume
> >>>  152 auto quick metadata qgroup send
> >>> -153 auto quick qgroup limit
> >>> +153 quick qgroup limit
> >
> > Hmm, if removing from auto it might make sense to also remove it
> > from quick, because people often use quick as a sanity regression group.
>
> That's also one of my concern.
>
> However recently I tend to run more same VMs on different ranges of
> fstests to speed up the testing progress other than using 'quick' group.
>
> Anyway this depends on the end users (QA and developers).
>
> >
> > The issue at hand is a recurring pattern.
> > It is also been discussed recently about generic/484:
> > https://lore.kernel.org/fstests/20200131164619.GA13005@infradead.org/
> >
> > I also handled something like this with:
> > fdb69864 overlay/061: remove from auto and quick groups
> >
> > I suggest adding a group 'broken' to mark known/wontfix issues
> > then a default regression test could run -g auto -x broken
> > or -g quick -x broken for a quick regression sanity.
>
> That's much better.
>
> Just one question, if a test is in both quick and broken group, will -g
> quick -x broken still exclude it?
>

Yes.
That is the main use case for -x. Often used with -x dangerous.

What we don't have, AFAIK, is the ability to request 'auto && quick'.

Thanks,
Amir.

Patch
diff mbox series

diff --git a/tests/btrfs/group b/tests/btrfs/group
index 697b6a38ea00..3c554a194742 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -155,7 +155,7 @@ 
 150 auto quick dangerous
 151 auto quick volume
 152 auto quick metadata qgroup send
-153 auto quick qgroup limit
+153 quick qgroup limit
 154 auto quick volume
 155 auto quick send
 156 auto quick trim