xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/4] xfstests: fsx fallocate support is b0rked
From: Alex Elder <aelder@xxxxxxx>
Date: Fri, 8 Jul 2011 13:58:51 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <1310086426-30605-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1310086426-30605-1-git-send-email-david@xxxxxxxxxxxxx> <1310086426-30605-3-git-send-email-david@xxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
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)



<Prev in Thread] Current Thread [Next in Thread>