From: Alex Elder Subject: Re: [PATCH 2/2 v7] XFS TESTS: Add ENOSPC Hole Punch Test Date: Thu, 14 Jul 2011 13:22:05 -0500 Message-ID: <1310667725.2067.43.camel@doink> References: <1309272301-5742-1-git-send-email-achender@linux.vnet.ibm.com> <1309272301-5742-3-git-send-email-achender@linux.vnet.ibm.com> Reply-To: Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: , , To: Allison Henderson Return-path: Received: from relay2.sgi.com ([192.48.179.30]:48779 "EHLO relay.sgi.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755400Ab1GNSWJ (ORCPT ); Thu, 14 Jul 2011 14:22:09 -0400 In-Reply-To: <1309272301-5742-3-git-send-email-achender@linux.vnet.ibm.com> Sender: linux-ext4-owner@vger.kernel.org List-ID: 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 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@linux.vnet.ibm.com > + > +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