[PATCH] fstests: generic/500 doesn't work for btrfs

[PATCH] fstests: generic/500 doesn't work for btrfs

[Josef Bacik]

Btrfs does COW, so when we unlink the file we need to update metadata
and write it to a new location, which we can't do because the thinp is
full.  This results in an EIO during a metadata write, which makes us
flip read only, thus making it impossible to fstrim the fs.  Just make
it so we skip this test for btrfs.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 tests/generic/500 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/generic/500 b/tests/generic/500
index 201d8b9f..5cd7126f 100755
--- a/tests/generic/500
+++ b/tests/generic/500
@@ -49,6 +49,12 @@ _supported_os Linux
 _require_scratch_nocheck
 _require_dm_target thin-pool
 
+# The unlink below will result in new metadata blocks for btrfs because of CoW,
+# and since we've filled the thinp device it'll return EIO, which will make
+# btrfs flip read only, making it fail this test when it just won't work right
+# for us in the first place.
+test $FSTYP == "btrfs"  && _notrun "btrfs doesn't work that way lol"
+
 # Require underlying device support discard
 _scratch_mkfs >>$seqres.full 2>&1
 _scratch_mount
-- 
2.21.0

Re: [PATCH] fstests: generic/500 doesn't work for btrfs

[Darrick J. Wong]

On Thu, Aug 15, 2019 at 02:26:59PM -0400, Josef Bacik wrote:
> Btrfs does COW, so when we unlink the file we need to update metadata
> and write it to a new location, which we can't do because the thinp is
> full.  This results in an EIO during a metadata write, which makes us
> flip read only, thus making it impossible to fstrim the fs.  Just make
> it so we skip this test for btrfs.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  tests/generic/500 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/generic/500 b/tests/generic/500
> index 201d8b9f..5cd7126f 100755
> --- a/tests/generic/500
> +++ b/tests/generic/500
> @@ -49,6 +49,12 @@ _supported_os Linux
>  _require_scratch_nocheck
>  _require_dm_target thin-pool
>  
> +# The unlink below will result in new metadata blocks for btrfs because of CoW,
> +# and since we've filled the thinp device it'll return EIO, which will make
> +# btrfs flip read only, making it fail this test when it just won't work right
> +# for us in the first place.
> +test $FSTYP == "btrfs"  && _notrun "btrfs doesn't work that way lol"

I did it for the lulz,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> +
>  # Require underlying device support discard
>  _scratch_mkfs >>$seqres.full 2>&1
>  _scratch_mount
> -- 
> 2.21.0
> 

Re: [PATCH] fstests: generic/500 doesn't work for btrfs

[Eryu Guan]

On Thu, Aug 15, 2019 at 02:26:59PM -0400, Josef Bacik wrote:
> Btrfs does COW, so when we unlink the file we need to update metadata
> and write it to a new location, which we can't do because the thinp is
> full.  This results in an EIO during a metadata write, which makes us
> flip read only, thus making it impossible to fstrim the fs.  Just make
> it so we skip this test for btrfs.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  tests/generic/500 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/generic/500 b/tests/generic/500
> index 201d8b9f..5cd7126f 100755
> --- a/tests/generic/500
> +++ b/tests/generic/500
> @@ -49,6 +49,12 @@ _supported_os Linux
>  _require_scratch_nocheck
>  _require_dm_target thin-pool
>  
> +# The unlink below will result in new metadata blocks for btrfs because of CoW,
> +# and since we've filled the thinp device it'll return EIO, which will make
> +# btrfs flip read only, making it fail this test when it just won't work right
> +# for us in the first place.
> +test $FSTYP == "btrfs"  && _notrun "btrfs doesn't work that way lol"

I'm wondering if we could introduce a proper _require rule to cover this
case? e.g. require the fs doesn't allocate new blocks on unlink? or
something like that. But I'm not sure what's the proper fs feature to
require here, any suggestions?

Thanks,
Eryu

> +
>  # Require underlying device support discard
>  _scratch_mkfs >>$seqres.full 2>&1
>  _scratch_mount
> -- 
> 2.21.0
> 

Re: [PATCH] fstests: generic/500 doesn't work for btrfs

[Darrick J. Wong]

On Sun, Aug 18, 2019 at 11:44:28PM +0800, Eryu Guan wrote:
> On Thu, Aug 15, 2019 at 02:26:59PM -0400, Josef Bacik wrote:
> > Btrfs does COW, so when we unlink the file we need to update metadata
> > and write it to a new location, which we can't do because the thinp is
> > full.  This results in an EIO during a metadata write, which makes us
> > flip read only, thus making it impossible to fstrim the fs.  Just make
> > it so we skip this test for btrfs.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  tests/generic/500 | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/generic/500 b/tests/generic/500
> > index 201d8b9f..5cd7126f 100755
> > --- a/tests/generic/500
> > +++ b/tests/generic/500
> > @@ -49,6 +49,12 @@ _supported_os Linux
> >  _require_scratch_nocheck
> >  _require_dm_target thin-pool
> >  
> > +# The unlink below will result in new metadata blocks for btrfs because of CoW,
> > +# and since we've filled the thinp device it'll return EIO, which will make
> > +# btrfs flip read only, making it fail this test when it just won't work right
> > +# for us in the first place.
> > +test $FSTYP == "btrfs"  && _notrun "btrfs doesn't work that way lol"
> 
> I'm wondering if we could introduce a proper _require rule to cover this
> case? e.g. require the fs doesn't allocate new blocks on unlink? or
> something like that. But I'm not sure what's the proper fs feature to
> require here, any suggestions?

I'd be careful with this -- xfs can allocate new metadata blocks on
unlink too -- changes in the free space btrees, expansion of the free
inode btree, etc.  For the 20 inodes in play in g/500 this won't be the
case, but if you had a test that created 20,000 inodes, then that could
happen.

--D

> Thanks,
> Eryu
> 
> > +
> >  # Require underlying device support discard
> >  _scratch_mkfs >>$seqres.full 2>&1
> >  _scratch_mount
> > -- 
> > 2.21.0
> >