[PATCH 0/4, V2] xfstests: fsx is fallocate challenged

[PATCH 0/4, V2] xfstests: fsx is fallocate challenged

[Dave Chinner]

This is a repost of the fsx fallocate/fpunch operation handling
fixes along with additional tests added to 091 to cover the failing
test cases that exposed the fallocate/fpunch operation handling
problems.

Version 2 fixes the review coments of the first version. The main
change is to re-number all the operations and use those directly to
generate the operations to execute.

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

[PATCH 1/4] xfstests: fix fsx fpunch test to actually test for fpunch

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The operation flags parameter to fallocate is the second parameter,
not the last. Hence the fpunch test is actually testing for falloc
support, not fpunch. Somebody needs a brown paper bag.

Also, add a ftruncate call whenthe fpunch succeeds just in case the
file was not already zero sized. Failing to ensure we start with a
zero length file can cause read ops to fail size checks if they
occur before the file is written to be the main test loop.

While there, observe the quiet flag the same as the falloc test
does and have them both emit the warning at the same error level.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 ltp/fsx.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index 0683853..a37e223 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -1243,7 +1243,7 @@ test_fallocate()
 	if (!lite && fallocate_calls) {
 		if (fallocate(fd, 0, 0, 1) && errno == EOPNOTSUPP) {
 			if(!quiet)
-				prt("fsx: main: filesystem does not support fallocate, disabling\n");
+				warn("main: filesystem does not support fallocate, disabling\n");
 			fallocate_calls = 0;
 		} else {
 			ftruncate(fd, 0);
@@ -1260,13 +1260,13 @@ test_punch_hole()
 {
 #ifdef FALLOC_FL_PUNCH_HOLE
 	if (!lite && punch_hole_calls) {
-		if (fallocate(fd, 0, 0,
-			FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE) &&
-			errno == EOPNOTSUPP) {
-
-			warn("main: filesystem does not support fallocate punch hole, disabling");
+		if (fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+				0, 1) && errno == EOPNOTSUPP) {
+			if(!quiet)
+				warn("main: filesystem does not support fallocate punch hole, disabling");
 			punch_hole_calls = 0;
-		}
+		} else
+			ftruncate(fd, 0);
 	}
 #else /* ! PUNCH HOLE */
 	punch_hole_calls = 0;
-- 
1.7.5.1

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

Re: [PATCH 1/4] xfstests: fix fsx fpunch test to actually test for fpunch

[Alex Elder]

On Fri, 2011-07-08 at 10:53 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The operation flags parameter to fallocate is the second parameter,
> not the last. Hence the fpunch test is actually testing for falloc
> support, not fpunch. Somebody needs a brown paper bag.
> 
> Also, add a ftruncate call whenthe fpunch succeeds just in case the
> file was not already zero sized. Failing to ensure we start with a
> zero length file can cause read ops to fail size checks if they
> occur before the file is written to be the main test loop.
> 
> While there, observe the quiet flag the same as the falloc test
> does and have them both emit the warning at the same error level.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good.  Even if the arguments were in the
right order, the length has to be greater than
zero also.

Reviewed-by: Alex Elder <aelder@sgi.com>



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

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

[Dave Chinner]

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()
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.

As a result, fsx uses fallocate/fpunch appropriately during
operations and uses/disableѕ the operations as defined on the
command line correctly.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 ltp/fsx.c |  192 +++++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 123 insertions(+), 69 deletions(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index a37e223..41d7c20 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -58,18 +58,47 @@ int			logptr = 0;	/* current position in log */
 int			logcount = 0;	/* total ops */
 
 /*
- *	Define operations
+ * The operation matrix is complex due to conditional execution of different
+ * features. Hence when we come to deciding what operation to run, we need to
+ * be careful in how we select the different operations. The active operations
+ * are mapped to numbers as follows:
+ *
+ *		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. Hence OP_SKIPPED needs to have a number higher than
+ * the operation selction matrix, as does the OP_CLOSEOPEN which is an
+ * operation modifier rather than an operation in itself.
+ *
+ * Because of the "lite" version, we also need to have different "maximum
+ * operation" defines to allow the ops to be selected correctly based on the
+ * mode being run.
  */
 
-#define	OP_READ		1
-#define OP_WRITE	2
-#define OP_TRUNCATE	3
-#define OP_CLOSEOPEN	4
-#define OP_MAPREAD	5
-#define OP_MAPWRITE	6
-#define OP_SKIPPED	7
-#define OP_FALLOCATE	8
-#define OP_PUNCH_HOLE	9
+/* common operations */
+#define	OP_READ		0
+#define OP_WRITE	1
+#define OP_MAPREAD	2
+#define OP_MAPWRITE	3
+#define OP_MAX_LITE	4
+
+/* !lite operations */
+#define OP_TRUNCATE	4
+#define OP_FALLOCATE	5
+#define OP_PUNCH_HOLE	6
+#define OP_MAX_FULL	7
+
+/* operation modifiers */
+#define OP_CLOSEOPEN	100
+#define OP_SKIPPED	101
 
 #undef PAGE_SIZE
 #define PAGE_SIZE       getpagesize()
@@ -955,6 +984,15 @@ 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)
 
 void
 test(void)
@@ -962,11 +1000,7 @@ 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 +1016,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 % OP_MAX_LITE;
+	else
+		op = rv % OP_MAX_FULL;
+
+	switch (op) {
+	case OP_MAPREAD:
+		if (!mapped_reads)
+			op = OP_READ;
+		break;
+	case OP_MAPWRITE:
+		if (!mapped_writes)
+			op = OP_WRITE;
+		break;
+	case OP_FALLOCATE:
+		if (!fallocate_calls) {
+			log4(OP_SKIPPED, OP_FALLOCATE, offset, size);
+			goto out;
+		}
+		break;
+	case OP_PUNCH_HOLE:
+		if (!punch_hole_calls) {
+			log4(OP_SKIPPED, OP_PUNCH_HOLE, offset, size);
+			goto out;
 		}
+		break;
 	}
+
+	switch (op) {
+	case OP_READ:
+		TRIM_OFF_LEN(offset, size, file_size);
+		doread(offset, size);
+		break;
+
+	case OP_WRITE:
+		TRIM_OFF_LEN(offset, size, maxfilelen);
+		dowrite(offset, size);
+		break;
+
+	case OP_MAPREAD:
+		TRIM_OFF_LEN(offset, size, file_size);
+		domapread(offset, size);
+		break;
+
+	case OP_MAPWRITE:
+		TRIM_OFF_LEN(offset, size, maxfilelen);
+		domapwrite(offset, size);
+		break;
+
+	case OP_TRUNCATE:
+		if (!style)
+			size = random() % maxfilelen;
+		dotruncate(size);
+		break;
+
+	case OP_FALLOCATE:
+		TRIM_OFF_LEN(offset, size, maxfilelen);
+		do_preallocate(offset, size);
+		break;
+
+	case OP_PUNCH_HOLE:
+		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)
-- 
1.7.5.1

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

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

[Alex Elder]

On Fri, 2011-07-08 at 10:53 +1000, 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()
> 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.
> 
> As a result, fsx uses fallocate/fpunch appropriately during
> operations and uses/disableѕ the operations as defined on the
> command line correctly.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

I started trying to understand the logic of the original in order
to make sure your new version preserved the logic.  But then I
concluded it was just plain broken, and understood exactly the
point of this change...

So I just looked at your new code.  I have a few minor things
but it otherwise looks good to me.  The way "closeopen" is
(still) computed is a bit funked up (and possibly just wrong),
but I'm not going to complain--you've done a lot of good here.

Reviewed-by: Alex Elder <aelder@sgi.com>

> ---
>  ltp/fsx.c |  192 +++++++++++++++++++++++++++++++++++++++----------------------
>  1 files changed, 123 insertions(+), 69 deletions(-)
> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index a37e223..41d7c20 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -58,18 +58,47 @@ int			logptr = 0;	/* current position in log */
>  int			logcount = 0;	/* total ops */
>  
>  /*
> - *	Define operations
> + * The operation matrix is complex due to conditional execution of different
> + * features. Hence when we come to deciding what operation to run, we need to
> + * be careful in how we select the different operations. The active operations
> + * are mapped to numbers as follows:
> + *
> + *		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. Hence OP_SKIPPED needs to have a number higher than

The "hence" is not obvious.  The reason it needs to have a
higher number is that the operation is selected at random
among the number of operations available (either in "lite"
or in "full" mode).

> + * the operation selction matrix, as does the OP_CLOSEOPEN which is an

                    selection

> + * operation modifier rather than an operation in itself.
> + *
> + * Because of the "lite" version, we also need to have different "maximum
> + * operation" defines to allow the ops to be selected correctly based on the
> + * mode being run.
>   */
>  
> -#define	OP_READ		1
> -#define OP_WRITE	2
> -#define OP_TRUNCATE	3
> -#define OP_CLOSEOPEN	4
> -#define OP_MAPREAD	5
> -#define OP_MAPWRITE	6
> -#define OP_SKIPPED	7
> -#define OP_FALLOCATE	8
> -#define OP_PUNCH_HOLE	9
> +/* common operations */
> +#define	OP_READ		0
> +#define OP_WRITE	1
> +#define OP_MAPREAD	2
> +#define OP_MAPWRITE	3
> +#define OP_MAX_LITE	4

To me, "max" suggests that it is an included value
in the range.  So I'd rather see this be "OP_NUM_LITE"
or (my preference) "OP_LITE_COUNT".  Similarly for
OP_MAX_FULL below.

> +
> +/* !lite operations */
> +#define OP_TRUNCATE	4
> +#define OP_FALLOCATE	5
> +#define OP_PUNCH_HOLE	6
> +#define OP_MAX_FULL	7
> +
> +/* operation modifiers */
> +#define OP_CLOSEOPEN	100
> +#define OP_SKIPPED	101
>  
>  #undef PAGE_SIZE
>  #define PAGE_SIZE       getpagesize()
> @@ -955,6 +984,15 @@ 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)

This macro is a very good idea.  However it differs
from the original behavior when used for OP_WRITE,
OP_MAPWRITE, OP_FALLOCATE, and OP_PUNCH_HOLE in
that if file_size is zero, offset will be set to
zero.  It seems like that behavior difference is
significant, but I don't think it has a practical
effect on the results of the test.  I've left the
code in question below for reference.

>  void
>  test(void)
> @@ -962,11 +1000,7 @@ 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 +1016,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 % OP_MAX_LITE;
> +	else
> +		op = rv % OP_MAX_FULL;
> +
> +	switch (op) {
> +	case OP_MAPREAD:
> +		if (!mapped_reads)
> +			op = OP_READ;
> +		break;
> +	case OP_MAPWRITE:
> +		if (!mapped_writes)
> +			op = OP_WRITE;
> +		break;
> +	case OP_FALLOCATE:
> +		if (!fallocate_calls) {
> +			log4(OP_SKIPPED, OP_FALLOCATE, offset, size);
> +			goto out;
> +		}
> +		break;
> +	case OP_PUNCH_HOLE:
> +		if (!punch_hole_calls) {
> +			log4(OP_SKIPPED, OP_PUNCH_HOLE, offset, size);
> +			goto out;
>  		}
> +		break;
>  	}
> +
> +	switch (op) {
> +	case OP_READ:
> +		TRIM_OFF_LEN(offset, size, file_size);
> +		doread(offset, size);
> +		break;
> +
> +	case OP_WRITE:
> +		TRIM_OFF_LEN(offset, size, maxfilelen);
> +		dowrite(offset, size);
> +		break;
> +
> +	case OP_MAPREAD:
> +		TRIM_OFF_LEN(offset, size, file_size);
> +		domapread(offset, size);
> +		break;
> +
> +	case OP_MAPWRITE:
> +		TRIM_OFF_LEN(offset, size, maxfilelen);
> +		domapwrite(offset, size);
> +		break;
> +
> +	case OP_TRUNCATE:
> +		if (!style)
> +			size = random() % maxfilelen;
> +		dotruncate(size);
> +		break;
> +
> +	case OP_FALLOCATE:
> +		TRIM_OFF_LEN(offset, size, maxfilelen);
> +		do_preallocate(offset, size);
> +		break;
> +
> +	case OP_PUNCH_HOLE:
> +		TRIM_OFF_LEN(offset, size, maxfilelen);
> +		do_punch_hole(offset, size);
> +		break;
> +	default:
> +		prterr("test: unknown operation");

		prterr("test: unknown operation (%d)", op);

> +		report_failure(42);

#define	ULTIMATE_ANSWER
		report_failure(ULTIMATE_ANSWER);

> +		break;
> +	}
> +
> +out:
>  	if (sizechecks && testcalls > simulatedopcount)
>  		check_size();
>  	if (closeopen)



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

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

[Dave Chinner]

On Fri, Jul 08, 2011 at 01:58:51PM -0500, Alex Elder wrote:
> On Fri, 2011-07-08 at 10:53 +1000, 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()
> > 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.
> > 
> > As a result, fsx uses fallocate/fpunch appropriately during
> > operations and uses/disableѕ the operations as defined on the
> > command line correctly.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> I started trying to understand the logic of the original in order
> to make sure your new version preserved the logic.  But then I
> concluded it was just plain broken, and understood exactly the
> point of this change...
> 
> So I just looked at your new code.  I have a few minor things
> but it otherwise looks good to me.  The way "closeopen" is
> (still) computed is a bit funked up (and possibly just wrong),
> but I'm not going to complain--you've done a lot of good here.
> 
> Reviewed-by: Alex Elder <aelder@sgi.com>
> 
> > ---
> >  ltp/fsx.c |  192 +++++++++++++++++++++++++++++++++++++++----------------------
> >  1 files changed, 123 insertions(+), 69 deletions(-)
> > 
> > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > index a37e223..41d7c20 100644
> > --- a/ltp/fsx.c
> > +++ b/ltp/fsx.c
> > @@ -58,18 +58,47 @@ int			logptr = 0;	/* current position in log */
> >  int			logcount = 0;	/* total ops */
> >  
> >  /*
> > - *	Define operations
> > + * The operation matrix is complex due to conditional execution of different
> > + * features. Hence when we come to deciding what operation to run, we need to
> > + * be careful in how we select the different operations. The active operations
> > + * are mapped to numbers as follows:
> > + *
> > + *		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. Hence OP_SKIPPED needs to have a number higher than
> 
> The "hence" is not obvious.  The reason it needs to have a
> higher number is that the operation is selected at random
> among the number of operations available (either in "lite"
> or in "full" mode).

I'll reword it so it is obvious ;)

> 
> > + * the operation selction matrix, as does the OP_CLOSEOPEN which is an
> 
>                     selection
> 
> > + * operation modifier rather than an operation in itself.
> > + *
> > + * Because of the "lite" version, we also need to have different "maximum
> > + * operation" defines to allow the ops to be selected correctly based on the
> > + * mode being run.
> >   */
> >  
> > -#define	OP_READ		1
> > -#define OP_WRITE	2
> > -#define OP_TRUNCATE	3
> > -#define OP_CLOSEOPEN	4
> > -#define OP_MAPREAD	5
> > -#define OP_MAPWRITE	6
> > -#define OP_SKIPPED	7
> > -#define OP_FALLOCATE	8
> > -#define OP_PUNCH_HOLE	9
> > +/* common operations */
> > +#define	OP_READ		0
> > +#define OP_WRITE	1
> > +#define OP_MAPREAD	2
> > +#define OP_MAPWRITE	3
> > +#define OP_MAX_LITE	4
> 
> To me, "max" suggests that it is an included value
> in the range.  So I'd rather see this be "OP_NUM_LITE"
> or (my preference) "OP_LITE_COUNT".  Similarly for
> OP_MAX_FULL below.

Ok, makes sense.

> 
> > +
> > +/* !lite operations */
> > +#define OP_TRUNCATE	4
> > +#define OP_FALLOCATE	5
> > +#define OP_PUNCH_HOLE	6
> > +#define OP_MAX_FULL	7
> > +
> > +/* operation modifiers */
> > +#define OP_CLOSEOPEN	100
> > +#define OP_SKIPPED	101
> >  
> >  #undef PAGE_SIZE
> >  #define PAGE_SIZE       getpagesize()
> > @@ -955,6 +984,15 @@ 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)
> 
> This macro is a very good idea.  However it differs
> from the original behavior when used for OP_WRITE,
> OP_MAPWRITE, OP_FALLOCATE, and OP_PUNCH_HOLE in
> that if file_size is zero, offset will be set to
> zero.  It seems like that behavior difference is
> significant, but I don't think it has a practical
> effect on the results of the test.  I've left the
> code in question below for reference.

True, it is a slight change in behaviour for OP_WRITE and
OP_FALLOCATE. I'll modify the macro to do the same thing.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 3/4] xfstests: fix brain-o in fallocate log dump

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

fsx segvs when dumping fallocate log entries. Fix magic string
array index parameters to be zero based rather than one based.

While touching log string related stuff, make the format consistent
with read and write operations so the log dump is easier to look at
with the human eye.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 ltp/fsx.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index 41d7c20..62dea0b 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -252,14 +252,14 @@ logdump(void)
 		int opnum;
 
 		opnum = i+1 + (logcount/LOGSIZE)*LOGSIZE;
-		prt("%d(%d mod 256): ", opnum, opnum%256);
+		prt("%d(%3d mod 256): ", opnum, opnum%256);
 		lp = &oplog[i];
 		if ((closeopen = lp->operation < 0))
 			lp->operation = ~ lp->operation;
 			
 		switch (lp->operation) {
 		case OP_MAPREAD:
-			prt("MAPREAD\t0x%x thru 0x%x\t(0x%x bytes)",
+			prt("MAPREAD  0x%x thru 0x%x\t(0x%x bytes)",
 			    lp->args[0], lp->args[0] + lp->args[1] - 1,
 			    lp->args[1]);
 			if (badoff >= lp->args[0] && badoff <
@@ -275,7 +275,7 @@ logdump(void)
 				prt("\t******WWWW");
 			break;
 		case OP_READ:
-			prt("READ\t0x%x thru 0x%x\t(0x%x bytes)",
+			prt("READ     0x%x thru 0x%x\t(0x%x bytes)",
 			    lp->args[0], lp->args[0] + lp->args[1] - 1,
 			    lp->args[1]);
 			if (badoff >= lp->args[0] &&
@@ -283,7 +283,7 @@ logdump(void)
 				prt("\t***RRRR***");
 			break;
 		case OP_WRITE:
-			prt("WRITE\t0x%x thru 0x%x\t(0x%x bytes)",
+			prt("WRITE    0x%x thru 0x%x\t(0x%x bytes)",
 			    lp->args[0], lp->args[0] + lp->args[1] - 1,
 			    lp->args[1]);
 			if (lp->args[0] > lp->args[2])
@@ -304,14 +304,15 @@ logdump(void)
 			break;
 		case OP_FALLOCATE:
 			/* 0: offset 1: length 2: where alloced */
-			prt("FALLOCATE %s\tfrom 0x%x to 0x%x",
-			    falloc_type[lp->args[2]], lp->args[0], lp->args[0] + lp->args[1]);
+			prt("FALLOC   0x%x thru 0x%x\t(0x%x bytes) %s",
+				lp->args[0], lp->args[0] + lp->args[1],
+				lp->args[1], falloc_type[lp->args[2]]);
 			if (badoff >= lp->args[0] &&
 			    badoff < lp->args[0] + lp->args[1])
 				prt("\t******FFFF");
 			break;
 		case OP_PUNCH_HOLE:
-			prt("PUNCH HOLE\t0x%x thru 0x%x\t(0x%x bytes)",
+			prt("PUNCH    0x%x thru 0x%x\t(0x%x bytes)",
 			    lp->args[0], lp->args[0] + lp->args[1] - 1,
 			    lp->args[1]);
 			if (badoff >= lp->args[0] && badoff <
@@ -906,12 +907,12 @@ do_preallocate(unsigned offset, unsigned length)
 	}
 
 	/*
-	 * last arg:
-	 * 	1: allocate past EOF
-	 * 	2: extending prealloc
-	 * 	3: interior prealloc
+	 * last arg matches fallocate string array index in logdump:
+	 * 	0: allocate past EOF
+	 * 	1: extending prealloc
+	 * 	2: interior prealloc
 	 */
-	log4(OP_FALLOCATE, offset, length, (end_offset > file_size) ? (keep_size ? 1 : 2) : 3);
+	log4(OP_FALLOCATE, offset, length, (end_offset > file_size) ? (keep_size ? 0 : 1) : 2);
 
 	if (end_offset > file_size) {
 		memset(good_buf + file_size, '\0', end_offset - file_size);
@@ -924,7 +925,8 @@ do_preallocate(unsigned offset, unsigned length)
 	if ((progressinterval && testcalls % progressinterval == 0) ||
 	    (debug && (monitorstart == -1 || monitorend == -1 ||
 		      end_offset <= monitorend)))
-		prt("%lu falloc\tfrom 0x%x to 0x%x\n", testcalls, offset, length);
+		prt("%lu falloc\tfrom 0x%x to 0x%x (0x%x bytes)\n", testcalls,
+				offset, offset + length, length);
 	if (fallocate(fd, keep_size ? FALLOC_FL_KEEP_SIZE : 0, (loff_t)offset, (loff_t)length) == -1) {
 	        prt("fallocate: %x to %x\n", offset, length);
 		prterr("do_preallocate: fallocate");
-- 
1.7.5.1

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

Re: [PATCH 3/4] xfstests: fix brain-o in fallocate log dump

[Alex Elder]

On Fri, 2011-07-08 at 10:53 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> fsx segvs when dumping fallocate log entries. Fix magic string
> array index parameters to be zero based rather than one based.
> 
> While touching log string related stuff, make the format consistent
> with read and write operations so the log dump is easier to look at
> with the human eye.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>


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

[PATCH 4/4] xfstests: add mapped write fsx operations to 091

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The recent busted fsx updates caused fsx to execute fsx with direct
IO and mmapped reads and writes on an XFS filesystem. The result
uncovered a direct-IO write vs mmap read bug to do with EOF
sub-block zeroing on the direct IO write.

Hence whiel we do not recommend that pepole mix DIO with mmap on the
same file, we should at least have tests that exercise it as they
often show up other problems like this.


Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 091     |    3 +++
 091.out |    2 ++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/091 b/091
index a13d979..11b599e 100755
--- a/091
+++ b/091
@@ -88,6 +88,9 @@ kernel=`uname -r  | sed -e 's/\(2\..\).*/\1/'`
 #run_fsx -N 10000  -o 128000 -l 500000 -r PSIZE -t PSIZE -w PSIZE -Z -W
  run_fsx -N 10000  -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -W
 
+ run_fsx -N 10000  -o 8192   -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
+ run_fsx -N 10000  -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
+
 # Commented out calls above are less likely to pick up issues, so
 # save time by commenting them out (leave 'em for manual testing).
 
diff --git a/091.out b/091.out
index 31bd25d..27ed1e3 100644
--- a/091.out
+++ b/091.out
@@ -5,3 +5,5 @@ fsx -N 10000 -o 32768 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W
 fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W
 fsx -N 10000 -o 32768 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W
 fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -W
+fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
+fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
-- 
1.7.5.1

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

Re: [PATCH 4/4] xfstests: add mapped write fsx operations to 091

[Alex Elder]

On Fri, 2011-07-08 at 10:53 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The recent busted fsx updates caused fsx to execute fsx with direct
> IO and mmapped reads and writes on an XFS filesystem. The result
> uncovered a direct-IO write vs mmap read bug to do with EOF
> sub-block zeroing on the direct IO write.
> 
> Hence whiel we do not recommend that pepole mix DIO with mmap on the
> same file, we should at least have tests that exercise it as they
> often show up other problems like this.
> 
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Good idea.

Reviewed-by: Alex Elder <aelder@sgi.com>


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