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: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Mon, 27 Jun 2011 16:16:20 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1309153722-1231-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1309153722-1231-1-git-send-email-david@xxxxxxxxxxxxx> <1309153722-1231-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.18) Gecko/20110616 Thunderbird/3.1.11
On 6/27/11 12:48 AM, 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()

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@xxxxxxxxxx>
> ---
>  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)

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