On Fri, 2011-07-08 at 10:53 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
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@xxxxxxx>
> ---
> 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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
