[Top] [All Lists]

Re: [PATCH] xfstests: xfs directory unbalance assert test

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfstests: xfs directory unbalance assert test
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Tue, 17 Sep 2013 10:58:19 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <52387831.5010205@xxxxxxxxxxx>
References: <20130917145946.124195107@xxxxxxx> <20130917145959.333796933@xxxxxxx> <52387831.5010205@xxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 09/17/13 10:41, Eric Sandeen wrote:
On 9/17/13 9:59 AM, Mark Tinguely wrote:
This tests triggers an assert in the XFS directory unbalance code.
This test originally written by Brian Foster and suggestions
from Micheal Semon.

cool, thanks.  Comments below.

Signed-off-by: Mark Tinguely<tinguely@xxxxxxx>
  tests/generic/319     |   62 
  tests/generic/319.out |    2 +
  tests/generic/group   |    1
  3 files changed, 65 insertions(+)

Index: b/tests/generic/319
--- /dev/null
+++ b/tests/generic/319
@@ -0,0 +1,62 @@
+#! /bin/bash
+# FS QA Test No. 319
+# Test directory code correctly handles fsstress filling the filesystem
+# Copyright (c) 2013 SGI.  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
+# 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`
+echo "QA output created by $seq"
+status=1       # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+    cd /

That seems pointless; usually it's done w/ rm -f $tmp.*
right after, but we have no tmpfile, so...

Yeah, no cleanup is needed.

+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+# real QA test starts here
+_supported_fs generic
+_supported_os IRIX Linux
+_scratch_unmount>  /dev/null 2>&1


I see this done both ways - is it required to unmount scratch at the beginning
of a test?  I don't think so (I know it's done in many tests, though, but
again, C&P&  cargo cult?  Or not?  I'm not sure :( )

I guess it doesn't hurt, but at some point I'd like to get it straight
about who's required to umount scratch, and when (if at all).

Have to unmount for the mkfs, as noted by Eryu, it is already done. I would rather manually unmount it than be surprised when someone changes the common files.

+_scratch_mkfs_sized 11g>>  $seqres.full 2>&1

_scratch_mkfs_sized doesn't take units like this ('g'), so the above fails to
actually make an 11g fs:

# Create fs of certain size on scratch device
# _scratch_mkfs_sized<size in bytes>  [optional blocksize]

so we get this in 319.full:

expr: non-numeric argument
./common/rc: line 576: [: 11g: integer expression expected

but then it seems like mkfs carries on anyway w/ defaults.  :(

Apparently the mkfs 11g part isn't actually critical? ;)

it works on xfs because mkfs.xfs size can take those values, but yes it breaks on other filesystem. my bad.

maybe _scratch_mkfs_sized needs something like this at the top:

if ! [[ $fssize =~ $re ]] ; then
    _notrun "error: _scratch_mkfs_sized: $fssize not a number of bytes"

+_scratch_mount>  /dev/null 2>&1

is ignore-all-output really the right thing to do?  When does _scratch_mount
emit anything?  (more cargo cult)? :)

+# Fill the filesystem.
+FSSTRESS_ARGS="-z -s 1378390208 -fsymlink=1 -n7500000 -p4 -d $SCRATCH_MNT"
+$FSSTRESS_PROG $FSSTRESS_ARGS>>  $seqres.full 2>&1
+cd $SCRATCH_MNT>>  $seqres.full 2>&1

cd doesn't emit anything except on error, right, and if there's an error we'd 
stop the test right here!

+# A debug XFS may assert in the remove due to a directory bug.
+rm -rf *

I'd feel a whole lot better if you did rm -rf $SCRATCH_MNT/* just in case
we somehow ended up in the wrong place here.

Or even better if you pointed fsstress at $SCRATCH_MNT/$seq.dir, and
then did rm -rf $SCRATCH_MNT/$seq.dir - just to avoid that nasty
should-never-happen-still-scary "rm -rf /*"

Makes a lot of sense.

Thanks Eric.

<Prev in Thread] Current Thread [Next in Thread>