xfs
[Top] [All Lists]

Re: [PATCH 2/2 v7] XFS TESTS: Add ENOSPC Hole Punch Test

To: Allison Henderson <achender@xxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH 2/2 v7] XFS TESTS: Add ENOSPC Hole Punch Test
From: Alex Elder <aelder@xxxxxxx>
Date: Thu, 14 Jul 2011 13:22:05 -0500
Cc: <xfs@xxxxxxxxxxx>, <linux-fsdevel@xxxxxxxxxxxxxxx>, <linux-ext4@xxxxxxxxxxxxxxx>
In-reply-to: <1309272301-5742-3-git-send-email-achender@xxxxxxxxxxxxxxxxxx>
References: <1309272301-5742-1-git-send-email-achender@xxxxxxxxxxxxxxxxxx> <1309272301-5742-3-git-send-email-achender@xxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Tue, 2011-06-28 at 07:45 -0700, Allison Henderson wrote:
> This patch adds a new test 255 that tests that a hole can be punched even 
> when the
> disk is full.  Reserved blocks should be used to allow a punch hole to 
> proceed even
> when there is not enough blocks to further fragment the file. To test this, 
> the
> file system is fragmented by punching holes in regular intervals and filling
> the file system between punches. This will eventually force the file system 
> to use
> reserved blocks to proceed with the punch hole operation.
> 
> The work in this patch is a continuation from a previous patch set that has 
> been
> partially accepted.
> 
> Signed-off-by: Allison Henderson <achender@xxxxxxxxxxxxxxxxxx>

First of all, I renumbered the test 256 (and will do
that for you if necessary before committing the change).

Second, I can confirm that this new test passes for
XFS on my setup.  I don't know what the threshold for
inclusion in the "quick" group is, but it's under a
minute so that's fine with me.

Third, maybe we can create a new group "punch" or
something like that to tag tests that exercise
hole punching functionality (tests 252, 255, and
now this one at least; maybe 175 and 176 too).

Finally, I found a bug and a few other spots that
really ought to be fixed but aren't really serious
problems.  I also have a number of other things that
I commented on, but tried to make it clear where
they're just remarks to consider, not requests to
make a change.

                                        -Alex

> ---
> v5->v6
> 
> Test moved out of 252 and put in its own test 255
> 
> _fill_fs and _test_full_fs_punch have been moved from common.punch
> to test 255 and modified to use the _user_do routines in common.rc
> 
> _fill_fs has been optimized to stop once files smaller than a block
> cannot be created.
> 
> v6->v7
> Fixed bad file add
> 
> 
> :000000 100644 0000000... 3d39fdb... A        255
> :000000 100644 0000000... 3525403... A        255.out
> :100644 100644 1f86075... c045e70... M        group
>  255     |  178 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  255.out |    1 +
>  group   |    1 +
>  3 files changed, 180 insertions(+), 0 deletions(-)
> 
> diff --git a/255 b/255
> new file mode 100644
> index 0000000..3d39fdb
> --- /dev/null
> +++ b/255
> @@ -0,0 +1,178 @@
> +#! /bin/bash
> +# FS QA Test No. 255
> +#
> +# Test Full File System Hole Punching
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2011 IBM Corporation.  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
> +#
> +#-----------------------------------------------------------------------
> +#
> +# creator
> +owner=achender@xxxxxxxxxxxxxxxxxx
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1     # failure is the default!
> +
> +_cleanup()
> +{
> +    rm -f $tmp.*
> +}
> +
> +trap "_cleanup ; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.punch
> +
> +# real QA test starts here
> +_supported_fs generic
> +_supported_os Linux
> +
> +_require_xfs_io_falloc_punch
> +_require_scratch
> +_require_user
> +
> +testfile=$TEST_DIR/255.$$
   testfile=$TEST_DIR/$seq.$$

> +
> +

The _fill_fs function seems like it may be useful elsewhere,
so it may end up in common.rc eventually.  (Easy enough to
move it there later though.)

> +# _fill_fs()
> +#
> +# Fills a file system by repeatedly creating files in the given folder
> +# starting with the given file size.  Files are reduced in size when
> +# they can no longer fit untill no more files can be created.
> +#
> +# This routine is used by _test_full_fs_punch to test that a hole may
> +# still be punched when the disk is full by borrowing reserved blocks.
> +# All files are created as a non root user to prevent reserved blocks
> +# from being consumed.
> +#
> +_fill_fs() {
> +     local file_size=$1
> +     local dir=$2
> +     local block_size=$3
> +     local file_count=1
> +     local bytes_written=0
> +
> +     if [ $# -ne 3 ]
> +     then
> +             echo "USAGE: _fill_fs filesize dir block size"
> +             exit 1
> +     fi
> +
> +     # Creation of files or folders
> +     # must not be done as root or
> +     # reserved blocks will be consumed
> +     _user_do "mkdir -p $dir &> /dev/null"

I personally prefer seeing "> /dev/null 2>&1" rather
than "&> /dev/null", to make it that much more
explicit that both are being redirected.  This is
purely a style thing, and I say this mainly to see
if anyone else has a preference one way or another.
(This appears several times in the file, so if you
do choose to change it, do so throughout.)

> +     if [ $? -ne 0 ] ; then
> +             return 0
> +     fi
> +
> +     if [ $file_size -lt $block_size ]
> +     then
> +             $file_size = $block_size
                  file_size=$block_size

(This one is a bug.)

> +     fi
> +
> +     while [ $file_size -ge $block_size ]
> +     do
> +             bytes_written=0
> +             _user_do "$XFS_IO_PROG -F -f -c \"pwrite 0 $file_size\" 
> $dir/$file_count.bin &> /dev/null"
> +

Try to split these long lines, e.g.:
                _user_do "$XFS_IO_PROG -F -f -c \
                                \"pwrite 0 $file_size\" \
                                $dir/$file_count.bin &> /dev/null"

> +             if [ -f $dir/$file_count.bin ]
> +             then
> +                     bytes_written=`$XFS_IO_PROG -F -c "stat"  
> $dir/$file_count.bin | grep size | cut -d ' ' -f3`

Personal preference again, but as long as we're using bash
and not sh, Use $(...) rather than `...` for sub-commands.

> +             fi
> +
> +             # If there was no room to make the file,
> +             # then divide it in half, and keep going
> +             if [ $bytes_written -lt $file_size ]
> +             then
> +                     file_size=$(( $file_size / 2 ))
> +             fi      
> +             file_count=$(( $file_count + 1 ))
> +
> +     done
> +}
> +
> +# _test_full_fs_punch()
> +#
> +# This function will test that a hole may be punched
> +# even when the file system is full.  Reserved blocks
> +# should be used to allow a punch hole to proceed even
> +# when there is not enough blocks to further fragment the
> +# file. To test this, this function will fragment the file
> +# system by punching holes in regular intervals and filling
> +# the file system between punches. 
> +#
> +_test_full_fs_punch()
> +{
> +     local hole_len=$1      # The length of the holes to punch
> +     local hole_interval=$2 # The interval between the holes
> +     local iterations=$3    # The number of holes to punch
> +     local file_name=$4     # File to punch holes in
> +     local block_size=$5    # File system block size
> +     local file_len=$(( $(( $hole_len + $hole_interval )) * $iterations ))
> +     local path=`dirname $file_name`
> +     local hole_offset=0
> +
> +     if [ $# -ne 5 ] 
> +     then
> +             echo "USAGE: _test_full_fs_punch hole_len hole_interval 
> iterations file_name block_size"
> +             exit 1
> +     fi
> +
> +     rm -f $file_name &> /dev/null
> +
> +     $XFS_IO_PROG -F -f -c "pwrite 0 $file_len" \
> +             -c "fsync" $file_name &> /dev/null
> +     chmod 666 $file_name
> +
> +     _fill_fs $(( 1024 * 1024 * 1024 )) $path/fill $block_size

You are specifying a file size that's equal to the filesystem
size, which I think is guaranteed to fail its first pass.  Maybe
start with half of the filesystem (or even 90% of it).  (I
don't know whether this is a a real issue or not though.)

Also, you could maybe use a shell variable to represent the
filesystem size, since you use it twice in this script.

> +
> +     for (( i=0; i<$iterations; i++ ))
> +     do
> +             # This part must not be done as root in order to
> +             # test that reserved blocks are used when needed
> +             _user_do "$XFS_IO_PROG -F -f -c \"fpunch $hole_offset 
> $hole_len\" $file_name"
> +             rc=$?
> +             if [ $? -ne 0 ] ; then
> +                     echo Punch hole failed
> +                     break
> +             fi
> +
> +             hole_offset=$(( $hole_offset + $hole_len + $hole_interval ))
> +
> +             _fill_fs $hole_len $path/fill.$i $block_size
> +
> +     done
> +}
> +
> +# Make a small file system to fillkk
> +umount $SCRATCH_DEV &> /dev/null
> +_scratch_mkfs_sized $(( 1024 * 1024 * 1024 )) &> /dev/null
> +_scratch_mount
> +# Test must be able to write files with non-root permissions
> +chmod 777 $SCRATCH_MNT
> +
> +block_size=`stat -f $SCRATCH_DEV | grep "Block size" | cut -d " " -f3`
> +_test_full_fs_punch $(( $block_size * 2 )) $block_size 500 
> $SCRATCH_MNT/252.$$ $block_size
                                           Use     $SCRATCH_MNT/$seq.$$

...and possibly define this path in a variable
at the top of the file.

> +
> +status=0 ; exit
> diff --git a/255.out b/255.out
> new file mode 100644
> index 0000000..3525403
> --- /dev/null
> +++ b/255.out
> @@ -0,0 +1 @@
> +QA output created by 255
> diff --git a/group b/group
> index 1f86075..c045e70 100644
> --- a/group
> +++ b/group
> @@ -368,3 +368,4 @@ deprecated
>  252 auto quick prealloc
>  253 auto quick
>  254 auto quick
> +255 auto quick



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