[PATCH v3] Add test 249: Check filesystem FITRIM implementation

[PATCH v3] Add test 249: Check filesystem FITRIM implementation

[Lukas Czerner]

FITRIM ioctl  is used on a mounted filesystem to discard (or "trim")
blocks which are not in use by the filesystem.  This is useful for
solid-state drives (SSDs) and thinly-provi-sioned storage. This test
helps to verify filesystem FITRIM implementation to assure that it
does not corrupts data.

This test creates checksums of all files in xfstests directory and
run several processes which clear its working directory on SCRATCH_MNT,
then copy everything from xfstests into its working directory, create
list of files in working directory and its checksums and compare it with the
original list of checksums. Every process works in the loop so it repeat
remove->copy->check, while fstrim tool is running simultaneously.

Fstrim is just a helper tool which uses FITRIM ioctl to actually do the
filesystem discard.

I found this very useful because when the FITRIM is really buggy (thus
data-destroying) the 249 test will notice, because checksums will most
likely change.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 249          |  170 ++++++++++++++++++++++++++++++++++++++
 249.out      |    3 +
 group        |    3 +-
 src/Makefile |    2 +-
 src/fstrim.c |  257 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 433 insertions(+), 2 deletions(-)
 create mode 100644 249
 create mode 100644 249.out
 create mode 100644 src/fstrim.c

diff --git a/249 b/249
new file mode 100644
index 0000000..9943176
--- /dev/null
+++ b/249
@@ -0,0 +1,170 @@
+#!/bin/bash
+# FS QA Test No. 248
+#
+# This test was created in order to verify filesystem FITRIM implementation.
+# By many concurrent copy and remove operations and checking that files
+# does not change after copied into SCRATCH_MNT test if FITRIM implementation
+# corrupts the filesystem (data/metadata).
+#
+#-----------------------------------------------------------------------
+# Copyright 2010 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
+#
+# 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
+#-----------------------------------------------------------------------
+
+owner=lczerner@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=`mktemp -d`
+status=1    # failure is the default!
+trap "_cleanup; exit \$status" 0 1 3
+trap "_destroy; exit \$status" 2 15
+chpid=0
+mypid=$$
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+_cleanup()
+{
+	rm -rf $tmp
+}
+
+_destroy()
+{
+	kill $pids $fstrim_pid
+	wait $pids $fstrim_pid
+	rm -rf $tmp
+}
+
+_destroy_fstrim()
+{
+	kill $fpid
+	wait $fpid
+}
+
+_fail()
+{
+	echo "$1"
+	kill $mypid
+}
+
+##
+# Background FSTRIM loop. We are trimming the device in the loop and for
+# test coverage, we are doing whole device trim followed by several smaller
+# trims.
+##
+fstrim_loop()
+{
+	trap "_destroy_fstrim; exit \$status" 2 15
+	fsize=$(df | grep $SCRATCH_MNT | grep $SCRATCH_DEV  | awk '{print $2}')
+
+	while true ; do
+		step=1048576
+		start=0
+		$here/src/fstrim $SCRATCH_MNT &
+		fpid=$!
+		wait $fpid
+		while [ $start -lt $fsize ] ; do
+			$here/src/fstrim -s ${start}k -l ${step}k $SCRATCH_MNT &
+			fpid=$!
+			wait $fpid
+			start=$(( $start + $step ))
+		done
+	done
+}
+
+function check_sums() {
+	(
+	cd $SCRATCH_MNT/$p
+	find -P . -xdev -type f -print0 | xargs -0 md5sum | sort -o $tmp/stress.$$.$p
+	)
+
+	diff $tmp/content.sums $tmp/stress.$$.$p
+	if [ $? -ne 0 ]; then
+		_fail "!!!Checksums has changed - Filesystem possibly corrupted!!!\n"
+	fi
+	rm -f $tmp/stress.$$.$p
+}
+
+function run_process() {
+	local p=$1
+	repeat=10
+
+	sleep $((5*$p))s &
+	export chpid=$! && wait $chpid &> /dev/null
+	chpid=0
+
+	while [ $repeat -gt 0 ]; do
+
+		# Remove old directories.
+		rm -rf $SCRATCH_MNT/$p
+		export chpid=$! && wait $chpid &> /dev/null
+
+		# Copy content -> partition.
+		mkdir $SCRATCH_MNT/$p
+		cp -axT $content $SCRATCH_MNT/$p
+		export chpid=$! && wait $chpid &> /dev/null
+
+		check_sums
+		repeat=$(( $repeat - 1 ))
+	done
+}
+
+nproc=20
+content=$here
+
+# Check for FITRIM support
+echo -n "Checking FITRIM support: "
+$here/src/fstrim -l 10M $SCRATCH_MNT &> /dev/null
+[ $? -ne 0 ] && exit
+echo "done."
+
+mkdir -p $tmp
+
+(
+cd $content
+find -P . -xdev -type f -print0 | xargs -0 md5sum | sort -o $tmp/content.sums
+)
+
+echo -n "Running the test: "
+pids=""
+fstrim_loop &
+fstrim_pid=$!
+p=1
+while [ $p -le $nproc ]; do
+	run_process $p &
+	pids="$pids $!"
+	p=$(($p+1))
+done
+echo "done."
+
+wait $pids
+kill $fstrim_pid
+wait $fstrim_pid
+
+status=0
+
+exit
diff --git a/249.out b/249.out
new file mode 100644
index 0000000..880d9c7
--- /dev/null
+++ b/249.out
@@ -0,0 +1,3 @@
+QA output created by 248
+Checking FITRIM support: done.
+Running the test: done.
diff --git a/group b/group
index a40c98f..04c45b5 100644
--- a/group
+++ b/group
@@ -361,4 +361,5 @@ deprecated
 245 auto quick dir
 246 auto quick rw
 247 auto quick rw
-248 auto quick rw 
+248 auto quick rw
+249 ioctl trim
diff --git a/src/Makefile b/src/Makefile
index 47d7334..3d35e26 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -17,7 +17,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
 	locktest unwritten_mmap bulkstat_unlink_test t_stripealign \
 	bulkstat_unlink_test_modified t_dir_offset t_futimens t_immutable \
-	stale_handle pwrite_mmap_blocked
+	stale_handle pwrite_mmap_blocked fstrim
 
 SUBDIRS =
 
diff --git a/src/fstrim.c b/src/fstrim.c
new file mode 100644
index 0000000..a93a6f3
--- /dev/null
+++ b/src/fstrim.c
@@ -0,0 +1,257 @@
+/*
+ * fstrim.c -- discard the part (or whole) of mounted filesystem.
+ *
+ * Copyright (C) 2009 Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
+ *
+ * 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, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will 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, see <http://www.gnu.org/licenses/>.
+ *
+ * This program uses FITRIM ioctl to discard parts or the whole filesystem
+ * online (mounted). You can specify range (start and lenght) to be
+ * discarded, or simply discard while filesystem.
+ *
+ * Usage: fstrim [options] <mount point>
+ */
+
+#include <string.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <stdarg.h>
+#include <getopt.h>
+
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <linux/fs.h>
+
+#ifndef FITRIM
+struct fstrim_range {
+	uint64_t start;
+	uint64_t len;
+	uint64_t minlen;
+};
+#define FITRIM		_IOWR('X', 121, struct fstrim_range)
+#endif
+
+const char *program_name = "fstrim";
+
+struct options {
+	struct fstrim_range *range;
+	char mpoint[PATH_MAX];
+	char verbose;
+};
+
+static void usage(void)
+{
+	fprintf(stderr,
+		"Usage: %s [-s start] [-l length] [-m minimum-extent]"
+		" [-v] {mountpoint}\n\t"
+		"-s Starting Byte to discard from\n\t"
+		"-l Number of Bytes to discard from the start\n\t"
+		"-m Minimum extent length to discard\n\t"
+		"-v Verbose - number of discarded bytes\n",
+		program_name);
+}
+
+static void err_exit(const char *fmt, ...)
+{
+	va_list pvar;
+	va_start(pvar, fmt);
+	vfprintf(stderr, fmt, pvar);
+	va_end(pvar);
+	usage();
+	exit(EXIT_FAILURE);
+}
+
+/**
+ * Get the number from argument. It can be number followed by
+ * units: k|K, m|M, g|G, t|T
+ */
+static unsigned long long get_number(char **optarg)
+{
+	char *opt, *end;
+	unsigned long long number, max;
+
+	/* get the max to avoid overflow */
+	max = ULLONG_MAX / 1024;
+	number = 0;
+	opt = *optarg;
+
+	if (*opt == '-') {
+		err_exit("%s: %s (%s)\n", program_name,
+			 strerror(ERANGE), *optarg);
+	}
+
+	errno = 0;
+	number = strtoul(opt, &end , 0);
+	if (errno)
+		err_exit("%s: %s (%s)\n", program_name,
+			 strerror(errno), *optarg);
+
+	/*
+	 * Convert units to numbers. Fall-through stack is used for units
+	 * so absence of breaks is intentional.
+	 */
+	switch (*end) {
+	case 'T': /* terabytes */
+	case 't':
+		if (number > max)
+			err_exit("%s: %s (%s)\n", program_name,
+				 strerror(ERANGE), *optarg);
+		number *= 1024;
+	case 'G': /* gigabytes */
+	case 'g':
+		if (number > max)
+			err_exit("%s: %s (%s)\n", program_name,
+				 strerror(ERANGE), *optarg);
+		number *= 1024;
+	case 'M': /* megabytes */
+	case 'm':
+		if (number > max)
+			err_exit("%s: %s (%s)\n", program_name,
+				 strerror(ERANGE), *optarg);
+		number *= 1024;
+	case 'K': /* kilobytes */
+	case 'k':
+		if (number > max)
+			err_exit("%s: %s (%s)\n", program_name,
+				 strerror(ERANGE), *optarg);
+		number *= 1024;
+		end++;
+	case '\0': /* end of the string */
+		break;
+	default:
+		err_exit("%s: %s (%s)\n", program_name,
+			 strerror(EINVAL), *optarg);
+		return 0;
+	}
+
+	if (*end != '\0') {
+		err_exit("%s: %s (%s)\n", program_name,
+			 strerror(EINVAL), *optarg);
+	}
+
+	return number;
+}
+
+static int parse_opts(int argc, char **argv, struct options *opts)
+{
+	int c;
+
+	while ((c = getopt(argc, argv, "s:l:m:v")) != EOF) {
+		switch (c) {
+		case 's': /* starting point */
+			opts->range->start = get_number(&optarg);
+			break;
+		case 'l': /* length */
+			opts->range->len = get_number(&optarg);
+			break;
+		case 'm': /* minlen */
+			opts->range->minlen = get_number(&optarg);
+			break;
+		case 'v': /* verbose */
+			opts->verbose = 1;
+			break;
+		default:
+			return EXIT_FAILURE;
+		}
+	}
+
+	return 0;
+}
+
+int main(int argc, char **argv)
+{
+	struct options *opts;
+	struct stat sb;
+	int fd, err = 0, ret = EXIT_FAILURE;
+
+	opts = malloc(sizeof(struct options));
+	if (!opts)
+		err_exit("%s: malloc(): %s\n", program_name, strerror(errno));
+
+	opts->range = NULL;
+	opts->verbose = 0;
+
+	if (argc > 1)
+		strncpy(opts->mpoint, argv[argc - 1], sizeof(opts->mpoint));
+
+	opts->range = calloc(1, sizeof(struct fstrim_range));
+	if (!opts->range) {
+		fprintf(stderr, "%s: calloc(): %s\n", program_name,
+			strerror(errno));
+		goto free_opts;
+	}
+
+	opts->range->len = ULLONG_MAX;
+
+	if (argc > 2)
+		err = parse_opts(argc, argv, opts);
+
+	if (err) {
+		usage();
+		goto free_opts;
+	}
+
+	if (strnlen(opts->mpoint, 1) < 1) {
+		fprintf(stderr, "%s: You have to specify mount point.\n",
+			program_name);
+		usage();
+		goto free_opts;
+	}
+
+	if (stat(opts->mpoint, &sb) == -1) {
+		fprintf(stderr, "%s: %s: %s\n", program_name,
+			opts->mpoint, strerror(errno));
+		usage();
+		goto free_opts;
+	}
+
+	if (!S_ISDIR(sb.st_mode)) {
+		fprintf(stderr, "%s: %s: (%s)\n", program_name,
+			opts->mpoint, strerror(ENOTDIR));
+		usage();
+		goto free_opts;
+	}
+
+	fd = open(opts->mpoint, O_RDONLY);
+	if (fd < 0) {
+		fprintf(stderr, "%s: open(%s): %s\n", program_name,
+			opts->mpoint, strerror(errno));
+		goto free_opts;
+	}
+
+	if (ioctl(fd, FITRIM, opts->range)) {
+		fprintf(stderr, "%s: FSTRIM: %s\n", program_name,
+			strerror(errno));
+		goto free_opts;
+	}
+
+	if ((opts->verbose) && (opts->range))
+		fprintf(stdout, "%llu Bytes were trimmed\n", (unsigned long long)opts->range->len);
+
+	ret = EXIT_SUCCESS;
+
+free_opts:
+	if (opts) {
+		if (opts->range)
+			free(opts->range);
+		free(opts);
+	}
+
+	return ret;
+}
-- 
1.7.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3] Add test 249: Check filesystem FITRIM implementation

[Alex Elder]

On Thu, 2011-02-03 at 20:43 +0100, Lukas Czerner wrote:
> FITRIM ioctl  is used on a mounted filesystem to discard (or "trim")
> blocks which are not in use by the filesystem.  This is useful for
> solid-state drives (SSDs) and thinly-provi-sioned storage. This test
> helps to verify filesystem FITRIM implementation to assure that it
> does not corrupts data.
> 
> This test creates checksums of all files in xfstests directory and
> run several processes which clear its working directory on SCRATCH_MNT,
> then copy everything from xfstests into its working directory, create
> list of files in working directory and its checksums and compare it with the
> original list of checksums. Every process works in the loop so it repeat
> remove->copy->check, while fstrim tool is running simultaneously.
> 
> Fstrim is just a helper tool which uses FITRIM ioctl to actually do the
> filesystem discard.
> 
> I found this very useful because when the FITRIM is really buggy (thus
> data-destroying) the 249 test will notice, because checksums will most
> likely change.

This sounds like a good test.  I ran it and it failed, but I
couldn't really tell why.  Turns out the ioctl() returned
ENOTSUP, which is fine.  But the test shouldn't work quite
that way.

Based on this very small experience, I have a few comments,
below.  I glanced at the C code also, and have a few suggestions
but they're not that important.

					-Alex

> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  249          |  170 ++++++++++++++++++++++++++++++++++++++
>  249.out      |    3 +
>  group        |    3 +-
>  src/Makefile |    2 +-
>  src/fstrim.c |  257 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 433 insertions(+), 2 deletions(-)
>  create mode 100644 249
>  create mode 100644 249.out
>  create mode 100644 src/fstrim.c
> 
> diff --git a/249 b/249
> new file mode 100644
> index 0000000..9943176
> --- /dev/null
> +++ b/249
> @@ -0,0 +1,170 @@
> +#!/bin/bash
> +# FS QA Test No. 248

Unfortunately for you, you'll need to change the number yet again.
(When the time comes to commit it I'll update the number for you
if needed.)  I made it number 252 for my own testing.  Remember
to change the name as well as the content of the test and its
output file.

> +#
> +# This test was created in order to verify filesystem FITRIM implementation.
> +# By many concurrent copy and remove operations and checking that files
> +# does not change after copied into SCRATCH_MNT test if FITRIM implementation
> +# corrupts the filesystem (data/metadata).
> +#

. . .

> +
> +function run_process() {
> +	local p=$1
> +	repeat=10
> +
> +	sleep $((5*$p))s &
> +	export chpid=$! && wait $chpid &> /dev/null
> +	chpid=0
> +
> +	while [ $repeat -gt 0 ]; do
> +
> +		# Remove old directories.
> +		rm -rf $SCRATCH_MNT/$p
> +		export chpid=$! && wait $chpid &> /dev/null
> +
> +		# Copy content -> partition.
> +		mkdir $SCRATCH_MNT/$p
> +		cp -axT $content $SCRATCH_MNT/$p
> +		export chpid=$! && wait $chpid &> /dev/null
> +
> +		check_sums
> +		repeat=$(( $repeat - 1 ))
> +	done
> +}
> +
> +nproc=20
> +content=$here
> +

Here (below), you are checking for support and including
that check in the test itself.  Instead, you should check
for support and if support isn't found, call something like:

_notrun "fstrim support not available"

Even better, you should encapsulate the check for support
in a shell function, so the call would look like:

_check_fstrim_support || _notrun "fstrim support not available"

I don't think there's a need for a "common.fstrim" file,
but you can look at some other examples (like filestreams)
for examples of this pattern.

> +# Check for FITRIM support
> +echo -n "Checking FITRIM support: "
> +$here/src/fstrim -l 10M $SCRATCH_MNT &> /dev/null

Redirecting both stdout and stderr is very unhelpful
for diagnosing why the command failed.

I also think you should have an explicit command line
option to the src/fstrim command to check for support,
silently, rather than specifying a length to trim.
(If it had been supported in my case, would this have
actually done the TRIM?  That is not desirable in a
feature check.)
 
> +[ $? -ne 0 ] && exit
> +echo "done."
> +
> +mkdir -p $tmp
. . .

>  
> diff --git a/src/fstrim.c b/src/fstrim.c
> new file mode 100644
> index 0000000..a93a6f3
> --- /dev/null
> +++ b/src/fstrim.c
> @@ -0,0 +1,257 @@
> +/*
> + * fstrim.c -- discard the part (or whole) of mounted filesystem.
> + *
> + * Copyright (C) 2009 Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>

Is this the right copyright date?  I don't care, just thought
I'd call this to your attention in case it's wrong.

. . .

> +
> +int main(int argc, char **argv)
> +{
> +	struct options *opts;
> +	struct stat sb;
> +	int fd, err = 0, ret = EXIT_FAILURE;
> +
> +	opts = malloc(sizeof(struct options));

No real need to malloc the options structure here, just define
it as a struct rather than pointer and avoid this failure.

> +	if (!opts)
> +		err_exit("%s: malloc(): %s\n", program_name, strerror(errno));
> +
> +	opts->range = NULL;
> +	opts->verbose = 0;
> +
> +	if (argc > 1)
> +		strncpy(opts->mpoint, argv[argc - 1], sizeof(opts->mpoint));
> +
> +	opts->range = calloc(1, sizeof(struct fstrim_range));

Same thing here--just make the range be a struct component
of the options structure and you won't have to allocate
it (and will avoid the need to check for and handle the
failure).

> +	if (!opts->range) {
> +		fprintf(stderr, "%s: calloc(): %s\n", program_name,
> +			strerror(errno));
> +		goto free_opts;
> +	}
> +
> +	opts->range->len = ULLONG_MAX;
> +
> +	if (argc > 2)
> +		err = parse_opts(argc, argv, opts);
> +
> +	if (err) {
> +		usage();
> +		goto free_opts;
> +	}
> +

This is more of a style comment...  I think I prefer
seeing both initializing default values (such as how
you set opts->range->len) and checking for their
validity after they've been parsed inside the argument
parsing routine itself.  That leaves the top-level
code simpler--just parse the options and go.

> +	if (strnlen(opts->mpoint, 1) < 1) {
> +		fprintf(stderr, "%s: You have to specify mount point.\n",
> +			program_name);
> +		usage();
> +		goto free_opts;
> +	}

. . .

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3] Add test 249: Check filesystem FITRIM implementation

[Lukas Czerner]

On Tue, 8 Feb 2011, Alex Elder wrote:

> On Thu, 2011-02-03 at 20:43 +0100, Lukas Czerner wrote:
> > FITRIM ioctl  is used on a mounted filesystem to discard (or "trim")
> > blocks which are not in use by the filesystem.  This is useful for
> > solid-state drives (SSDs) and thinly-provi-sioned storage. This test
> > helps to verify filesystem FITRIM implementation to assure that it
> > does not corrupts data.
> > 
> > This test creates checksums of all files in xfstests directory and
> > run several processes which clear its working directory on SCRATCH_MNT,
> > then copy everything from xfstests into its working directory, create
> > list of files in working directory and its checksums and compare it with the
> > original list of checksums. Every process works in the loop so it repeat
> > remove->copy->check, while fstrim tool is running simultaneously.
> > 
> > Fstrim is just a helper tool which uses FITRIM ioctl to actually do the
> > filesystem discard.
> > 
> > I found this very useful because when the FITRIM is really buggy (thus
> > data-destroying) the 249 test will notice, because checksums will most
> > likely change.
> 
> This sounds like a good test.  I ran it and it failed, but I
> couldn't really tell why.  Turns out the ioctl() returned
> ENOTSUP, which is fine.  But the test shouldn't work quite
> that way.
> 
> Based on this very small experience, I have a few comments,
> below.  I glanced at the C code also, and have a few suggestions
> but they're not that important.
> 
> 					-Alex
> 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> >  249          |  170 ++++++++++++++++++++++++++++++++++++++
> >  249.out      |    3 +
> >  group        |    3 +-
> >  src/Makefile |    2 +-
> >  src/fstrim.c |  257 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 433 insertions(+), 2 deletions(-)
> >  create mode 100644 249
> >  create mode 100644 249.out
> >  create mode 100644 src/fstrim.c
> > 
> > diff --git a/249 b/249
> > new file mode 100644
> > index 0000000..9943176
> > --- /dev/null
> > +++ b/249
> > @@ -0,0 +1,170 @@
> > +#!/bin/bash
> > +# FS QA Test No. 248
> 
> Unfortunately for you, you'll need to change the number yet again.
> (When the time comes to commit it I'll update the number for you
> if needed.)  I made it number 252 for my own testing.  Remember
> to change the name as well as the content of the test and its
> output file.

Oh, I have reposted this so many times :) I always forget some stupid
little thing. Thanks.

> 
> > +#
> > +# This test was created in order to verify filesystem FITRIM implementation.
> > +# By many concurrent copy and remove operations and checking that files
> > +# does not change after copied into SCRATCH_MNT test if FITRIM implementation
> > +# corrupts the filesystem (data/metadata).
> > +#
> 
> . . .
> 
> > +
> > +function run_process() {
> > +	local p=$1
> > +	repeat=10
> > +
> > +	sleep $((5*$p))s &
> > +	export chpid=$! && wait $chpid &> /dev/null
> > +	chpid=0
> > +
> > +	while [ $repeat -gt 0 ]; do
> > +
> > +		# Remove old directories.
> > +		rm -rf $SCRATCH_MNT/$p
> > +		export chpid=$! && wait $chpid &> /dev/null
> > +
> > +		# Copy content -> partition.
> > +		mkdir $SCRATCH_MNT/$p
> > +		cp -axT $content $SCRATCH_MNT/$p
> > +		export chpid=$! && wait $chpid &> /dev/null
> > +
> > +		check_sums
> > +		repeat=$(( $repeat - 1 ))
> > +	done
> > +}
> > +
> > +nproc=20
> > +content=$here
> > +
> 
> Here (below), you are checking for support and including
> that check in the test itself.  Instead, you should check
> for support and if support isn't found, call something like:
> 
> _notrun "fstrim support not available"
> 
> Even better, you should encapsulate the check for support
> in a shell function, so the call would look like:
> 
> _check_fstrim_support || _notrun "fstrim support not available"

Right, sounds good.

> 
> I don't think there's a need for a "common.fstrim" file,
> but you can look at some other examples (like filestreams)
> for examples of this pattern.
> 
> > +# Check for FITRIM support
> > +echo -n "Checking FITRIM support: "
> > +$here/src/fstrim -l 10M $SCRATCH_MNT &> /dev/null
> 
> Redirecting both stdout and stderr is very unhelpful
> for diagnosing why the command failed.
> 
> I also think you should have an explicit command line
> option to the src/fstrim command to check for support,
> silently, rather than specifying a length to trim.
> (If it had been supported in my case, would this have
> actually done the TRIM?  That is not desirable in a
> feature check.)

Hmm, I do not think it is really needed. Calling fstrim on the part of the
SCRATCH_MNT is just fine. I do not see any problem here, if it is
not supported nothing will happen, but if it is supported part of the
filesystem will be TRIM'med, but that is going to happen anyway since this is
the intention of this test.

>  
> > +[ $? -ne 0 ] && exit
> > +echo "done."
> > +
> > +mkdir -p $tmp
> . . .
> 
> >  
> > diff --git a/src/fstrim.c b/src/fstrim.c
> > new file mode 100644
> > index 0000000..a93a6f3
> > --- /dev/null
> > +++ b/src/fstrim.c
> > @@ -0,0 +1,257 @@
> > +/*
> > + * fstrim.c -- discard the part (or whole) of mounted filesystem.
> > + *
> > + * Copyright (C) 2009 Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
> 
> Is this the right copyright date?  I don't care, just thought
> I'd call this to your attention in case it's wrong.
> 
> . . .
> 
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	struct options *opts;
> > +	struct stat sb;
> > +	int fd, err = 0, ret = EXIT_FAILURE;
> > +
> > +	opts = malloc(sizeof(struct options));
> 
> No real need to malloc the options structure here, just define
> it as a struct rather than pointer and avoid this failure.

Yeah, I do not really know why I did it with malloc :), however I am not
going to change it since it does not matter at all. If the malloc shall
fail, you would not want to run xfstests anyway, but rather deal with
"what is going on":).

> 
> > +	if (!opts)
> > +		err_exit("%s: malloc(): %s\n", program_name, strerror(errno));
> > +
> > +	opts->range = NULL;
> > +	opts->verbose = 0;
> > +
> > +	if (argc > 1)
> > +		strncpy(opts->mpoint, argv[argc - 1], sizeof(opts->mpoint));
> > +
> > +	opts->range = calloc(1, sizeof(struct fstrim_range));
> 
> Same thing here--just make the range be a struct component
> of the options structure and you won't have to allocate
> it (and will avoid the need to check for and handle the
> failure).
> 
> > +	if (!opts->range) {
> > +		fprintf(stderr, "%s: calloc(): %s\n", program_name,
> > +			strerror(errno));
> > +		goto free_opts;
> > +	}
> > +
> > +	opts->range->len = ULLONG_MAX;
> > +
> > +	if (argc > 2)
> > +		err = parse_opts(argc, argv, opts);
> > +
> > +	if (err) {
> > +		usage();
> > +		goto free_opts;
> > +	}
> > +
> 
> This is more of a style comment...  I think I prefer
> seeing both initializing default values (such as how
> you set opts->range->len) and checking for their
> validity after they've been parsed inside the argument
> parsing routine itself.  That leaves the top-level
> code simpler--just parse the options and go.
> 
> > +	if (strnlen(opts->mpoint, 1) < 1) {
> > +		fprintf(stderr, "%s: You have to specify mount point.\n",
> > +			program_name);
> > +		usage();
> > +		goto free_opts;
> > +	}

Thanks for review, I'll repost it ASAP.

-Lukas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3] Add test 249: Check filesystem FITRIM implementation

[Lukas Czerner]

On Tue, 8 Feb 2011, Alex Elder wrote:

> On Thu, 2011-02-03 at 20:43 +0100, Lukas Czerner wrote:
> > FITRIM ioctl  is used on a mounted filesystem to discard (or "trim")
> > blocks which are not in use by the filesystem.  This is useful for
> > solid-state drives (SSDs) and thinly-provi-sioned storage. This test
> > helps to verify filesystem FITRIM implementation to assure that it
> > does not corrupts data.
> > 
> > This test creates checksums of all files in xfstests directory and
> > run several processes which clear its working directory on SCRATCH_MNT,
> > then copy everything from xfstests into its working directory, create
> > list of files in working directory and its checksums and compare it with the
> > original list of checksums. Every process works in the loop so it repeat
> > remove->copy->check, while fstrim tool is running simultaneously.
> > 
> > Fstrim is just a helper tool which uses FITRIM ioctl to actually do the
> > filesystem discard.
> > 
> > I found this very useful because when the FITRIM is really buggy (thus
> > data-destroying) the 249 test will notice, because checksums will most
> > likely change.
> 
> This sounds like a good test.  I ran it and it failed, but I
> couldn't really tell why.  Turns out the ioctl() returned
> ENOTSUP, which is fine.  But the test shouldn't work quite
> that way.
> 
> Based on this very small experience, I have a few comments,
> below.  I glanced at the C code also, and have a few suggestions
> but they're not that important.
> 
> 					-Alex
> 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> >  249          |  170 ++++++++++++++++++++++++++++++++++++++
> >  249.out      |    3 +
> >  group        |    3 +-
> >  src/Makefile |    2 +-
> >  src/fstrim.c |  257 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 433 insertions(+), 2 deletions(-)
> >  create mode 100644 249
> >  create mode 100644 249.out
> >  create mode 100644 src/fstrim.c
> > 
> > diff --git a/249 b/249
> > new file mode 100644
> > index 0000000..9943176
> > --- /dev/null
> > +++ b/249
> > @@ -0,0 +1,170 @@
> > +#!/bin/bash
> > +# FS QA Test No. 248
> 
> Unfortunately for you, you'll need to change the number yet again.
> (When the time comes to commit it I'll update the number for you
> if needed.)  I made it number 252 for my own testing.  Remember
> to change the name as well as the content of the test and its
> output file.
> 

One more thing though. Where did you get the number 252 ? This patch
still apply cleanly on the top of the master branch. I am using this
repo: 

git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git

Shall I use a different one ? Which one is the most up-to-date ?

Thanks!
-Lukas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3] Add test 249: Check filesystem FITRIM implementation

[Alex Elder]

On Wed, 2011-02-09 at 13:13 +0100, Lukas Czerner wrote:
> 
> One more thing though. Where did you get the number 252 ? This patch
> still apply cleanly on the top of the master branch. I am using this
> repo: 
> 
> git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
> 
> Shall I use a different one ? Which one is the most up-to-date ? 

Like I said, use what works for you, and when the time
comes to commit it I will update all the numbers for you
to match what's current.  I've been testing with some
other tests not yet committed.

					-Alex

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs