diff mbox

xfs test: Buffer resubmittion test

Message ID 20170703153200.14423-1-cmaiolino@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Carlos Maiolino July 3, 2017, 3:32 p.m. UTC 
Hi folks,

this is the xfstests case for the buffer resubmition failures with
dm-thin I'm working on.

I used test 999 to avoid the need of changing the test number between
revisions. I don't write a xfstest for a while, so I am not sure if the
test is ok as it is.

It is doing its job though :) Passing with a kernel using my last
patches for the problem (although they still need revision), and hanging
up when the test is executed with a kernel without my patches, so be
careful to try it.

Thoughts?

Cheers

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 tests/xfs/999     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/999.out |   2 ++
 tests/xfs/group   |   1 +
 3 files changed, 105 insertions(+)
 create mode 100755 tests/xfs/999
 create mode 100644 tests/xfs/999.out

Comments

Brian Foster July 5, 2017, 12:50 p.m. UTC | #1 
On Mon, Jul 03, 2017 at 05:32:00PM +0200, Carlos Maiolino wrote:
> Hi folks,
> 
> this is the xfstests case for the buffer resubmition failures with
> dm-thin I'm working on.
> 
> I used test 999 to avoid the need of changing the test number between
> revisions. I don't write a xfstest for a while, so I am not sure if the
> test is ok as it is.
> 
> It is doing its job though :) Passing with a kernel using my last
> patches for the problem (although they still need revision), and hanging
> up when the test is executed with a kernel without my patches, so be
> careful to try it.
> 
> Thoughts?
> 
> Cheers
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  tests/xfs/999     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/999.out |   2 ++
>  tests/xfs/group   |   1 +
>  3 files changed, 105 insertions(+)
>  create mode 100755 tests/xfs/999
>  create mode 100644 tests/xfs/999.out
> 
> diff --git a/tests/xfs/999 b/tests/xfs/999
> new file mode 100755
> index 0000000..5ae8b74
> --- /dev/null
> +++ b/tests/xfs/999
> @@ -0,0 +1,102 @@
> +#! /bin/bash
> +# FS QA Test 999
> +#
> +# Test buffer resubmission after a failed writeback with to a full overcommited
> +# dm-thin device. 

FYI: trailing space on the line above. A bit more information about the
problem this is testing for would be useful as well.

> +#
> +# This test will hang the filesystem when ran on an unpatched kernel
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017 YOUR NAME HERE.  All Rights Reserved.
> +#

Needs a copyright. :)

> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +	$UMOUNT_PROG $mnt >/dev/null 2>&1
> +	$LVM_PROG vgremove -ff $vgname >>$seqres.full 2>&1
> +	$LVM_PROG pvremove -ff $SCRATCH_DEV >>$seqres.full 2>&1
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs xfs
> +_supported_os Linux
> +_require_scratch_nocheck
> +_require_dm_target thin-pool
> +_require_command $LVM_PROG lvm
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +vgname=vg_$seq
> +lvname=lv_$seq
> +poolname=pool_$seq
> +snapname=snap_$seq
> +origpsize=100
> +virtsize=200
> +newpsize=200
> +mnt=$SCRATCH_DIR/mnt_$seq

I think this should be $SCRATCH_MNT.

> +mkdir -p $mnt
> +

We haven't mounted scratch at this point so it looks like this creates a
directory on the local rootfs..? Looking ahead.. if we're not really
going to mount the scratch dev directly, could we just use $SCRATCH_MNT
as the mountpoint?

> +#Ensure we have enough disk space
> +_scratch_mkfs_sized $((250 * 1024 * 1024)) >>$seqres.full 2>&1
> +
> +$LVM_PROG pvcreate -f $SCRATCH_DEV >/dev/null 2>&1
> +$LVM_PROG vgcreate -f $vgname $SCRATCH_DEV >/dev/null 2>&1
> +
> +$LVM_PROG lvcreate  --thinpool $poolname  --errorwhenfull y --zero n \
> +-L $origpsize --poolmetadatasize 4M $vgname >/dev/null 2>&1
> +
> +$LVM_PROG lvcreate  --virtualsize $virtsize -T $vgname/$poolname \
> +-n $lvname >/dev/null 2>&1

The above multi-line commands could use some indentation and perhaps
some one-liner comments to explain what's going on. E.g., "create an
overprovisioned thin volume."

Also, perhaps some of the /dev/null redirection should go to
$seqres.full..?

> +
> +_mkfs_dev /dev/mapper/$vgname-$lvname >/dev/null 2>&1
> +
> +$LVM_PROG lvcreate  -k n -s $vgname/$lvname -n $snapname >/dev/null 2>&1
> +
> +_mount /dev/mapper/$vgname-$snapname $mnt
> +

"Consume all of the space in the volume and freeze to ensure everything
required to make the fs consistent is flushed to disk. Note that this
may hang on affected XFS filesystems."

> +xfs_io -f -d -c 'pwrite -b 1m 0 120m' $mnt/f1 >/dev/null 2>&1
> +
> +fsfreeze -f $mnt &
> +
> +#Wait fsfreeze to its job
> +sleep 10

I think you could use 'wait' here rather than a sleep. But is there a
reason we need to put the freeze in the background in the first place?
It seems to me this test will either complete or hang regardless. ;P

> +

"extend the volume with sufficient space and unfreeze ..."

> +lvextend -L $newpsize $vgname/$poolname >/dev/null 2>&1
> +
> +fsfreeze -u $mnt
> +echo "Test OK"
> +
> +status=0
> +exit
> diff --git a/tests/xfs/999.out b/tests/xfs/999.out
> new file mode 100644
> index 0000000..8c3c938
> --- /dev/null
> +++ b/tests/xfs/999.out
> @@ -0,0 +1,2 @@
> +QA output created by 999
> +Test OK
> diff --git a/tests/xfs/group b/tests/xfs/group
> index 792161a..2bde916 100644
> --- a/tests/xfs/group
> +++ b/tests/xfs/group
> @@ -416,3 +416,4 @@
>  416 dangerous_fuzzers dangerous_scrub dangerous_repair
>  417 dangerous_fuzzers dangerous_scrub dangerous_online_repair
>  418 dangerous_fuzzers dangerous_scrub dangerous_repair
> +999 dangerous

I think the auto group makes sense too.

Also, since the discussion around having a couple tests here (one using
error injection and this one using dm-thin) it seems like this test
could be more suited to a generic test.  Nothing in the test really
depends on a particular filesystem. The header comment could simply be
updated to explain the specific XFS issue as the inspiration and that
the test generically exercises the ability to recover/continue after a
dm-thin ENOPSPC and subsequent volume extend. Thoughts?

Brian

> -- 
> 2.9.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong July 5, 2017, 4:17 p.m. UTC | #2 
On Wed, Jul 05, 2017 at 08:50:16AM -0400, Brian Foster wrote:
> On Mon, Jul 03, 2017 at 05:32:00PM +0200, Carlos Maiolino wrote:
> > Hi folks,
> > 
> > this is the xfstests case for the buffer resubmition failures with
> > dm-thin I'm working on.
> > 
> > I used test 999 to avoid the need of changing the test number between
> > revisions. I don't write a xfstest for a while, so I am not sure if the
> > test is ok as it is.
> > 
> > It is doing its job though :) Passing with a kernel using my last
> > patches for the problem (although they still need revision), and hanging
> > up when the test is executed with a kernel without my patches, so be
> > careful to try it.
> > 
> > Thoughts?
> > 
> > Cheers
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  tests/xfs/999     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/999.out |   2 ++
> >  tests/xfs/group   |   1 +
> >  3 files changed, 105 insertions(+)
> >  create mode 100755 tests/xfs/999
> >  create mode 100644 tests/xfs/999.out
> > 
> > diff --git a/tests/xfs/999 b/tests/xfs/999
> > new file mode 100755
> > index 0000000..5ae8b74
> > --- /dev/null
> > +++ b/tests/xfs/999
> > @@ -0,0 +1,102 @@
> > +#! /bin/bash
> > +# FS QA Test 999
> > +#
> > +# Test buffer resubmission after a failed writeback with to a full overcommited
> > +# dm-thin device. 
> 
> FYI: trailing space on the line above. A bit more information about the
> problem this is testing for would be useful as well.
> 
> > +#
> > +# This test will hang the filesystem when ran on an unpatched kernel
> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2017 YOUR NAME HERE.  All Rights Reserved.
> > +#
> 
> Needs a copyright. :)
> 
> > +# This program is free software; you can redistribute it and/or
> > +# modify it under the terms of the GNU General Public License as
> > +# published by the Free Software Foundation.
> > +#
> > +# This program is distributed in the hope that it would be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program; if not, write the Free Software Foundation,
> > +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > +#-----------------------------------------------------------------------
> > +#
> > +
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1	# failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $tmp.*
> > +	$UMOUNT_PROG $mnt >/dev/null 2>&1
> > +	$LVM_PROG vgremove -ff $vgname >>$seqres.full 2>&1
> > +	$LVM_PROG pvremove -ff $SCRATCH_DEV >>$seqres.full 2>&1
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs xfs
> > +_supported_os Linux
> > +_require_scratch_nocheck
> > +_require_dm_target thin-pool
> > +_require_command $LVM_PROG lvm
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +vgname=vg_$seq
> > +lvname=lv_$seq
> > +poolname=pool_$seq
> > +snapname=snap_$seq
> > +origpsize=100
> > +virtsize=200
> > +newpsize=200
> > +mnt=$SCRATCH_DIR/mnt_$seq
> 
> I think this should be $SCRATCH_MNT.
> 
> > +mkdir -p $mnt
> > +
> 
> We haven't mounted scratch at this point so it looks like this creates a
> directory on the local rootfs..?

Yes, it does, or at least it would on my test vm were the root fs not ro. :P

> Looking ahead.. if we're not really
> going to mount the scratch dev directly, could we just use $SCRATCH_MNT
> as the mountpoint?

So I gathered. :)

> > +#Ensure we have enough disk space
> > +_scratch_mkfs_sized $((250 * 1024 * 1024)) >>$seqres.full 2>&1
> > +
> > +$LVM_PROG pvcreate -f $SCRATCH_DEV >/dev/null 2>&1
> > +$LVM_PROG vgcreate -f $vgname $SCRATCH_DEV >/dev/null 2>&1
> > +
> > +$LVM_PROG lvcreate  --thinpool $poolname  --errorwhenfull y --zero n \
> > +-L $origpsize --poolmetadatasize 4M $vgname >/dev/null 2>&1
> > +
> > +$LVM_PROG lvcreate  --virtualsize $virtsize -T $vgname/$poolname \
> > +-n $lvname >/dev/null 2>&1
> 
> The above multi-line commands could use some indentation and perhaps
> some one-liner comments to explain what's going on. E.g., "create an
> overprovisioned thin volume."

Yes please!

> Also, perhaps some of the /dev/null redirection should go to
> $seqres.full..?
> 
> > +
> > +_mkfs_dev /dev/mapper/$vgname-$lvname >/dev/null 2>&1
> > +
> > +$LVM_PROG lvcreate  -k n -s $vgname/$lvname -n $snapname >/dev/null 2>&1
> > +
> > +_mount /dev/mapper/$vgname-$snapname $mnt
> > +
> 
> "Consume all of the space in the volume and freeze to ensure everything
> required to make the fs consistent is flushed to disk. Note that this
> may hang on affected XFS filesystems."
> 
> > +xfs_io -f -d -c 'pwrite -b 1m 0 120m' $mnt/f1 >/dev/null 2>&1
> > +
> > +fsfreeze -f $mnt &
> > +
> > +#Wait fsfreeze to its job
> > +sleep 10
> 
> I think you could use 'wait' here rather than a sleep. But is there a
> reason we need to put the freeze in the background in the first place?
> It seems to me this test will either complete or hang regardless. ;P

I had the same thought.

> > +
> 
> "extend the volume with sufficient space and unfreeze ..."
> 
> > +lvextend -L $newpsize $vgname/$poolname >/dev/null 2>&1
> > +
> > +fsfreeze -u $mnt
> > +echo "Test OK"
> > +
> > +status=0
> > +exit
> > diff --git a/tests/xfs/999.out b/tests/xfs/999.out
> > new file mode 100644
> > index 0000000..8c3c938
> > --- /dev/null
> > +++ b/tests/xfs/999.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 999
> > +Test OK
> > diff --git a/tests/xfs/group b/tests/xfs/group
> > index 792161a..2bde916 100644
> > --- a/tests/xfs/group
> > +++ b/tests/xfs/group
> > @@ -416,3 +416,4 @@
> >  416 dangerous_fuzzers dangerous_scrub dangerous_repair
> >  417 dangerous_fuzzers dangerous_scrub dangerous_online_repair
> >  418 dangerous_fuzzers dangerous_scrub dangerous_repair
> > +999 dangerous
> 
> I think the auto group makes sense too.
> 
> Also, since the discussion around having a couple tests here (one using
> error injection and this one using dm-thin) it seems like this test
> could be more suited to a generic test.  Nothing in the test really
> depends on a particular filesystem. The header comment could simply be
> updated to explain the specific XFS issue as the inspiration and that
> the test generically exercises the ability to recover/continue after a
> dm-thin ENOPSPC and subsequent volume extend. Thoughts?

I agree. :)

(new laptop, let's see if this mail even makes it to the ml...)

--D

> Brian
> 
> > -- 
> > 2.9.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Carlos Maiolino July 7, 2017, 9:24 a.m. UTC | #3 
Hi

Thanks for the comments, I agree with all the changes pointed but one inlined
below.

> > > +xfs_io -f -d -c 'pwrite -b 1m 0 120m' $mnt/f1 >/dev/null 2>&1
> > > +
> > > +fsfreeze -f $mnt &
> > > +
> > > +#Wait fsfreeze to its job
> > > +sleep 10
> > 
> > I think you could use 'wait' here rather than a sleep. But is there a
> > reason we need to put the freeze in the background in the first place?
> > It seems to me this test will either complete or hang regardless. ;P
> 
> I had the same thought.

not running fsfreeze in the background won't complete the test. fsfreeze needs
some space in the device to complete, and with a full device it will hang until
the device is extended with lvextend.

The difference between a fixed filesystem and a buggy one, is whether the
filesystem can be properly thawed or not, but both cases will hang in fsfreeze
if the device isn't expanded right after freezing the fs.

The main reason of the freeze, is to force xfsaild to run and push ail items, if
no space is availale, xfsaild will stay stuck:

~ $ sudo cat /proc/4712/stack 
[<ffffffff813f7e16>] xfs_ail_push_all_sync+0xb6/0x100
[<ffffffff813e714f>] xfs_log_quiesce+0x4f/0x3a0
[<ffffffff813dd5cd>] xfs_quiesce_attr+0x6d/0xb0
[<ffffffff813dd660>] xfs_fs_freeze+0x50/0x80
[<ffffffff81246a5f>] freeze_super+0xcf/0x1a0
[<ffffffff81258ca3>] do_vfs_ioctl+0x4b3/0x5e0
[<ffffffff81258e49>] SyS_ioctl+0x79/0x90
[<ffffffff818da677>] entry_SYSCALL_64_fastpath+0x1a/0xa9
[<ffffffffffffffff>] 0xffffffffffffffff


Other than that, I'll agree with everything.

Thanks for the review guys


> 
> > > +
> > 
> > "extend the volume with sufficient space and unfreeze ..."
> > 
> > > +lvextend -L $newpsize $vgname/$poolname >/dev/null 2>&1
> > > +
> > > +fsfreeze -u $mnt
> > > +echo "Test OK"
> > > +
> > > +status=0
> > > +exit
> > > diff --git a/tests/xfs/999.out b/tests/xfs/999.out
> > > new file mode 100644
> > > index 0000000..8c3c938
> > > --- /dev/null
> > > +++ b/tests/xfs/999.out
> > > @@ -0,0 +1,2 @@
> > > +QA output created by 999
> > > +Test OK
> > > diff --git a/tests/xfs/group b/tests/xfs/group
> > > index 792161a..2bde916 100644
> > > --- a/tests/xfs/group
> > > +++ b/tests/xfs/group
> > > @@ -416,3 +416,4 @@
> > >  416 dangerous_fuzzers dangerous_scrub dangerous_repair
> > >  417 dangerous_fuzzers dangerous_scrub dangerous_online_repair
> > >  418 dangerous_fuzzers dangerous_scrub dangerous_repair
> > > +999 dangerous
> > 
> > I think the auto group makes sense too.
> > 
> > Also, since the discussion around having a couple tests here (one using
> > error injection and this one using dm-thin) it seems like this test
> > could be more suited to a generic test.  Nothing in the test really
> > depends on a particular filesystem. The header comment could simply be
> > updated to explain the specific XFS issue as the inspiration and that
> > the test generically exercises the ability to recover/continue after a
> > dm-thin ENOPSPC and subsequent volume extend. Thoughts?
> 
> I agree. :)
> 
> (new laptop, let's see if this mail even makes it to the ml...)
> 
> --D
> 
> > Brian
> > 
> > > -- 
> > > 2.9.4
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Foster July 7, 2017, 10:21 a.m. UTC | #4 
On Fri, Jul 07, 2017 at 11:24:31AM +0200, Carlos Maiolino wrote:
> Hi
> 
> Thanks for the comments, I agree with all the changes pointed but one inlined
> below.
> 
> > > > +xfs_io -f -d -c 'pwrite -b 1m 0 120m' $mnt/f1 >/dev/null 2>&1
> > > > +
> > > > +fsfreeze -f $mnt &
> > > > +
> > > > +#Wait fsfreeze to its job
> > > > +sleep 10
> > > 
> > > I think you could use 'wait' here rather than a sleep. But is there a
> > > reason we need to put the freeze in the background in the first place?
> > > It seems to me this test will either complete or hang regardless. ;P
> > 
> > I had the same thought.
> 
> not running fsfreeze in the background won't complete the test. fsfreeze needs
> some space in the device to complete, and with a full device it will hang until
> the device is extended with lvextend.
> 
> The difference between a fixed filesystem and a buggy one, is whether the
> filesystem can be properly thawed or not, but both cases will hang in fsfreeze
> if the device isn't expanded right after freezing the fs.
> 
> The main reason of the freeze, is to force xfsaild to run and push ail items, if
> no space is availale, xfsaild will stay stuck:
> 

Ok, I see.. that makes sense. Could you update the comments a bit here
just to explain why the background task is necessary and that the freeze
is going to block on the lvextend due to the state of the fs? I also
think a wait after the lvextend couldn't hurt so it's more clear that
the freeze is what is blocked, but not a big deal either way.

Brian

> ~ $ sudo cat /proc/4712/stack 
> [<ffffffff813f7e16>] xfs_ail_push_all_sync+0xb6/0x100
> [<ffffffff813e714f>] xfs_log_quiesce+0x4f/0x3a0
> [<ffffffff813dd5cd>] xfs_quiesce_attr+0x6d/0xb0
> [<ffffffff813dd660>] xfs_fs_freeze+0x50/0x80
> [<ffffffff81246a5f>] freeze_super+0xcf/0x1a0
> [<ffffffff81258ca3>] do_vfs_ioctl+0x4b3/0x5e0
> [<ffffffff81258e49>] SyS_ioctl+0x79/0x90
> [<ffffffff818da677>] entry_SYSCALL_64_fastpath+0x1a/0xa9
> [<ffffffffffffffff>] 0xffffffffffffffff
> 
> 
> Other than that, I'll agree with everything.
> 
> Thanks for the review guys
> 
> 
> > 
> > > > +
> > > 
> > > "extend the volume with sufficient space and unfreeze ..."
> > > 
> > > > +lvextend -L $newpsize $vgname/$poolname >/dev/null 2>&1
> > > > +
> > > > +fsfreeze -u $mnt
> > > > +echo "Test OK"
> > > > +
> > > > +status=0
> > > > +exit
> > > > diff --git a/tests/xfs/999.out b/tests/xfs/999.out
> > > > new file mode 100644
> > > > index 0000000..8c3c938
> > > > --- /dev/null
> > > > +++ b/tests/xfs/999.out
> > > > @@ -0,0 +1,2 @@
> > > > +QA output created by 999
> > > > +Test OK
> > > > diff --git a/tests/xfs/group b/tests/xfs/group
> > > > index 792161a..2bde916 100644
> > > > --- a/tests/xfs/group
> > > > +++ b/tests/xfs/group
> > > > @@ -416,3 +416,4 @@
> > > >  416 dangerous_fuzzers dangerous_scrub dangerous_repair
> > > >  417 dangerous_fuzzers dangerous_scrub dangerous_online_repair
> > > >  418 dangerous_fuzzers dangerous_scrub dangerous_repair
> > > > +999 dangerous
> > > 
> > > I think the auto group makes sense too.
> > > 
> > > Also, since the discussion around having a couple tests here (one using
> > > error injection and this one using dm-thin) it seems like this test
> > > could be more suited to a generic test.  Nothing in the test really
> > > depends on a particular filesystem. The header comment could simply be
> > > updated to explain the specific XFS issue as the inspiration and that
> > > the test generically exercises the ability to recover/continue after a
> > > dm-thin ENOPSPC and subsequent volume extend. Thoughts?
> > 
> > I agree. :)
> > 
> > (new laptop, let's see if this mail even makes it to the ml...)
> > 
> > --D
> > 
> > > Brian
> > > 
> > > > -- 
> > > > 2.9.4
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Carlos
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Carlos Maiolino July 7, 2017, 10:26 a.m. UTC | #5 
On Fri, Jul 07, 2017 at 06:21:17AM -0400, Brian Foster wrote:
> On Fri, Jul 07, 2017 at 11:24:31AM +0200, Carlos Maiolino wrote:
> > Hi
> > 
> > Thanks for the comments, I agree with all the changes pointed but one inlined
> > below.
> > 
> > > > > +xfs_io -f -d -c 'pwrite -b 1m 0 120m' $mnt/f1 >/dev/null 2>&1
> > > > > +
> > > > > +fsfreeze -f $mnt &
> > > > > +
> > > > > +#Wait fsfreeze to its job
> > > > > +sleep 10
> > > > 
> > > > I think you could use 'wait' here rather than a sleep. But is there a
> > > > reason we need to put the freeze in the background in the first place?
> > > > It seems to me this test will either complete or hang regardless. ;P
> > > 
> > > I had the same thought.
> > 
> > not running fsfreeze in the background won't complete the test. fsfreeze needs
> > some space in the device to complete, and with a full device it will hang until
> > the device is extended with lvextend.
> > 
> > The difference between a fixed filesystem and a buggy one, is whether the
> > filesystem can be properly thawed or not, but both cases will hang in fsfreeze
> > if the device isn't expanded right after freezing the fs.
> > 
> > The main reason of the freeze, is to force xfsaild to run and push ail items, if
> > no space is availale, xfsaild will stay stuck:
> > 
> 
> Ok, I see.. that makes sense. Could you update the comments a bit here
> just to explain why the background task is necessary and that the freeze
> is going to block on the lvextend due to the state of the fs? I also
> think a wait after the lvextend couldn't hurt so it's more clear that
> the freeze is what is blocked, but not a big deal either way.
> 

Sure, no problem at all, I'll work on this, thanks Brian


> Brian
> 
> > ~ $ sudo cat /proc/4712/stack 
> > [<ffffffff813f7e16>] xfs_ail_push_all_sync+0xb6/0x100
> > [<ffffffff813e714f>] xfs_log_quiesce+0x4f/0x3a0
> > [<ffffffff813dd5cd>] xfs_quiesce_attr+0x6d/0xb0
> > [<ffffffff813dd660>] xfs_fs_freeze+0x50/0x80
> > [<ffffffff81246a5f>] freeze_super+0xcf/0x1a0
> > [<ffffffff81258ca3>] do_vfs_ioctl+0x4b3/0x5e0
> > [<ffffffff81258e49>] SyS_ioctl+0x79/0x90
> > [<ffffffff818da677>] entry_SYSCALL_64_fastpath+0x1a/0xa9
> > [<ffffffffffffffff>] 0xffffffffffffffff
> > 
> > 
> > Other than that, I'll agree with everything.
> > 
> > Thanks for the review guys
> > 
> > 
> > > 
> > > > > +
> > > > 
> > > > "extend the volume with sufficient space and unfreeze ..."
> > > > 
> > > > > +lvextend -L $newpsize $vgname/$poolname >/dev/null 2>&1
> > > > > +
> > > > > +fsfreeze -u $mnt
> > > > > +echo "Test OK"
> > > > > +
> > > > > +status=0
> > > > > +exit
> > > > > diff --git a/tests/xfs/999.out b/tests/xfs/999.out
> > > > > new file mode 100644
> > > > > index 0000000..8c3c938
> > > > > --- /dev/null
> > > > > +++ b/tests/xfs/999.out
> > > > > @@ -0,0 +1,2 @@
> > > > > +QA output created by 999
> > > > > +Test OK
> > > > > diff --git a/tests/xfs/group b/tests/xfs/group
> > > > > index 792161a..2bde916 100644
> > > > > --- a/tests/xfs/group
> > > > > +++ b/tests/xfs/group
> > > > > @@ -416,3 +416,4 @@
> > > > >  416 dangerous_fuzzers dangerous_scrub dangerous_repair
> > > > >  417 dangerous_fuzzers dangerous_scrub dangerous_online_repair
> > > > >  418 dangerous_fuzzers dangerous_scrub dangerous_repair
> > > > > +999 dangerous
> > > > 
> > > > I think the auto group makes sense too.
> > > > 
> > > > Also, since the discussion around having a couple tests here (one using
> > > > error injection and this one using dm-thin) it seems like this test
> > > > could be more suited to a generic test.  Nothing in the test really
> > > > depends on a particular filesystem. The header comment could simply be
> > > > updated to explain the specific XFS issue as the inspiration and that
> > > > the test generically exercises the ability to recover/continue after a
> > > > dm-thin ENOPSPC and subsequent volume extend. Thoughts?
> > > 
> > > I agree. :)
> > > 
> > > (new laptop, let's see if this mail even makes it to the ml...)
> > > 
> > > --D
> > > 
> > > > Brian
> > > > 
> > > > > -- 
> > > > > 2.9.4
> > > > > 
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > -- 
> > Carlos
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/tests/xfs/999 b/tests/xfs/999
new file mode 100755
index 0000000..5ae8b74
--- /dev/null
+++ b/tests/xfs/999
@@ -0,0 +1,102 @@ 
+#! /bin/bash
+# FS QA Test 999
+#
+# Test buffer resubmission after a failed writeback with to a full overcommited
+# dm-thin device. 
+#
+# This test will hang the filesystem when ran on an unpatched kernel
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2017 YOUR NAME HERE.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+	$UMOUNT_PROG $mnt >/dev/null 2>&1
+	$LVM_PROG vgremove -ff $vgname >>$seqres.full 2>&1
+	$LVM_PROG pvremove -ff $SCRATCH_DEV >>$seqres.full 2>&1
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs
+_supported_os Linux
+_require_scratch_nocheck
+_require_dm_target thin-pool
+_require_command $LVM_PROG lvm
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+vgname=vg_$seq
+lvname=lv_$seq
+poolname=pool_$seq
+snapname=snap_$seq
+origpsize=100
+virtsize=200
+newpsize=200
+mnt=$SCRATCH_DIR/mnt_$seq
+mkdir -p $mnt
+
+#Ensure we have enough disk space
+_scratch_mkfs_sized $((250 * 1024 * 1024)) >>$seqres.full 2>&1
+
+$LVM_PROG pvcreate -f $SCRATCH_DEV >/dev/null 2>&1
+$LVM_PROG vgcreate -f $vgname $SCRATCH_DEV >/dev/null 2>&1
+
+$LVM_PROG lvcreate  --thinpool $poolname  --errorwhenfull y --zero n \
+-L $origpsize --poolmetadatasize 4M $vgname >/dev/null 2>&1
+
+$LVM_PROG lvcreate  --virtualsize $virtsize -T $vgname/$poolname \
+-n $lvname >/dev/null 2>&1
+
+_mkfs_dev /dev/mapper/$vgname-$lvname >/dev/null 2>&1
+
+$LVM_PROG lvcreate  -k n -s $vgname/$lvname -n $snapname >/dev/null 2>&1
+
+_mount /dev/mapper/$vgname-$snapname $mnt
+
+xfs_io -f -d -c 'pwrite -b 1m 0 120m' $mnt/f1 >/dev/null 2>&1
+
+fsfreeze -f $mnt &
+
+#Wait fsfreeze to its job
+sleep 10
+
+lvextend -L $newpsize $vgname/$poolname >/dev/null 2>&1
+
+fsfreeze -u $mnt
+echo "Test OK"
+
+status=0
+exit
diff --git a/tests/xfs/999.out b/tests/xfs/999.out
new file mode 100644
index 0000000..8c3c938
--- /dev/null
+++ b/tests/xfs/999.out
@@ -0,0 +1,2 @@ 
+QA output created by 999
+Test OK
diff --git a/tests/xfs/group b/tests/xfs/group
index 792161a..2bde916 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -416,3 +416,4 @@ 
 416 dangerous_fuzzers dangerous_scrub dangerous_repair
 417 dangerous_fuzzers dangerous_scrub dangerous_online_repair
 418 dangerous_fuzzers dangerous_scrub dangerous_repair
+999 dangerous