Re: [PATCH 2/4] xfstests: fsx fallocate support is b0rked
From: Alex Elder <hidden>
Date: 2011-07-08 18:58:56
On Fri, 2011-07-08 at 10:53 +1000, Dave Chinner wrote:
From: Dave Chinner <redacted> 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 <redacted>
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 <redacted>
quoted hunk ↗ jump to hunk
--- 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.
quoted hunk ↗ jump to hunk
+ +/* !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.
quoted hunk ↗ jump to hunk
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