btrfs/172,206: call _log_writes_cleanup in _cleanup

Commit Message

Su Yue Feb. 15, 2024, 2:02 p.m. UTC

From: Su Yue <[e-mail]>

Because block group tree requires require no-holes feature,
_log_writes_mkfs "-O ^no-holes" fails when "-O block-group-tree" is
given in MKFS_OPTION.
Without explicit _log_writes_cleanup, the two tests fail with
logwrites-test device left. And all next tests will fail due to
SCRATCH DEVICE EBUSY.

Fix it by overriding _cleanup to call _log_writes_cleanup.

Signed-off-by: Su Yue <[e-mail]>
---
 tests/btrfs/172 | 6 ++++++
 tests/btrfs/206 | 6 ++++++
 2 files changed, 12 insertions(+)

Comments

Filipe Manana Feb. 15, 2024, 3:58 p.m. UTC | #1

On Thu, Feb 15, 2024 at 2:04 PM Su Yue <[e-mail]> wrote:
>
> From: Su Yue <[e-mail]>
>
> Because block group tree requires require no-holes feature,
> _log_writes_mkfs "-O ^no-holes" fails when "-O block-group-tree" is
> given in MKFS_OPTION.
> Without explicit _log_writes_cleanup, the two tests fail with
> logwrites-test device left. And all next tests will fail due to
> SCRATCH DEVICE EBUSY.
>
> Fix it by overriding _cleanup to call _log_writes_cleanup.
>
> Signed-off-by: Su Yue <[e-mail]>

Reviewed-by: Filipe Manana <[e-mail]>

Looks good, just one comment below.

> ---
>  tests/btrfs/172 | 6 ++++++
>  tests/btrfs/206 | 6 ++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/tests/btrfs/172 b/tests/btrfs/172
> index f5acc6982cd7..fceff56c9d37 100755
> --- a/tests/btrfs/172
> +++ b/tests/btrfs/172
> @@ -13,6 +13,12 @@
>  . ./common/preamble
>  _begin_fstest auto quick log replay recoveryloop
>
> +# Override the default cleanup function.

You don't need to add this comment.
Same for the other test.

Thanks.

> +_cleanup()
> +{
> +       _log_writes_cleanup &> /dev/null
> +}
> +
>  # Import common functions.
>  . ./common/filter
>  . ./common/dmlogwrites
> diff --git a/tests/btrfs/206 b/tests/btrfs/206
> index f6571649076f..e05adf75b67e 100755
> --- a/tests/btrfs/206
> +++ b/tests/btrfs/206
> @@ -14,6 +14,12 @@
>  . ./common/preamble
>  _begin_fstest auto quick log replay recoveryloop punch prealloc
>
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +       _log_writes_cleanup &> /dev/null
> +}
> +
>  # Import common functions.
>  . ./common/filter
>  . ./common/dmlogwrites
> --
> 2.43.0
>
>

Qu Wenruo Feb. 15, 2024, 11:10 p.m. UTC | #2

在 2024/2/16 00:32, Su Yue 写道:
> From: Su Yue <[e-mail]>
>
> Because block group tree requires require no-holes feature,
> _log_writes_mkfs "-O ^no-holes" fails when "-O block-group-tree" is
> given in MKFS_OPTION.
> Without explicit _log_writes_cleanup, the two tests fail with
> logwrites-test device left. And all next tests will fail due to
> SCRATCH DEVICE EBUSY.
>
> Fix it by overriding _cleanup to call _log_writes_cleanup.
>
> Signed-off-by: Su Yue <[e-mail]>

Reviewed-by: Qu Wenruo <[e-mail]>

Thanks,
Qu
> ---
>   tests/btrfs/172 | 6 ++++++
>   tests/btrfs/206 | 6 ++++++
>   2 files changed, 12 insertions(+)
>
> diff --git a/tests/btrfs/172 b/tests/btrfs/172
> index f5acc6982cd7..fceff56c9d37 100755
> --- a/tests/btrfs/172
> +++ b/tests/btrfs/172
> @@ -13,6 +13,12 @@
>   . ./common/preamble
>   _begin_fstest auto quick log replay recoveryloop
>
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	_log_writes_cleanup &> /dev/null
> +}
> +
>   # Import common functions.
>   . ./common/filter
>   . ./common/dmlogwrites
> diff --git a/tests/btrfs/206 b/tests/btrfs/206
> index f6571649076f..e05adf75b67e 100755
> --- a/tests/btrfs/206
> +++ b/tests/btrfs/206
> @@ -14,6 +14,12 @@
>   . ./common/preamble
>   _begin_fstest auto quick log replay recoveryloop punch prealloc
>
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	_log_writes_cleanup &> /dev/null
> +}
> +
>   # Import common functions.
>   . ./common/filter
>   . ./common/dmlogwrites

Zorro Lang March 1, 2024, 1:49 p.m. UTC | #3

On Thu, Feb 15, 2024 at 10:02:36PM +0800, Su Yue wrote:
> From: Su Yue <[e-mail]>
> 
> Because block group tree requires require no-holes feature,
> _log_writes_mkfs "-O ^no-holes" fails when "-O block-group-tree" is
> given in MKFS_OPTION.
> Without explicit _log_writes_cleanup, the two tests fail with
> logwrites-test device left. And all next tests will fail due to
> SCRATCH DEVICE EBUSY.
> 
> Fix it by overriding _cleanup to call _log_writes_cleanup.
> 
> Signed-off-by: Su Yue <[e-mail]>
> ---
>  tests/btrfs/172 | 6 ++++++
>  tests/btrfs/206 | 6 ++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/tests/btrfs/172 b/tests/btrfs/172
> index f5acc6982cd7..fceff56c9d37 100755
> --- a/tests/btrfs/172
> +++ b/tests/btrfs/172
> @@ -13,6 +13,12 @@
>  . ./common/preamble
>  _begin_fstest auto quick log replay recoveryloop
>  
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	_log_writes_cleanup &> /dev/null

This _cleanup will override the default one, so better to copy the
default cleanup in this function,

  cd /
  rm -r -f $tmp.*

You can refer to btrfs/196 or generic/482 etc.

> +}
> +
>  # Import common functions.
>  . ./common/filter
>  . ./common/dmlogwrites
> diff --git a/tests/btrfs/206 b/tests/btrfs/206
> index f6571649076f..e05adf75b67e 100755
> --- a/tests/btrfs/206
> +++ b/tests/btrfs/206
> @@ -14,6 +14,12 @@
>  . ./common/preamble
>  _begin_fstest auto quick log replay recoveryloop punch prealloc
>  
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	_log_writes_cleanup &> /dev/null


Same as above.

Thanks,
Zorro

> +}
> +
>  # Import common functions.
>  . ./common/filter
>  . ./common/dmlogwrites
> -- 
> 2.43.0
> 
>

Filipe Manana March 1, 2024, 2:55 p.m. UTC | #4

On Fri, Mar 1, 2024 at 1:49 PM Zorro Lang <[e-mail]> wrote:
>
> On Thu, Feb 15, 2024 at 10:02:36PM +0800, Su Yue wrote:
> > From: Su Yue <[e-mail]>
> >
> > Because block group tree requires require no-holes feature,
> > _log_writes_mkfs "-O ^no-holes" fails when "-O block-group-tree" is
> > given in MKFS_OPTION.
> > Without explicit _log_writes_cleanup, the two tests fail with
> > logwrites-test device left. And all next tests will fail due to
> > SCRATCH DEVICE EBUSY.
> >
> > Fix it by overriding _cleanup to call _log_writes_cleanup.
> >
> > Signed-off-by: Su Yue <[e-mail]>
> > ---
> >  tests/btrfs/172 | 6 ++++++
> >  tests/btrfs/206 | 6 ++++++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/tests/btrfs/172 b/tests/btrfs/172
> > index f5acc6982cd7..fceff56c9d37 100755
> > --- a/tests/btrfs/172
> > +++ b/tests/btrfs/172
> > @@ -13,6 +13,12 @@
> >  . ./common/preamble
> >  _begin_fstest auto quick log replay recoveryloop
> >
> > +# Override the default cleanup function.
> > +_cleanup()
> > +{
> > +     _log_writes_cleanup &> /dev/null
>
> This _cleanup will override the default one, so better to copy the
> default cleanup in this function,
>
>   cd /
>   rm -r -f $tmp.*

Zorro,

You had already replied to v2 of this patch with exactly the same comments:

https://lore.kernel.org/fstests/[e-mail]/

It's trivial to do those changes.
Do you expect Su to send yet another version just for that, or could
you amend the patch?

Can you please be more clear in future replies about that, i.e. if you
expect the author to send a new version or if you'll amend the patch
for trivial changes?

Speaking for myself, I very often get confused with your replies, and
I feel that some patches often get stalled for that reason.
Usually with Eryu or Dave that didn't happen, the course of action was clear.

Thanks.

>
> You can refer to btrfs/196 or generic/482 etc.
>
> > +}
> > +
> >  # Import common functions.
> >  . ./common/filter
> >  . ./common/dmlogwrites
> > diff --git a/tests/btrfs/206 b/tests/btrfs/206
> > index f6571649076f..e05adf75b67e 100755
> > --- a/tests/btrfs/206
> > +++ b/tests/btrfs/206
> > @@ -14,6 +14,12 @@
> >  . ./common/preamble
> >  _begin_fstest auto quick log replay recoveryloop punch prealloc
> >
> > +# Override the default cleanup function.
> > +_cleanup()
> > +{
> > +     _log_writes_cleanup &> /dev/null
>
>
> Same as above.
>
> Thanks,
> Zorro
>
> > +}
> > +
> >  # Import common functions.
> >  . ./common/filter
> >  . ./common/dmlogwrites
> > --
> > 2.43.0
> >
> >
>
>

Zorro Lang March 2, 2024, 7:03 a.m. UTC | #5

On Fri, Mar 01, 2024 at 02:55:26PM +0000, Filipe Manana wrote:
> On Fri, Mar 1, 2024 at 1:49 PM Zorro Lang <[e-mail]> wrote:
> >
> > On Thu, Feb 15, 2024 at 10:02:36PM +0800, Su Yue wrote:
> > > From: Su Yue <[e-mail]>
> > >
> > > Because block group tree requires require no-holes feature,
> > > _log_writes_mkfs "-O ^no-holes" fails when "-O block-group-tree" is
> > > given in MKFS_OPTION.
> > > Without explicit _log_writes_cleanup, the two tests fail with
> > > logwrites-test device left. And all next tests will fail due to
> > > SCRATCH DEVICE EBUSY.
> > >
> > > Fix it by overriding _cleanup to call _log_writes_cleanup.
> > >
> > > Signed-off-by: Su Yue <[e-mail]>
> > > ---
> > >  tests/btrfs/172 | 6 ++++++
> > >  tests/btrfs/206 | 6 ++++++
> > >  2 files changed, 12 insertions(+)
> > >
> > > diff --git a/tests/btrfs/172 b/tests/btrfs/172
> > > index f5acc6982cd7..fceff56c9d37 100755
> > > --- a/tests/btrfs/172
> > > +++ b/tests/btrfs/172
> > > @@ -13,6 +13,12 @@
> > >  . ./common/preamble
> > >  _begin_fstest auto quick log replay recoveryloop
> > >
> > > +# Override the default cleanup function.
> > > +_cleanup()
> > > +{
> > > +     _log_writes_cleanup &> /dev/null
> >
> > This _cleanup will override the default one, so better to copy the
> > default cleanup in this function,
> >
> >   cd /
> >   rm -r -f $tmp.*
> 
> Zorro,
> 
> You had already replied to v2 of this patch with exactly the same comments:
> 
> https://lore.kernel.org/fstests/[e-mail]/

Hi Filipe,

Oh, sorry for this duplicated review, I tried to scan the email list to
make sure I didn't miss something.

> 
> It's trivial to do those changes.
> Do you expect Su to send yet another version just for that, or could
> you amend the patch?

I expect to get a simple response from the author, for his patch.
E.g.:
1) "Yes, agree, could you help to change that when you merge it?"
2) "No, I think it doesn't make sense"
3) "Sure, I'll make a bit change in next version"
4) "Oh, that reminds me, I should cleanup more, I think I can change more about that"
...

Is that hard?

If the author doesn't have any opinions, just asks for the amending from me,
I'll do that if there's not risk. Or I suppose the author always hope to change
his code by himself, maybe not follow the review points 100%.

> 
> Can you please be more clear in future replies about that, i.e. if you
> expect the author to send a new version or if you'll amend the patch
> for trivial changes?

You think it's trivial, due to you think you don't need more changes about
that. But I'm not sure before you say that. You can check the list, I say
"I'll help to change that when I merge it", but just when I'm sure it's clear
enough. You shouldn't use your measure to think my standard.

I didn't say I'll amend that means my review point is a reference, to check
if you have more ideas.
We just got a big regression in a _cleanup function which does:
  cd /
  rm -r -f $tmp.*
  specific_xxxx_cleanup

Then that specific_xxxx_cleanup() remove all things in "/".

If it's
  specific_xxxx_cleanup
  cd /
  rm -r -f $tmp.*

things might be different or not.

I'm not saying that change must makes differences. But the cleanup things
(e.g. theorder of the steps) might worth a bit evaluating, not just changed
by myself without any response. And a reply from the author to the review
points of his patch is not hard. Even just say "yes, please help that",
that means you reviewed and agreed my review points at least.

I'm not anyone else. Each one has his distinctive style. I don't know what's
wrong with that way to deal with patches, to make you such unsatisfied. It is
difficult to cater to all tastes.

Thanks,
Zorro

> 
> Speaking for myself, I very often get confused with your replies, and
> I feel that some patches often get stalled for that reason.
> Usually with Eryu or Dave that didn't happen, the course of action was clear.
> 
> Thanks.
> 
> >
> > You can refer to btrfs/196 or generic/482 etc.
> >
> > > +}
> > > +
> > >  # Import common functions.
> > >  . ./common/filter
> > >  . ./common/dmlogwrites
> > > diff --git a/tests/btrfs/206 b/tests/btrfs/206
> > > index f6571649076f..e05adf75b67e 100755
> > > --- a/tests/btrfs/206
> > > +++ b/tests/btrfs/206
> > > @@ -14,6 +14,12 @@
> > >  . ./common/preamble
> > >  _begin_fstest auto quick log replay recoveryloop punch prealloc
> > >
> > > +# Override the default cleanup function.
> > > +_cleanup()
> > > +{
> > > +     _log_writes_cleanup &> /dev/null
> >
> >
> > Same as above.
> >
> > Thanks,
> > Zorro
> >
> > > +}
> > > +
> > >  # Import common functions.
> > >  . ./common/filter
> > >  . ./common/dmlogwrites
> > > --
> > > 2.43.0
> > >
> > >
> >
> >
>

Patch

diff --git a/tests/btrfs/172 b/tests/btrfs/172
index f5acc6982cd7..fceff56c9d37 100755
--- a/tests/btrfs/172
+++ b/tests/btrfs/172
@@ -13,6 +13,12 @@ 
 . ./common/preamble
 _begin_fstest auto quick log replay recoveryloop
 
+# Override the default cleanup function.
+_cleanup()
+{
+	_log_writes_cleanup &> /dev/null
+}
+
 # Import common functions.
 . ./common/filter
 . ./common/dmlogwrites
diff --git a/tests/btrfs/206 b/tests/btrfs/206
index f6571649076f..e05adf75b67e 100755
--- a/tests/btrfs/206
+++ b/tests/btrfs/206
@@ -14,6 +14,12 @@ 
 . ./common/preamble
 _begin_fstest auto quick log replay recoveryloop punch prealloc
 
+# Override the default cleanup function.
+_cleanup()
+{
+	_log_writes_cleanup &> /dev/null
+}
+
 # Import common functions.
 . ./common/filter
 . ./common/dmlogwrites