max_dir_size_kb option list

From 21b1da618d0fcb4cd4666d10c41583274ed4eeed Mon Sep 17 00:00:00 2001

Hello,

I've prepared test for xfstests suite that runs some checks for ext4 mount option max_dir_size_kb introduced in Linux Kernel 3.7, could someone please look on it?

From 21b1da618d0fcb4cd4666d10c41583274ed4eeed Mon Sep 17 00:00:00 2001
From: Alexander Tsvetkov <[email protected]>
Date: Wed, 10 Dec 2014 15:31:02 +0300
Subject: [PATCH] added test for max_dir_size_kb mount option

---
 tests/ext4/309     | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/ext4/309.out |   2 +
 tests/ext4/group   |   3 +-
 3 files changed, 217 insertions(+), 1 deletion(-)
 create mode 100755 tests/ext4/309
 create mode 100755 tests/ext4/309.out

tests/ext4/309 new file +21 -0

new file mode 100755
index 0000000..e2f4e43
@@ -0,0 +1,213 @@
+#! /bin/bash
+# FS QA Test
+#
+# Test for mount option max_dir_size_kb
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2014 Oracle and/or its affiliates.  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
+#-----------------------------------------------------------------------

tests/ext4/309 new file +10 -0

+echo "QA output created by $seq"
+echo "Silence is golden"
+rm -f $seqres.full
+
+status=1 # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup() {
+   if [ ! -z SCRATCH_MNT ]; then
+      rm -fr $SCRATCH_MNT/test*

tests/ext4/309 new file +25 -0

+   for i in $dirs; do
+      rm -fr $i/*
+   done
+}
+
+# $1 - device
+# $2 - options
+_make_ext4fs() {
+   device=$1
+   opts=$2
+   umount $device 1>/dev/null 2>&1
+   mkfs.ext4 $opts $device 1>/dev/null 2>&1
+}
+
+# $1 - options
+# $2 - mount point
+_make_loopfs() {
+   lpf=$testfile
+   dd if=/dev/zero of=$lpf bs=4k count=256 1>/dev/null 2>&1
+   loopdev=$(losetup -f)
+   losetup $loopdev $lpf
+   mkfs.ext4 -O ^dir_index,^has_journal $loopdev 1>/dev/null 2>&1
+   mount -t ext4 $1 $loopdev $2
+}
+

tests/ext4/309 new file +27 -0

+       if [ -e $dir/$item ]; then
+          continue
+       fi 
+       create_cmd="$2 $dir/$item 2>$tmp.out 1>/dev/null"
+       eval "$create_cmd"
+       res=$?
+       if [ $res -ne 0 ]; then
+          _filter_error "No space left on device" < $tmp.out > $tmp.out2
+          if [ -s $tmp.out2 ]; then
+             cat $tmp.out2 | tr -d '\n' >> $seqres.full
+          else
+             echo "FAIL! expected ENOSPC" | tee -a $seqres.full
+          fi
+          break
+      fi
+   done
+   size=$(stat -c %s $dir)
+   size=$((size/1024))
+   if [ $size -gt $limit ]; then
+      echo "FAIL! expected dir size: $limit, actually: $size" | tee -a $seqres.full
+   fi
+   rm -f $tmp*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+

tests/ext4/309 new file +25 -0

+_scratch_mount "-o max_dir_size_kb=$LIMIT1"
+mkdir $testdir 2>/dev/null
+
+echo -e "\nExceed $LIMIT1 Kb limit with new files in testdir/: " >> $seqres.full
+_create_items $LIMIT1 "touch"
+
+echo -e "\nRemount with $LIMIT1 Kb limit,\nnew item in testdir/ should result to ENOSPC: " >>$seqres.full
+_scratch_mount "-o remount,max_dir_size_kb=$LIMIT1"
+_create_items $LIMIT1 "touch"
+
+echo -e "\nExceed $LIMIT2 Kb limit with new files in testdir/: " >> $seqres.full
+_scratch_mount "-o remount,max_dir_size_kb=$LIMIT2"
+_create_items $LIMIT2 "touch"
+
+echo -e "\nExceed $LIMIT2 Kb limit with new files in testdir2/: " >> $seqres.full
+mkdir $SCRATCH_MNT/testdir2 2>/dev/null
+_create_items $LIMIT2 "touch" "$SCRATCH_MNT/testdir2"
+
+echo -e "\nRemount with $LIMIT1 Kb limit,\nnew item in testdir/ should result to ENOSPC: " >> $seqres.full
+_scratch_mount "-o remount,max_dir_size_kb=$LIMIT1"
+_create_items $LIMIT2 "touch"
+echo -e "\nnew item in testdir2/ should result to ENOSPC: " >> $seqres.full
+_create_items $LIMIT2 "touch" "$SCRATCH_MNT/testdir2"
+_clear_testdir "$SCRATCH_MNT/testdir2"
+rmdir $testdir

tests/ext4/309 new file +10 -0

+echo -e "\nExceed $LIMIT1 Kb directory limit with hardlinks: " >> $seqres.full
+_create_items $LIMIT1 "ln $testfile"
+_clear_testdir
+
+echo -e "\nExceed $LIMIT1 Kb directory limit with FIFOs: " >> $seqres.full
+_create_items $LIMIT1 "mkfifo"
+_clear_testdir
+
+echo -e "\nCreate ext4 fs on testdir/subdir with $LIMIT2 Kb limit," >> $seqres.full
+mkdir $testdir/subdir 2>/dev/null

tests/ext4/309 new file +30 -0

+umount $TEST_DEV 1>/dev/null 2>&1
+_scratch_mount "-o remount,max_dir_size_kb=$LIMIT2"
+mount -t ext4 -o max_dir_size_kb=$LIMIT1 $TEST_DEV $testdir/subdir
+
+echo "exceed new $LIMIT2 Kb limit of testdir/ with a set of files:" >> $seqres.full
+_create_items $LIMIT2 "touch"
+echo -e "\nnew item in testdir/subdir should result to ENOSPC: " >> $seqres.full
+_create_items $LIMIT2 "touch" "$testdir/subdir"
+
+umount $TEST_DEV 1>/dev/null 2>&1
+_clear_testdir
+
+echo -e "\ntestdir/ limit $LIMIT2 Kb, loop fs: testdir/subdir limit $LIMIT1 Kb," >> $seqres.full
+mkdir $testdir/subdir
+
+_make_loopfs "-o max_dir_size_kb=$LIMIT1" "$testdir/subdir"
+
+echo "exceed $LIMIT1 Kb limit of testdir/subdir with a set of files:" >> $seqres.full
+_create_items $LIMIT1 "touch" "$testdir/subdir"
+
+echo -e "\nexceed $LIMIT2 Kb limit of testdir/ with a set of files:" >> $seqres.full
+_create_items $LIMIT2 "touch"
+
+umount $testdir/subdir
+losetup -d $loopdev
+_clear_testdir
+
+# success, all done
+status=0
+exit 0

tests/ext4/309.out new file +2 -0

new file mode 100755
index 0000000..56330d6
@@ -0,0 +1,2 @@
+QA output created by 309
+Silence is golden

tests/ext4/group +2 -1

index aa6a53b..9bf1061 100644
@@ -14,4 +14,5 @@
 305 auto
 306 auto rw resize quick
 307 auto ioctl rw
-308 auto ioctl rw prealloc quick
\ No newline at end of file
+308 auto ioctl rw prealloc quick
+309 auto

Signature · 1 line

1.9.3

Quoted 3 lines · Alexander Tsvetkov

Hello,

I've prepared test for xfstests suite that runs some checks for ext4 mount option max_dir_size_kb introduced in Linux Kernel 3.7, could someone please look on it?

For future reference, inline patches as plain text are best.

Quoted 40 lines

From 21b1da618d0fcb4cd4666d10c41583274ed4eeed Mon Sep 17 00:00:00 2001
From: Alexander Tsvetkov <[email protected]>
Date: Wed, 10 Dec 2014 15:31:02 +0300
Subject: [PATCH] added test for max_dir_size_kb mount option

---
  tests/ext4/309     | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++
  tests/ext4/309.out |   2 +
  tests/ext4/group   |   3 +-
  3 files changed, 217 insertions(+), 1 deletion(-)

create mode 100755 tests/ext4/309
create mode 100755 tests/ext4/309.out

tests/ext4/309 new file +21 -0

new file mode 100755
index 0000000..e2f4e43
@@ -0,0 +1,213 @@
+#! /bin/bash
+# FS QA Test
+#
+# Test for mount option max_dir_size_kb
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2014 Oracle and/or its affiliates.  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
+#-----------------------------------------------------------------------

tests/ext4/309 line 31

Quoted 10 lines

tests/ext4/309 new file +10 -0

+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+tmp=/tmp/$$
+
+testdir=$SCRATCH_MNT/testdir
+testfile=$SCRATCH_MNT/testfile
+sdir=`dirname $0`
+sdir=`cd "$sdir"; pwd`

Since this script is /bin/bash, I'd recommend to use $(...) instead of ... for subshells, since the former can be nested, is much easier to see the start/end, and isn't confused for '...' quotes.

Quoted 10 lines

tests/ext4/309 new file +10 -0

+echo "QA output created by $seq"
+echo "Silence is golden"
+rm -f $seqres.full
+
+status=1 # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup() {
+   if [ ! -z SCRATCH_MNT ]; then
+      rm -fr $SCRATCH_MNT/test*

tests/ext4/309 line 51

Quoted 10 lines

tests/ext4/309 new file +10 -0

+      _scratch_unmount
+   fi
+}
+
+_filter_error() {
+   sed -n -e "s/.*\($1\).*/\"\1\"/p"
+}
+
+_clear_testdir() {
+   dirs="$testdir *$"

This "*$" looks like a bug. Do you mean "$*"?

Quoted 25 lines

tests/ext4/309 new file +25 -0

+   for i in $dirs; do
+      rm -fr $i/*
+   done
+}
+
+# $1 - device
+# $2 - options
+_make_ext4fs() {
+   device=$1
+   opts=$2
+   umount $device 1>/dev/null 2>&1
+   mkfs.ext4 $opts $device 1>/dev/null 2>&1
+}
+
+# $1 - options
+# $2 - mount point
+_make_loopfs() {
+   lpf=$testfile
+   dd if=/dev/zero of=$lpf bs=4k count=256 1>/dev/null 2>&1
+   loopdev=$(losetup -f)
+   losetup $loopdev $lpf
+   mkfs.ext4 -O ^dir_index,^has_journal $loopdev 1>/dev/null 2>&1
+   mount -t ext4 $1 $loopdev $2
+}
+

tests/ext4/309 line 86

Quoted 10 lines

tests/ext4/309 new file +10 -0

+# $1 - expected limit after items creation
+# $2 - command to create item
+# $3 - where to create (testdir by default)
+_create_items() {
+    limit=$1
+    create_cmd=$2
+    dir=${3:-$testdir}
+    sync
+    echo 3 > /proc/sys/vm/drop_caches
+    MAX_INUM=$(((limit*1024*2)/24))

(style) spaces around operators, and no need for extra () around multiplication.

tests/ext4/309 line 88

Quoted 2 lines

tests/ext4/309 new file +2 -0

+    for i in $(seq 0 $MAX_INUM); do
+       tmp_name=`mktemp -u`

Is there a reason to use mktemp instead of just some sequential names like item="f.$limit.$i" or similar? There are several problems with the complexity here that could be avoided.

- since mktemp is creating temp files in $TMPDIR by default and "-u" is unlinking them, this is doing double the work needed
- the "continue" below implies that some items may not be created, due to name collisions (since mktemp doesn't actually see the entries created in the "real" directory), so fewer than $MAX_INUM entries may be created, causing the test to be inconsistent

This could just be something simple like:

          tmp_name=$(mktemp $dir/f.XXXXXX)

and if we need to get directory creation involved, then something like:

          [ "$2" = "mkdir" ] && MKTEMP_OPT="-d"
          tmp_name=$(mktemp $MKTEMP_OPT $dir/f.XXXXXX 2>$tmp.out)

I see that there are callers of _create_items() that are not "touch" or "mkdir", but I don't think those are adding much value to this test (more on that below).

tests/ext4/309 line 89

Quoted 1 line

tests/ext4/309 new file +1 -0

+       item=`basename $tmp_name`

Use $(...), especially since this style is already used here.

Quoted 32 lines

tests/ext4/309 new file +32 -0

+       if [ -e $dir/$item ]; then
+          continue
+       fi
+       create_cmd="$2 $dir/$item 2>$tmp.out 1>/dev/null"
+       eval "$create_cmd"
+       res=$?
+       if [ $res -ne 0 ]; then
+          _filter_error "No space left on device" < $tmp.out > $tmp.out2
+          if [ -s $tmp.out2 ]; then
+             cat $tmp.out2 | tr -d '\n' >> $seqres.full
+          else
+             echo "FAIL! expected ENOSPC" | tee -a $seqres.full
+          fi
+          break
+      fi
+   done
+   size=$(stat -c %s $dir)
+   size=$((size/1024))
+   if [ $size -gt $limit ]; then
+      echo "FAIL! expected dir size: $limit, actually: $size" | tee -a $seqres.full
+   fi
+   rm -f $tmp*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+
+# real QA test starts here
+
+_supported_fs ext4
+_supported_os Linux
+_require_scratch

tests/ext4/309 line 126

Quoted 5 lines

tests/ext4/309 new file +5 -0

+
+LIMIT1=8
+LIMIT2=16
+
+_make_ext4fs $SCRATCH_DEV "-O ^dir_index,^filetype"

It isn't clear why you disable the filetype option? That has been a standard ext2/3/4 feature for 10+ years, so it doesn't make sense to test without it.

It makes sense to test both with and without the dir_index option (this is enabled by default for ext4), but it doesn't make sense to only test with it disabled.

Better to increase the LIMIT1 and LIMIT2
values to allow testing with this enabled (e.g. 16 and 32) as well as disabled.

Quoted 31 lines

tests/ext4/309 new file +31 -0

+_scratch_mount "-o max_dir_size_kb=$LIMIT1"
+mkdir $testdir 2>/dev/null
+
+echo -e "\nExceed $LIMIT1 Kb limit with new files in testdir/: " >> $seqres.full
+_create_items $LIMIT1 "touch"
+
+echo -e "\nRemount with $LIMIT1 Kb limit,\nnew item in testdir/ should result to ENOSPC: " >>$seqres.full
+_scratch_mount "-o remount,max_dir_size_kb=$LIMIT1"
+_create_items $LIMIT1 "touch"
+
+echo -e "\nExceed $LIMIT2 Kb limit with new files in testdir/: " >> $seqres.full
+_scratch_mount "-o remount,max_dir_size_kb=$LIMIT2"
+_create_items $LIMIT2 "touch"
+
+echo -e "\nExceed $LIMIT2 Kb limit with new files in testdir2/: " >> $seqres.full
+mkdir $SCRATCH_MNT/testdir2 2>/dev/null
+_create_items $LIMIT2 "touch" "$SCRATCH_MNT/testdir2"
+
+echo -e "\nRemount with $LIMIT1 Kb limit,\nnew item in testdir/ should result to ENOSPC: " >> $seqres.full
+_scratch_mount "-o remount,max_dir_size_kb=$LIMIT1"
+_create_items $LIMIT2 "touch"
+echo -e "\nnew item in testdir2/ should result to ENOSPC: " >> $seqres.full
+_create_items $LIMIT2 "touch" "$SCRATCH_MNT/testdir2"
+_clear_testdir "$SCRATCH_MNT/testdir2"
+rmdir $testdir
+mkdir $testdir
+
+echo -e "\nExceed $LIMIT1 Kb directory limit with new subdirectories: " >> $seqres.full
+_create_items $LIMIT1 "mkdir"
+_clear_testdir
+

tests/ext4/309 line 161

Quoted 4 lines

tests/ext4/309 new file +4 -0

+echo -e "\nExceed $LIMIT1 Kb directory limit with symlinks: " >> $seqres.full
+dd if=/dev/urandom of=$testfile bs=1 seek=4096 count=4096 > /dev/null 2>&1
+_create_items $LIMIT1 "ln -s $testfile"
+_clear_testdir

It isn't clear there is any value to test with symlinks, hard links, and FIFOs, since the limit is imposed in the common code for inserting the name into the directory, and it doesn't care what type of inode the name is referencing.

Quoted 14 lines

tests/ext4/309 new file +14 -0

+echo -e "\nExceed $LIMIT1 Kb directory limit with hardlinks: " >> $seqres.full
+_create_items $LIMIT1 "ln $testfile"
+_clear_testdir
+
+echo -e "\nExceed $LIMIT1 Kb directory limit with FIFOs: " >> $seqres.full
+_create_items $LIMIT1 "mkfifo"
+_clear_testdir
+
+echo -e "\nCreate ext4 fs on testdir/subdir with $LIMIT2 Kb limit," >> $seqres.full
+mkdir $testdir/subdir 2>/dev/null
+_make_ext4fs $TEST_DEV "-O ^dir_index,^filetype"
+mount -t ext4 -o max_dir_size_kb=$LIMIT2 $TEST_DEV $testdir/subdir
+
+echo "exceed $LIMIT1 Kb limit of testdir/:" >> $seqres.full

tests/ext4/309 line 181

Quoted 6 lines

tests/ext4/309 new file +6 -0

+_create_items $LIMIT1 "touch"
+
+echo -e "\nexceed $LIMIT2 Kb limit of testdir/subdir:" >> $seqres.full
+_create_items $LIMIT2 "touch" "$testdir/subdir"
+
+echo -e "\ntestdir/ limit $LIMIT2 Kb, testdir/subdir limit $LIMIT1 Kb," >> $seqres.full

I think this message is backward. testdir has a limit of $LIMIT1, and testdir/subdir has a limit of $LIMIT2.

Quoted 47 lines

tests/ext4/309 new file +30 -0

+umount $TEST_DEV 1>/dev/null 2>&1
+_scratch_mount "-o remount,max_dir_size_kb=$LIMIT2"
+mount -t ext4 -o max_dir_size_kb=$LIMIT1 $TEST_DEV $testdir/subdir
+
+echo "exceed new $LIMIT2 Kb limit of testdir/ with a set of files:" >> $seqres.full
+_create_items $LIMIT2 "touch"
+echo -e "\nnew item in testdir/subdir should result to ENOSPC: " >> $seqres.full
+_create_items $LIMIT2 "touch" "$testdir/subdir"
+
+umount $TEST_DEV 1>/dev/null 2>&1
+_clear_testdir
+
+echo -e "\ntestdir/ limit $LIMIT2 Kb, loop fs: testdir/subdir limit $LIMIT1 Kb," >> $seqres.full
+mkdir $testdir/subdir
+
+_make_loopfs "-o max_dir_size_kb=$LIMIT1" "$testdir/subdir"
+
+echo "exceed $LIMIT1 Kb limit of testdir/subdir with a set of files:" >> $seqres.full
+_create_items $LIMIT1 "touch" "$testdir/subdir"
+
+echo -e "\nexceed $LIMIT2 Kb limit of testdir/ with a set of files:" >> $seqres.full
+_create_items $LIMIT2 "touch"
+
+umount $testdir/subdir
+losetup -d $loopdev
+_clear_testdir
+
+# success, all done
+status=0
+exit 0

tests/ext4/309.out new file +2 -0

new file mode 100755
index 0000000..56330d6
@@ -0,0 +1,2 @@
+QA output created by 309
+Silence is golden

tests/ext4/group +0 -1

index aa6a53b..9bf1061 100644
@@ -14,4 +14,5 @@
  305 auto
  306 auto rw resize quick
  307 auto ioctl rw
-308 auto ioctl rw prealloc quick

Quoted 7 lines

\ No newline at end of file
+308 auto ioctl rw prealloc quick
+309 auto

Signature · 1 line

1.9.3

Cheers, Andreas

Hello,

I've prepared test for xfstests suite that runs some checks for ext4 mount option max_dir_size_kb introduced in Linux Kernel 3.7, could someone please look on it?

Thanks,
Alexander Tsvetkov

From 21b1da618d0fcb4cd4666d10c41583274ed4eeed Mon Sep 17 00:00:00 2001
From: Alexander Tsvetkov <[email protected]>
Date: Wed, 10 Dec 2014 15:31:02 +0300
Subject: [PATCH] added test for max_dir_size_kb mount option

---
 tests/ext4/309     | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/ext4/309.out |   2 +
 tests/ext4/group   |   3 +-
 3 files changed, 217 insertions(+), 1 deletion(-)
 create mode 100755 tests/ext4/309
 create mode 100755 tests/ext4/309.out

tests/ext4/309 new file +19 -0

new file mode 100755
index 0000000..e2f4e43
@@ -0,0 +1,213 @@
+#! /bin/bash
+# FS QA Test
+#
+# Test for mount option max_dir_size_kb
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2014 Oracle and/or its affiliates.  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,

tests/ext4/309 new file +7 -0

+   done
+   size=$(stat -c %s $dir)
+   size=$((size/1024))
+   if [ $size -gt $limit ]; then
+      echo "FAIL! expected dir size: $limit, actually: $size" | tee -a $seqres.full
+   fi
+   rm -f $tmp*

Quoted 46 lines · Alexander Tsvetkov

Hello,

I've prepared test for xfstests suite that runs some checks for ext4 mount option max_dir_size_kb introduced in Linux Kernel 3.7, could someone please look on it?

Thanks,
Alexander Tsvetkov
From 21b1da618d0fcb4cd4666d10c41583274ed4eeed Mon Sep 17 00:00:00 2001
From: Alexander Tsvetkov <[email protected]>
Date: Wed, 10 Dec 2014 15:31:02 +0300
Subject: [PATCH] added test for max_dir_size_kb mount option

---
  tests/ext4/309     | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++
  tests/ext4/309.out |   2 +
  tests/ext4/group   |   3 +-
  3 files changed, 217 insertions(+), 1 deletion(-)

create mode 100755 tests/ext4/309
create mode 100755 tests/ext4/309.out

tests/ext4/309 new file +19 -0

new file mode 100755
index 0000000..e2f4e43
@@ -0,0 +1,213 @@
+#! /bin/bash
+# FS QA Test
+#
+# Test for mount option max_dir_size_kb
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2014 Oracle and/or its affiliates.  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,

tests/ext4/309 line 29

Quoted 10 lines

tests/ext4/309 new file +10 -0

+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+tmp=/tmp/$$
+
+testdir=$SCRATCH_MNT/testdir
+testfile=$SCRATCH_MNT/testfile

Do not call a file on the scratch device "testdir". That name has specific meaning: it's the mount point for the test device. Using the same variable name that differs only different syntax for somethingon the scratch device is not a good idea.

tests/ext4/309 line 31

Quoted 2 lines

tests/ext4/309 new file +2 -0

+sdir=`dirname $0`
+sdir=`cd "$sdir"; pwd`

$here is already set to the current xfstests run location.

tests/ext4/309 line 44

Quoted 13 lines

tests/ext4/309 new file +13 -0

+echo "QA output created by $seq"
+echo "Silence is golden"
+rm -f $seqres.full
+
+status=1 # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup() {
+   if [ ! -z SCRATCH_MNT ]; then
+      rm -fr $SCRATCH_MNT/test*
+      _scratch_unmount
+   fi
+}

No. The harness will unmount the scratch device if it was mounted. Unless you need to do specific cleanup fo rthe test, do not modify the standard cleanup function. At minimum, it still needs to remove all the $tmp files created during the test run.

tests/ext4/309 line 48

Quoted 4 lines

tests/ext4/309 new file +4 -0

+
+_filter_error() {
+   sed -n -e "s/.*\($1\).*/\"\1\"/p"
+}

"_" prefix is reserved for library functions.

Comments are generally required to explain the intent of regexs that look like line noise.

8 space tabs.

tests/ext4/309 line 55

Quoted 7 lines

tests/ext4/309 new file +7 -0

+
+_clear_testdir() {
+   dirs="$testdir *$"
+   for i in $dirs; do
+      rm -fr $i/*
+   done
+}

I thought this was going to clean up the TEST_DIR, but as per above, it's actually doing stuff to the scratch device. "remove_files()" is good enough....

tests/ext4/309 line 64

Quoted 9 lines

tests/ext4/309 new file +9 -0

+
+# $1 - device
+# $2 - options
+_make_ext4fs() {
+   device=$1
+   opts=$2
+   umount $device 1>/dev/null 2>&1
+   mkfs.ext4 $opts $device 1>/dev/null 2>&1
+}

_mkfs_dev

tests/ext4/309 line 74

Quoted 10 lines

tests/ext4/309 new file +10 -0

+# $1 - options
+# $2 - mount point
+_make_loopfs() {
+   lpf=$testfile
+   dd if=/dev/zero of=$lpf bs=4k count=256 1>/dev/null 2>&1
+   loopdev=$(losetup -f)
+   losetup $loopdev $lpf
+   mkfs.ext4 -O ^dir_index,^has_journal $loopdev 1>/dev/null 2>&1
+   mount -t ext4 $1 $loopdev $2
+}

no need to create a loop device. mount -o loop will do what you want. Also, $MKFS_EXT4_PROG (or whatever the var is) shoul dbe used. As should _mount. And 8 space tabs.

tests/ext4/309 line 87

Quoted 13 lines

tests/ext4/309 new file +13 -0

+
+# $1 - expected limit after items creation
+# $2 - command to create item
+# $3 - where to create (testdir by default)
+_create_items() {
+    limit=$1
+    create_cmd=$2
+    dir=${3:-$testdir}
+    sync
+    echo 3 > /proc/sys/vm/drop_caches
+    MAX_INUM=$(((limit*1024*2)/24))
+    for i in $(seq 0 $MAX_INUM); do
+       tmp_name=`mktemp -u`

We use $tmp as the location for temporary files in tests

tests/ext4/309 line 91

Quoted 5 lines

tests/ext4/309 new file +4 -0

+       item=`basename $tmp_name`
+       if [ -e $dir/$item ]; then
+          continue
+       fi

Why?

tests/ext4/309 line 100

Quoted 9 lines

tests/ext4/309 new file +9 -0

+       create_cmd="$2 $dir/$item 2>$tmp.out 1>/dev/null"
+       eval "$create_cmd"
+       res=$?
+       if [ $res -ne 0 ]; then
+          _filter_error "No space left on device" < $tmp.out > $tmp.out2
+          if [ -s $tmp.out2 ]; then
+             cat $tmp.out2 | tr -d '\n' >> $seqres.full
+          else
+             echo "FAIL! expected ENOSPC" | tee -a $seqres.full

_fail?

tests/ext4/309 line 103

Quoted 4 lines

tests/ext4/309 new file +3 -0

+          fi
+          break
+      fi

This all seems rather convoluted. You're creating a tmp file to capture the error, then if you get an ENOSPC error you dump it to the debug file, otherwise you dump an error message to main output to cause the test to eventually fail?

Quoted 7 lines

tests/ext4/309 new file +7 -0

+   done
+   size=$(stat -c %s $dir)
+   size=$((size/1024))
+   if [ $size -gt $limit ]; then
+      echo "FAIL! expected dir size: $limit, actually: $size" | tee -a $seqres.full
+   fi
+   rm -f $tmp*

tests/ext4/309 line 120

Quoted 10 lines

tests/ext4/309 new file +10 -0

+}
+
+# get standard environment, filters and checks
+. ./common/rc
+
+# real QA test starts here
+
+_supported_fs ext4
+_supported_os Linux
+_require_scratch

_require_loop

tests/ext4/309 line 125

Quoted 5 lines

tests/ext4/309 new file +5 -0

+
+LIMIT1=8
+LIMIT2=16
+
+_make_ext4fs $SCRATCH_DEV "-O ^dir_index,^filetype"

_scratch_mkfs -O ^dir_index,^filetype

Cheers,

Dave.

From e30cd49f5ab84c029c0b376e702caeac42f59f49 Mon Sep 17 00:00:00 2001

Hello Dave,

Thank you for the review, I've updated test according to your comments

From e30cd49f5ab84c029c0b376e702caeac42f59f49 Mon Sep 17 00:00:00 2001
From: Alexander Tsvetkov <[email protected]>
Date: Mon, 15 Dec 2014 18:49:42 +0300
Subject: [PATCH] added test for max_dir_size_kb mount option

---
 tests/ext4/309     | 178 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/ext4/309.out |   2 +
 tests/ext4/group   |   1 +
 3 files changed, 181 insertions(+)
 create mode 100755 tests/ext4/309
 create mode 100755 tests/ext4/309.out

tests/ext4/309 +6 -0

+   done
+   size=$(stat -c %s $dir)
+   size=$((size / 1024))
+   if [ $size -gt $limit ]; then
+      echo "FAIL! expected dir size: $limit, actually: $size" | tee -a $seqres.full
+   fi

tests/ext4/309 +12 -0

+
+   echo -e "\nRemount with $LIMIT1 Kb limit,\nnew item in testdir/ should result to ENOSPC: " >>$seqres.full
+   _scratch_mount "-o remount,max_dir_size_kb=$LIMIT1"
+   _create_items $LIMIT1
+
+   echo -e "\nExceed $LIMIT2 Kb limit with new files in testdir/: " >> $seqres.full
+   _scratch_mount "-o remount,max_dir_size_kb=$LIMIT2"
+   _create_items $LIMIT2
+
+   echo -e "\nExceed $LIMIT2 Kb limit with new files in testdir2/: " >> $seqres.full
+   mkdir $SCRATCH_MNT/testdir2 2>/dev/null
+   _create_items $LIMIT2 "$SCRATCH_MNT/testdir2"

Quoted 3 lines · Alexander Tsvetkov

Hello Dave,

Thank you for the review, I've updated test according to your comments

....

Quoted 5 lines

From e30cd49f5ab84c029c0b376e702caeac42f59f49 Mon Sep 17 00:00:00 2001
From: Alexander Tsvetkov <[email protected]>
Date: Mon, 15 Dec 2014 18:49:42 +0300
Subject: [PATCH] added test for max_dir_size_kb mount option

Quoted 7 lines

---
  tests/ext4/309     | 178 +++++++++++++++++++++++++++++++++++++++++++++++++++++
  tests/ext4/309.out |   2 +
  tests/ext4/group   |   1 +
  3 files changed, 181 insertions(+)

create mode 100755 tests/ext4/309
create mode 100755 tests/ext4/309.out

This is missing a commit message describing the change, as well as a change log telling me what changed from v1 to v2. Hence I don't know exactly what you changed and what you ignored.

tests/ext4/309

Quoted 1 line

tests/ext4/309 +0 -0

Just use the next unused number in the ext4 directory.

tests/ext4/309 line 49

Quoted 7 lines

tests/ext4/309 +6 -0

+remove_files()
+{
+   dirs="$testdir $*"
+   for i in $dirs; do
+      rm -fr $i/*
+   done

Still whitespace damaged. Please use 8 space tabs.

tests/ext4/309 line 55

Quoted 6 lines

tests/ext4/309 +6 -0

+}
+
+# $1 - expected limit after items creation
+# $2 - command to create item
+# $3 - where to create (testdir by default)
+_create_items()

Still got a "_ prefix"

tests/ext4/309 line 65

Quoted 10 lines

tests/ext4/309 +10 -0

+{
+    limit=$1
+    dir=${2:-$testdir}
+    MKTEMP_OPT=""
+    [ "$3" = "mkdir" ] && MKTEMP_OPT="-d"
+    sync
+    echo 3 > /proc/sys/vm/drop_caches
+    MAX_INUM=$((limit * 1024 * 2 / 24))
+    for i in $(seq 0 $MAX_INUM); do
+       error=$(mktemp $MKTEMP_OPT --tmpdir=$dir 2>&1 >/dev/null)

Still using mktemp, only now in a much more convoluted manner.

If you just want to create a file, "touch $dir/$i" is all you need to do.

tests/ext4/309 line 71

Quoted 6 lines

tests/ext4/309 +6 -0

+       res=$?
+       if [ $res -ne 0 ]; then
+          echo $error >> $seqres.full
+          [[ ! $error =~ ^.*'No space left on device'$ ]] && echo "FAIL! expected ENOSPC" | tee -a $seqres.full
+          break
+      fi

You didn't answer any of the questions I asked about this, nor address the comments I made.

Just filter the error to sanitse it down to "No space left on device" and break. The golden output match will fail the test if there's any other type of error. i.e:

	for i in $(seq 0 $MAX_INUM); do
		touch $dir/$i 2>&1 | _filter_scratch
		if [ $? -ne 0 ]; then
			break;
		fi
	done

will test everything the above loop do (except the obvious touch vs mkdir difference).

Quoted 9 lines

tests/ext4/309 +9 -0

+   done
+   size=$(stat -c %s $dir)
+   size=$((size / 1024))
+   if [ $size -gt $limit ]; then
+      echo "FAIL! expected dir size: $limit, actually: $size" | tee -a $seqres.full
+   fi
+}
+
+run_test()

tests/ext4/309 line 87

Quoted 7 lines

tests/ext4/309 +7 -0

+{
+   LIMIT1=$1
+   LIMIT2=$2
+   MKFS_OPT=$3
+
+   _scratch_unmount >/dev/null 2>&1
+   _scratch_mkfs $MKFS_OPT >>$seqres.full 2>&1

_scratch_mkfs unmounts the SCRATCH_DEV.

tests/ext4/309 line 92

Quoted 5 lines

tests/ext4/309 +5 -0

+   _scratch_mount -o max_dir_size_kb=$LIMIT1
+   mkdir $testdir
+
+   echo -e "\nExceed $LIMIT1 Kb limit with new files in testdir/: " >> $seqres.full
+   _create_items $LIMIT1

I don't see much point in all these echos to $seqres.full.

Quoted 15 lines

tests/ext4/309 +15 -0

+
+   echo -e "\nRemount with $LIMIT1 Kb limit,\nnew item in testdir/ should result to ENOSPC: " >>$seqres.full
+   _scratch_mount "-o remount,max_dir_size_kb=$LIMIT1"
+   _create_items $LIMIT1
+
+   echo -e "\nExceed $LIMIT2 Kb limit with new files in testdir/: " >> $seqres.full
+   _scratch_mount "-o remount,max_dir_size_kb=$LIMIT2"
+   _create_items $LIMIT2
+
+   echo -e "\nExceed $LIMIT2 Kb limit with new files in testdir2/: " >> $seqres.full
+   mkdir $SCRATCH_MNT/testdir2 2>/dev/null
+   _create_items $LIMIT2 "$SCRATCH_MNT/testdir2"
+
+   echo -e "\nRemount with $LIMIT1 Kb limit,\nnew item in testdir/ should result to ENOSPC: " >> $seqres.full
+   _scratch_mount "-o remount,max_dir_size_kb=$LIMIT1"

tests/ext4/309 line 114

Quoted 7 lines

tests/ext4/309 +7 -0

+   _create_items $LIMIT2
+   echo -e "\nnew item in testdir2/ should result to ENOSPC: " >> $seqres.full
+   _create_items $LIMIT2 "$SCRATCH_MNT/testdir2"
+   remove_files "$SCRATCH_MNT/testdir2"
+   rmdir $testdir
+   mkdir $testdir
+   dd if=/dev/urandom of=$testfile bs=1 seek=4096 count=4096 > /dev/null 2>&1

Use xfs_io to write data to files, not dd.

tests/ext4/309 line 123

Quoted 9 lines

tests/ext4/309 +9 -0

+
+   echo -e "\nExceed $LIMIT1 Kb directory limit with new subdirectories: " >> $seqres.full
+   _create_items $LIMIT1 $testdir "mkdir"
+   remove_files
+
+   echo -e "\nCreate ext4 fs on testdir/subdir with $LIMIT2 Kb limit," >> $seqres.full
+   mkdir $testdir/subdir 2>/dev/null
+   umount $TEST_DEV 1>/dev/null 2>&1
+   _mkfs_dev $TEST_DEV $MKFS_OPT >>$seqres.full 2>&1

You are not allowed to mkfs the test device during any test. You should not even be unmounting it.

You need to use loop devices
if you want to do this, though I don't see why you need to use a second nested filesystem mount just to test a different limit,

especially as:

tests/ext4/309 line 157

Quoted 10 lines

tests/ext4/309 +10 -0

+   $MKFS_EXT4_PROG -F $MKFS_OPT $testfile 2m >> $seqres.full 2>&1
+   _mount -o loop,max_dir_size_kb=$LIMIT1 $testfile $testdir/subdir
+
+   echo "exceed $LIMIT1 Kb limit of testdir/subdir with a set of files:" >> $seqres.full
+   _create_items $LIMIT1 "$testdir/subdir"
+
+   echo -e "\nexceed $LIMIT2 Kb limit of testdir/ with a set of files:" >> $seqres.full
+   _create_items $LIMIT2
+
+   umount -d $testdir/subdir

You test loop devices here....

Cheers,

Dave.

Quoted 19 lines · Dave Chinner

Quoted 3 lines · Alexander Tsvetkov

Hello Dave,

Thank you for the review, I've updated test according to your comments

....

Quoted 5 lines

From e30cd49f5ab84c029c0b376e702caeac42f59f49 Mon Sep 17 00:00:00 2001
From: Alexander Tsvetkov <[email protected]>
Date: Mon, 15 Dec 2014 18:49:42 +0300
Subject: [PATCH] added test for max_dir_size_kb mount option

Quoted 8 lines

---
  tests/ext4/309     | 178 
+++++++++++++++++++++++++++++++++++++++++++++++++++++
  tests/ext4/309.out |   2 +
  tests/ext4/group   |   1 +
  3 files changed, 181 insertions(+)

create mode 100755 tests/ext4/309
create mode 100755 tests/ext4/309.out

Quoted 3 lines

This is missing a commit message describing the change, as well as a change log telling me what changed from v1 to v2. Hence I don't know exactly what you changed and what you ignored.

ok, it seems patch was not correctly collected

Quoted 2 lines

tests/ext4/309

Quoted 1 line

tests/ext4/309 +0 -0

Just use the next unused number in the ext4 directory.

do you mean 004?

Quoted 8 lines

tests/ext4/309

Quoted 7 lines

tests/ext4/309 +6 -0

+remove_files()
+{
+   dirs="$testdir $*"
+   for i in $dirs; do
+      rm -fr $i/*
+   done

Still whitespace damaged. Please use 8 space tabs.

ok

Quoted 7 lines

tests/ext4/309

Quoted 6 lines

tests/ext4/309 +6 -0

+}
+
+# $1 - expected limit after items creation
+# $2 - command to create item
+# $3 - where to create (testdir by default)
+_create_items()

Still got a "_ prefix"

ok

Quoted 10 lines

tests/ext4/309

Quoted 10 lines

tests/ext4/309 +10 -0

+{
+    limit=$1
+    dir=${2:-$testdir}
+    MKTEMP_OPT=""
+    [ "$3" = "mkdir" ] && MKTEMP_OPT="-d"
+    sync
+    echo 3 > /proc/sys/vm/drop_caches
+    MAX_INUM=$((limit * 1024 * 2 / 24))
+    for i in $(seq 0 $MAX_INUM); do
+       error=$(mktemp $MKTEMP_OPT --tmpdir=$dir 2>&1 >/dev/null)

Quoted 4 lines

Still using mktemp, only now in a much more convoluted manner.

If you just want to create a file, "touch $dir/$i" is all you need to do.

I use mktemp to create items of fixed size that allows
to define the maximum dir items number corresponding to specified limit which is calculated MAX_INUM=$((limit * 1024 * 2 / 24)): file name "tmp.XXXXXXXXXX" of 14 bytes +8 bytes of ext4_dir_entry control data+2 bytes for padding = 24 bytes. It is multiplied on 2 so in case of failed max_dir_size_kb, i.e. overlimit, the size of test directory will be greater on one block.

Quoted 9 lines

tests/ext4/309

Quoted 7 lines

tests/ext4/309 +4 -0

+       res=$?
+       if [ $res -ne 0 ]; then
+          echo $error >> $seqres.full
+          [[ ! $error =~ ^.*'No space left on device'$ ]] && echo 

"FAIL! expected ENOSPC" | tee -a $seqres.full

tests/ext4/309 +2 -0

+          break
+      fi

You didn't answer any of the questions I asked about this, nor address the comments I made.

Sorry, I thought your comments were about convolution only.

Quoted 11 lines

Just filter the error to sanitse it down to "No space left on device" and break. The golden output match will fail the test if there's any other type of error. i.e:

    for i in $(seq 0 $MAX_INUM); do
        touch $dir/$i 2>&1 | _filter_scratch
        if [ $? -ne 0 ]; then
            break;
        fi
    done

Quoted 3 lines

will test everything the above loop do (except the obvious touch vs mkdir difference).

The error output style is the same as the similar one in this function:

(quoted diff) +3 -1

+   if [ $size -gt $limit ]; then
+      echo "FAIL! expected dir size: $limit, actually: $size" | tee 
-a $seqres.full
+   fi

The idea is to provide more descriptive error messages in output for both checks, what was expected and what's happened actually helping more quickly understand the type of failure.

Quoted 10 lines

Quoted 10 lines

tests/ext4/309 +9 -1

+   done
+   size=$(stat -c %s $dir)
+   size=$((size / 1024))
+   if [ $size -gt $limit ]; then
+      echo "FAIL! expected dir size: $limit, actually: $size" | tee 
-a $seqres.full
+   fi
+}
+
+run_test()

Quoted 8 lines

tests/ext4/309

Quoted 7 lines

tests/ext4/309 +7 -0

+{
+   LIMIT1=$1
+   LIMIT2=$2
+   MKFS_OPT=$3
+
+   _scratch_unmount >/dev/null 2>&1
+   _scratch_mkfs $MKFS_OPT >>$seqres.full 2>&1

_scratch_mkfs unmounts the SCRATCH_DEV.

ok

Quoted 7 lines

Quoted 6 lines

tests/ext4/309 +4 -0

+   _scratch_mount -o max_dir_size_kb=$LIMIT1
+   mkdir $testdir
+
+   echo -e "\nExceed $LIMIT1 Kb limit with new files in testdir/: " 

Quoted 1 line

Quoted 1 line

$seqres.full

+ _create_items $LIMIT1

I don't see much point in all these echos to $seqres.full.

These test descriptions are used to differ test case from others in the test and in logs, helping the finding of test case failure or it's source code when reading the test. Otherwise it's unclear which test case failed when getting some error in test out file.

Quoted 21 lines

Quoted 21 lines

tests/ext4/309 +2 -0

+
+   echo -e "\nRemount with $LIMIT1 Kb limit,\nnew item in testdir/ 

should result to ENOSPC: " >>$seqres.full

tests/ext4/309 +4 -0

+   _scratch_mount "-o remount,max_dir_size_kb=$LIMIT1"
+   _create_items $LIMIT1
+
+   echo -e "\nExceed $LIMIT2 Kb limit with new files in testdir/: " 

Quoted 1 line

Quoted 1 line

$seqres.full

tests/ext4/309 +4 -0

+   _scratch_mount "-o remount,max_dir_size_kb=$LIMIT2"
+   _create_items $LIMIT2
+
+   echo -e "\nExceed $LIMIT2 Kb limit with new files in testdir2/: 

" >> $seqres.full

tests/ext4/309 +4 -0

+   mkdir $SCRATCH_MNT/testdir2 2>/dev/null
+   _create_items $LIMIT2 "$SCRATCH_MNT/testdir2"
+
+   echo -e "\nRemount with $LIMIT1 Kb limit,\nnew item in testdir/ 

should result to ENOSPC: " >> $seqres.full

tests/ext4/309 +3 -0

+   _scratch_mount "-o remount,max_dir_size_kb=$LIMIT1"
+   _create_items $LIMIT2
+   echo -e "\nnew item in testdir2/ should result to ENOSPC: " >> 

Quoted 8 lines

Quoted 7 lines

$seqres.full

tests/ext4/309 +5 -0

+   _create_items $LIMIT2 "$SCRATCH_MNT/testdir2"
+   remove_files "$SCRATCH_MNT/testdir2"
+   rmdir $testdir
+   mkdir $testdir
+   dd if=/dev/urandom of=$testfile bs=1 seek=4096 count=4096 > 

/dev/null 2>&1

Use xfs_io to write data to files, not dd.

ok, will be applied

Quoted 5 lines

Quoted 5 lines

tests/ext4/309 +2 -0

+
+   echo -e "\nExceed $LIMIT1 Kb directory limit with new 

subdirectories: " >> $seqres.full

tests/ext4/309 +2 -0

+   _create_items $LIMIT1 $testdir "mkdir"
+   remove_files

Quoted 8 lines

tests/ext4/309

Quoted 6 lines

tests/ext4/309 +2 -0

+
+   echo -e "\nCreate ext4 fs on testdir/subdir with $LIMIT2 Kb 

limit," >> $seqres.full

tests/ext4/309 +3 -0

+   mkdir $testdir/subdir 2>/dev/null
+   umount $TEST_DEV 1>/dev/null 2>&1
+   _mkfs_dev $TEST_DEV $MKFS_OPT >>$seqres.full 2>&1

You are not allowed to mkfs the test device during any test. You should not even be unmounting it.

I didn't know about this restriction, will rewrite these parts.

Quoted 3 lines

You need to use loop devices
if you want to do this, though I don't see why you need to use a second nested filesystem mount just to test a different limit,

This is robustness testing when test conditions are special cases,
to test filling of directories up to different limits which, for example, are nested and mounted on different filesystems, when another filesystem
on loop etc. to cover possibly more paths in the filesystem code.

The loop devices test case was also separated as particular because I had some xfstests failures in other tests reproduced on loop devices only. Just to be sure that it is also covered here.

Quoted 16 lines

especially as:

tests/ext4/309

Quoted 12 lines

tests/ext4/309 +4 -0

+   $MKFS_EXT4_PROG -F $MKFS_OPT $testfile 2m >> $seqres.full 2>&1
+   _mount -o loop,max_dir_size_kb=$LIMIT1 $testfile $testdir/subdir
+
+   echo "exceed $LIMIT1 Kb limit of testdir/subdir with a set of 

files:" >> $seqres.full

tests/ext4/309 +3 -0

+   _create_items $LIMIT1 "$testdir/subdir"
+
+   echo -e "\nexceed $LIMIT2 Kb limit of testdir/ with a set of 

files:" >> $seqres.full

tests/ext4/309 +3 -0

+   _create_items $LIMIT2
+
+   umount -d $testdir/subdir

You test loop devices here....

Quoted 3 lines

Cheers,

Dave.

Thanks,
Alexander Tsvetkov

From 6f90894347d579e6cc6be9af159eb5d4a12c059e Mon Sep 17 00:00:00 2001

Hello Dave,

Attached updated version of the test.

From 6f90894347d579e6cc6be9af159eb5d4a12c059e Mon Sep 17 00:00:00 2001
From: Alexander Tsvetkov <[email protected]>
Date: Tue, 23 Dec 2014 14:58:13 +0300
Subject: [PATCH] added test for max_dir_size_kb mount option

---

 tests/ext4/005     | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/ext4/005.out |   2 +
 tests/ext4/group   |   1 +
 3 files changed, 140 insertions(+)
 create mode 100755 tests/ext4/005
 create mode 100644 tests/ext4/005.out

tests/ext4/005 new file +20 -0

new file mode 100755
index 0000000..4cfcb25
@@ -0,0 +1,137 @@
+#! /bin/bash
+# FS QA Test
+#
+# Test for mount option max_dir_size_kb
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2014 Oracle and/or its affiliates.  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

tests/ext4/005 new file +16 -0

+
+echo "QA output created by $seq"
+echo "Silence is golden"
+rm -f $seqres.full
+
+status=1 # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -rf $tmp.*
+}
+
+# filter expected output with ENOSPC error
+filter_enospc()

tests/ext4/005 new file +19 -0

+		if [ ${PIPESTATUS[0]} -ne 0 ]; then
+			break
+		fi
+	done
+	size=$(stat -c %s $dir)
+	size=$((size / 1024))
+	if [ $size -gt $limit ]; then
+		echo "FAIL! expected dir size: $limit, actually: $size"
+	fi
+}
+
+# $1 - low directory limit value
+# $2 - high directory limit value 
+# $3 - mkfs options
+run_test()
+{
+	LIMIT1=$1
+	LIMIT2=$2
+	MKFS_OPT=$3

tests/ext4/005 new file +5 -0

+	# Exceed with high limit after remount 
+	_scratch_mount "-o remount,max_dir_size_kb=$LIMIT2"
+	create_items $LIMIT2
+
+	# Exceed with low limit after remount 

tests/ext4/005 new file +11 -0

+	_mount -o loop,max_dir_size_kb=$LIMIT2 $testfile $testdir2
+	create_items $LIMIT1
+	create_items $LIMIT2 $testdir2
+	_scratch_mount "-o remount,max_dir_size_kb=$LIMIT2"
+	_mount -o remount,max_dir_size_kb=$LIMIT1 $testfile $testdir2
+	create_items $LIMIT2
+	create_items $LIMIT2 $testdir2
+
+	umount -d $testdir2
+	_scratch_unmount >/dev/null 2>&1
+}

Quoted 3 lines · Alexander Tsvetkov

Hello Dave,

Attached updated version of the test.

[snip entire quoted previous emails]

Closer, but still not quite all there.

A couple of things about posting patches, first. please don't top post and when sending a new version of a patch, please send it with an appropriate subject such as:

[PATCH v4] ext4: Add new test for max_dir_size_kb mount option

So it's clear that there's a new version of the patch been sent. Also, the patch should be in-line, not an attachment.

Quoted 6 lines

From 6f90894347d579e6cc6be9af159eb5d4a12c059e Mon Sep 17 00:00:00 2001
From: Alexander Tsvetkov <[email protected]>
Date: Tue, 23 Dec 2014 14:58:13 +0300
Subject: [PATCH] added test for max_dir_size_kb mount option

---

The commit message should not be blank - it needs to explain why the new test is needed.

Also, the change history of the patch (what's changed between each version posted) should be documented here below the first "---" delimiter so people who have already commented on previous versions know what you have and haven't changed.

Fo more information, please go and read Documentation/SubmittingPatches from your local kernel source tree.

Quoted 40 lines

  tests/ext4/005     | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++++
  tests/ext4/005.out |   2 +
  tests/ext4/group   |   1 +
  3 files changed, 140 insertions(+)

create mode 100755 tests/ext4/005
create mode 100644 tests/ext4/005.out

tests/ext4/005 new file +26 -0

new file mode 100755
index 0000000..4cfcb25
@@ -0,0 +1,137 @@
+#! /bin/bash
+# FS QA Test
+#
+# Test for mount option max_dir_size_kb
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2014 Oracle and/or its affiliates.  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
+tmp=/tmp/$$

tests/ext4/005 line 30

Quoted 4 lines

tests/ext4/005 new file +4 -0

+
+testdir=$SCRATCH_MNT/dir1.$seq
+testdir2=$testdir/dir2.$seq
+testfile=$SCRATCH_MNT/testfile

I think I've already pointed out that "testdir" should not be used as a local variable to point to something on the scratch device. It's too easily confused with TEST_DIR. Please rename them to something less confusing e.g. dir1, dir2, and fsimg_file.

Quoted 19 lines

tests/ext4/005 new file +19 -0

+
+echo "QA output created by $seq"
+echo "Silence is golden"
+rm -f $seqres.full
+
+status=1 # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -rf $tmp.*
+}
+
+# filter expected output with ENOSPC error
+filter_enospc()
+{
+	sed -e "/^.*No space left on device$/d"
+}

tests/ext4/005 line 56

Quoted 7 lines

tests/ext4/005 new file +7 -0

+
+# $1 - expected limit after filling
+# $2 - where to create
+create_items()
+{
+	limit=$1
+	dir=${2:-$testdir}

Just pass the variable at the call site.

tests/ext4/005 line 58

Quoted 2 lines

tests/ext4/005 new file +2 -0

+	sync
+	echo 3 > /proc/sys/vm/drop_caches

Why is this necessary given there's always a remount just before this is called?

tests/ext4/005 line 61

Quoted 3 lines

tests/ext4/005 new file +3 -0

+	MAX_INUM=$((limit * 1024 * 3 / 24))
+	for i in $(seq 0 $MAX_INUM); do
+		touch $dir/$i 2>&1 1>/dev/null | filter_enospc

touch is silent when it succeeds, so there's no need to redirect anything to /dev/null.

Quoted 24 lines

tests/ext4/005 new file +24 -0

+		if [ ${PIPESTATUS[0]} -ne 0 ]; then
+			break
+		fi
+	done
+	size=$(stat -c %s $dir)
+	size=$((size / 1024))
+	if [ $size -gt $limit ]; then
+		echo "FAIL! expected dir size: $limit, actually: $size"
+	fi
+}
+
+# $1 - low directory limit value
+# $2 - high directory limit value
+# $3 - mkfs options
+run_test()
+{
+	LIMIT1=$1
+	LIMIT2=$2
+	MKFS_OPT=$3
+
+	_scratch_mkfs $MKFS_OPT >>$seqres.full 2>&1
+	_scratch_mount -o max_dir_size_kb=$LIMIT1
+	mkdir $testdir
+

tests/ext4/005 line 90

Quoted 5 lines

tests/ext4/005 new file +5 -0

+	# Exceed with low limit
+	create_items $LIMIT1
+
+	# Exceed with the same limit after remount
+	_scratch_mount "-o remount,max_dir_size_kb=$LIMIT1"

Urk. We have _scratch_remount, so this is kinda confusing as to be using _scratch_mount to do remounts. You need to document why this is a safe thing to do.

tests/ext4/005 line 91

Quoted 1 line

tests/ext4/005 new file +1 -0

+	create_items $LIMIT1

And. well, after a remount, running sync and dropping caches is unnecessary...

Quoted 5 lines

tests/ext4/005 new file +5 -0

+	# Exceed with high limit after remount
+	_scratch_mount "-o remount,max_dir_size_kb=$LIMIT2"
+	create_items $LIMIT2
+
+	# Exceed with low limit after remount

tests/ext4/005 line 106

Quoted 10 lines

tests/ext4/005 new file +10 -0

+	_scratch_mount "-o remount,max_dir_size_kb=$LIMIT1"
+	create_items $LIMIT2
+
+	# Exceed limits of two test dirs resided on different fs,
+	# second fs is mounted on nested test dir of the first fs
+	rm -fr $testdir/*
+	rmdir $testdir
+	mkdir -p $testdir2
+	touch $testfile
+	$MKFS_EXT4_PROG -F $MKFS_OPT $testfile 4m >> $seqres.full 2>&1

_mkfs_dev $testfile 4m ?

Quoted 15 lines

tests/ext4/005 new file +14 -0

+	_mount -o loop,max_dir_size_kb=$LIMIT2 $testfile $testdir2
+	create_items $LIMIT1
+	create_items $LIMIT2 $testdir2
+	_scratch_mount "-o remount,max_dir_size_kb=$LIMIT2"
+	_mount -o remount,max_dir_size_kb=$LIMIT1 $testfile $testdir2
+	create_items $LIMIT2
+	create_items $LIMIT2 $testdir2
+
+	umount -d $testdir2
+	_scratch_unmount >/dev/null 2>&1
+}
+
+# get standard environment, filters and checks
+. ./common/rc

tests/ext4/005 line 127

Quoted 7 lines

tests/ext4/005 new file +7 -0

+
+# real QA test starts here
+
+_supported_fs ext4
+_supported_os Linux
+_require_scratch
+_require_loop

Put these at the top after the _cleanup function.

Cheers,

Dave.