FSTests Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] fstests: btrfs/153: Remove it from auto group
@ 2020-01-14 12:50 Qu Wenruo
  2020-02-01  7:36 ` Eryu Guan
  0 siblings, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2020-01-14 12:50 UTC (permalink / raw)
  To: fstests, linux-btrfs

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(-)

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] fstests: btrfs/153: Remove it from auto group
  2020-01-14 12:50 [PATCH] fstests: btrfs/153: Remove it from auto group Qu Wenruo
@ 2020-02-01  7:36 ` Eryu Guan
  2020-02-01 11:10   ` Amir Goldstein
  0 siblings, 1 reply; 5+ messages in thread
From: Eryu Guan @ 2020-02-01  7:36 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: fstests, linux-btrfs

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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] fstests: btrfs/153: Remove it from auto group
  2020-02-01  7:36 ` Eryu Guan
@ 2020-02-01 11:10   ` Amir Goldstein
  2020-02-01 12:38     ` Qu Wenruo
  0 siblings, 1 reply; 5+ messages in thread
From: Amir Goldstein @ 2020-02-01 11:10 UTC (permalink / raw)
  To: Eryu Guan; +Cc: Qu Wenruo, fstests, Linux Btrfs

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] fstests: btrfs/153: Remove it from auto group
  2020-02-01 11:10   ` Amir Goldstein
@ 2020-02-01 12:38     ` Qu Wenruo
  2020-02-01 13:07       ` Amir Goldstein
  0 siblings, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2020-02-01 12:38 UTC (permalink / raw)
  To: Amir Goldstein, Eryu Guan; +Cc: fstests, Linux Btrfs



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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] fstests: btrfs/153: Remove it from auto group
  2020-02-01 12:38     ` Qu Wenruo
@ 2020-02-01 13:07       ` Amir Goldstein
  0 siblings, 0 replies; 5+ messages in thread
From: Amir Goldstein @ 2020-02-01 13:07 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: Eryu Guan, fstests, Linux Btrfs

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-14 12:50 [PATCH] fstests: btrfs/153: Remove it from auto group Qu Wenruo
2020-02-01  7:36 ` Eryu Guan
2020-02-01 11:10   ` Amir Goldstein
2020-02-01 12:38     ` Qu Wenruo
2020-02-01 13:07       ` Amir Goldstein

FSTests Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/fstests/0 fstests/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 fstests fstests/ https://lore.kernel.org/fstests \
		fstests@vger.kernel.org
	public-inbox-index fstests

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.fstests


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git