[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 at 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 at 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;
> +	}

. . .