Re: [PATCH 2/4] xfstests: fsx fallocate support is b0rked

[Eric Sandeen <sandeen@sandeen.net>]

On 6/27/11 12:48 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The recent fallocate/fpunch additions to fsx have not actually be
> executing fallocate/fpunch operations. The logic to select what
> operation to run is broken in such a way that fsx has been executing
> mapped writes and truncates instead of fallocate and fpunch
> operations.
> 
> Remove all the (b0rken) smarty-pants selection logic from the test()

I hope I only extended that smarty-pants logic and didn't invent it.
I suppose maybe I broke it first though, damn.

> function. Replace it with a clearly defined set of operations for
> each mode and use understandable fallback logic when various
> operation types have been disabled. Then use a simple switch
> statement to execute each of the different operations, removing the
> tortured nesting of if/else statements that only serve to obfuscate
> the code.
> 
> NAs a result, fsx uses fallocate/fpunch appropriately during
> operations, and uses/disableѕ the operations as defined on the
> command line correctly.

Hm we're back in the fsx maintenance business I guess.

Would it be worth doing defines or an enum for the ops/cases, to make it
a little more readable?

Otherwise looks much better.

-Eric

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  ltp/fsx.c |  162 +++++++++++++++++++++++++++++++++++++++----------------------
>  1 files changed, 103 insertions(+), 59 deletions(-)
> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index a37e223..66daefe 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -955,18 +955,42 @@ docloseopen(void)
>  	}
>  }
>  
> +#define TRIM_OFF_LEN(off, len, size)	\
> +do {					\
> +	if (file_size)			\
> +		offset %= size;		\
> +	else				\
> +		offset = 0;		\
> +	if (offset + len > size)	\
> +		len = size - offset;	\
> +} while (0)
>  
> +/*
> + * The operation matrix is complex due to conditional variables.
> + * We calculate how many different operations we can use, then
> + * map them appropriately according to how many options are enabled.
> + * The mapping is:
> + *
> + *		lite	!lite
> + * READ:	0	0
> + * WRITE:	1	1
> + * MAPREAD:	2	2
> + * MAPWRITE:	3	3
> + * TRUNCATE:	-	4
> + * FALLOCATE:	-	5
> + * PUNCH HOLE:	-	6
> + *
> + * When mapped read/writes are disabled, they are simply converted to normal
> + * reads and writes. When fallocate/fpunch calls are disabled, they are
> + * converted to OP_SKIPPED.
> + */
>  void
>  test(void)
>  {
>  	unsigned long	offset;
>  	unsigned long	size = maxoplen;
>  	unsigned long	rv = random();
> -	unsigned long	op = rv % (3 + !lite + mapped_writes + fallocate_calls + punch_hole_calls);
> -        /* turn off the map read if necessary */
> -
> -        if (op == 2 && !mapped_reads)
> -            op = 0;
> +	unsigned long	op;
>  
>  	if (simulatedopcount > 0 && testcalls == simulatedopcount)
>  		writefileimage();
> @@ -982,62 +1006,82 @@ test(void)
>  	if (!quiet && testcalls < simulatedopcount && testcalls % 100000 == 0)
>  		prt("%lu...\n", testcalls);
>  
> -	/*
> -	 *                 lite  !lite
> -	 * READ:	op = 0	   0
> -	 * WRITE:	op = 1     1
> -	 * MAPREAD:     op = 2     2
> -	 * TRUNCATE:	op = -     3
> -	 * MAPWRITE:    op = 3     4
> -	 * FALLOCATE:   op = -     5
> -	 * PUNCH HOLE:  op = -     6
> -	 */
> -	if (lite ? 0 : op == 3 && (style & 1) == 0) /* vanilla truncate? */
> -		dotruncate(random() % maxfilelen);
> -	else {
> -		if (randomoplen)
> -			size = random() % (maxoplen+1);
> -
> -		if (lite ? 0 : op == 3) {
> -			/* truncate */
> -			dotruncate(size);
> -		} else {
> -			offset = random();
> -			if (op == 5) {
> -				/* fallocate */
> -				offset %= maxfilelen;
> -				if (offset + size > maxfilelen)
> -					size = maxfilelen - offset;
> -				do_preallocate(offset, size);
> -			} else if (op == 6) {
> -				offset %= maxfilelen;
> -				if (offset + size > maxfilelen)
> -					size = maxfilelen - offset;
> -				do_punch_hole(offset, size);
> -			} else if (op == 1 || op == (lite ? 3 : 4)) {
> -				/* write / mapwrite */
> -				offset %= maxfilelen;
> -				if (offset + size > maxfilelen)
> -					size = maxfilelen - offset;
> -				if (op != 1)
> -					domapwrite(offset, size);
> -				else
> -					dowrite(offset, size);
> -			} else {
> -				/* read / mapread */
> -				if (file_size)
> -					offset %= file_size;
> -				else
> -					offset = 0;
> -				if (offset + size > file_size)
> -					size = file_size - offset;
> -				if (op != 0)
> -					domapread(offset, size);
> -				else
> -					doread(offset, size);
> -			}
> +	offset = random();
> +	if (randomoplen)
> +		size = random() % (maxoplen + 1);
> +
> +	/* calculate appropriate op to run */
> +	if (lite)
> +		op = rv % 4;
> +	else
> +		op = rv % 7;
> +
> +	switch (op) {
> +	case 2:
> +		if (!mapped_reads)
> +			op = 0;
> +		break;
> +	case 3:
> +		if (!mapped_writes)
> +			op = 1;
> +		break;
> +	case 5:
> +		if (!fallocate_calls) {
> +			log4(OP_SKIPPED, OP_FALLOCATE, offset, size);
> +			goto out;
> +		}
> +		break;
> +	case 6:
> +		if (!punch_hole_calls) {
> +			log4(OP_SKIPPED, OP_PUNCH_HOLE, offset, size);
> +			goto out;
>  		}
> +		break;
>  	}
> +
> +	switch (op) {
> +	case 0:	/* read */
> +		TRIM_OFF_LEN(offset, size, file_size);
> +		doread(offset, size);
> +		break;
> +
> +	case 1: /* write */
> +		TRIM_OFF_LEN(offset, size, maxfilelen);
> +		dowrite(offset, size);
> +		break;
> +
> +	case 2:	/* mapread */
> +		TRIM_OFF_LEN(offset, size, maxfilelen);
> +		domapread(offset, size);
> +		break;
> +
> +	case 3:	/* mapwrite */
> +		TRIM_OFF_LEN(offset, size, maxfilelen);
> +		domapwrite(offset, size);
> +		break;
> +
> +	case 4: /* truncate */
> +		if (!style)
> +			size = random() % maxfilelen;
> +		dotruncate(size);
> +		break;
> +
> +	case 5:	/* fallocate */
> +		TRIM_OFF_LEN(offset, size, maxfilelen);
> +		do_preallocate(offset, size);
> +		break;
> +
> +	case 6:	/* punch */
> +		TRIM_OFF_LEN(offset, size, maxfilelen);
> +		do_punch_hole(offset, size);
> +		break;
> +	default:
> +		prterr("test: unknown operation");
> +		report_failure(42);
> +		break;
> +	}
> +
> +out:
>  	if (sizechecks && testcalls > simulatedopcount)
>  		check_size();
>  	if (closeopen)

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