xfs
[Top] [All Lists]

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

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 2/4] xfstests: fsx fallocate support is b0rked
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 11 Jul 2011 16:14:58 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1310151531.3024.37.camel@doink>
References: <1310086426-30605-1-git-send-email-david@xxxxxxxxxxxxx> <1310086426-30605-3-git-send-email-david@xxxxxxxxxxxxx> <1310151531.3024.37.camel@doink>
User-agent: Mutt/1.5.20 (2009-06-14)
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@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).

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@xxxxxxxxxxxxx

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