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 _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs