| Message ID | 20190815182659.27875-1-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: generic/500 doesn't work for btrfs | expand |
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 >
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 >
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 > >
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
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(+)