Re: [PATCH v2 1/4] fsx: don't skip file size and buf updates on simulated ops

[Brian Foster <bfoster@redhat.com>]

On Wed, Aug 28, 2024 at 06:27:16PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 28, 2024 at 02:15:31PM -0400, Brian Foster wrote:
> > fsx supports the ability to skip through a certain number of
> > operations of a given command sequence before beginning full
> > operation. The way this works is by tracking the operation count,
> > simulating minimal side effects of skipped operations in-memory, and
> > then finally writing out the in-memory state to the target file when
> > full operation begins.
> > 
> > Several fallocate() related operations don't correctly track
> > in-memory state when simulated, however. For example, consider an
> > ops file with the following two operations:
> > 
> >   zero_range 0x0 0x1000 0x0
> >   read 0x0 0x1000 0x0
> > 
> > ... and an fsx run like so:
> > 
> >   fsx -d -b 2 --replay-ops=<opsfile> <file>
> > 
> > This simulates the zero_range operation, but fails to track the file
> > extension that occurs as a side effect such that the subsequent read
> > doesn't occur as expected:
> > 
> >   Will begin at operation 2
> >   skipping zero size read
> > 
> > The read is skipped in this case because the file size is zero.  The
> > proper behavior, and what is consistent with other size changing
> > operations, is to make the appropriate in-core changes before
> > checking whether an operation is simulated so the end result of
> > those changes can be reflected on-disk for eventual non-simulated
> > operations. This results in expected behavior with the same ops file
> > and test command:
> > 
> >   Will begin at operation 2
> >   2 read  0x0 thru        0xfff   (0x1000 bytes)
> > 
> > Update zero, copy and clone range to do the file size and EOF change
> > related zeroing before checking against the simulated ops count.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> 
> Oh wow, I really got that wrong. :(
> 

Eh, so did I. ;) That the code was mostly right was pretty much just
luck. Thanks for the thoughtful reviews.

Brian

> Well, thank you for uncovering that error;
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 
> 
> > ---
> >  ltp/fsx.c | 40 +++++++++++++++++++++-------------------
> >  1 file changed, 21 insertions(+), 19 deletions(-)
> > 
> > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > index 2dc59b06..c5727cff 100644
> > --- a/ltp/fsx.c
> > +++ b/ltp/fsx.c
> > @@ -1247,6 +1247,17 @@ do_zero_range(unsigned offset, unsigned length, int keep_size)
> >  	log4(OP_ZERO_RANGE, offset, length,
> >  	     keep_size ? FL_KEEP_SIZE : FL_NONE);
> >  
> > +	if (!keep_size && end_offset > file_size) {
> > +		/*
> > +		 * If there's a gap between the old file size and the offset of
> > +		 * the zero range operation, fill the gap with zeroes.
> > +		 */
> > +		if (offset > file_size)
> > +			memset(good_buf + file_size, '\0', offset - file_size);
> > +
> > +		file_size = end_offset;
> > +	}
> > +
> >  	if (testcalls <= simulatedopcount)
> >  		return;
> >  
> > @@ -1263,17 +1274,6 @@ do_zero_range(unsigned offset, unsigned length, int keep_size)
> >  	}
> >  
> >  	memset(good_buf + offset, '\0', length);
> > -
> > -	if (!keep_size && end_offset > file_size) {
> > -		/*
> > -		 * If there's a gap between the old file size and the offset of
> > -		 * the zero range operation, fill the gap with zeroes.
> > -		 */
> > -		if (offset > file_size)
> > -			memset(good_buf + file_size, '\0', offset - file_size);
> > -
> > -		file_size = end_offset;
> > -	}
> >  }
> >  
> >  #else
> > @@ -1538,6 +1538,11 @@ do_clone_range(unsigned offset, unsigned length, unsigned dest)
> >  
> >  	log5(OP_CLONE_RANGE, offset, length, dest, FL_NONE);
> >  
> > +	if (dest > file_size)
> > +		memset(good_buf + file_size, '\0', dest - file_size);
> > +	if (dest + length > file_size)
> > +		file_size = dest + length;
> > +
> >  	if (testcalls <= simulatedopcount)
> >  		return;
> >  
> > @@ -1556,10 +1561,6 @@ do_clone_range(unsigned offset, unsigned length, unsigned dest)
> >  	}
> >  
> >  	memcpy(good_buf + dest, good_buf + offset, length);
> > -	if (dest > file_size)
> > -		memset(good_buf + file_size, '\0', dest - file_size);
> > -	if (dest + length > file_size)
> > -		file_size = dest + length;
> >  }
> >  
> >  #else
> > @@ -1756,6 +1757,11 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
> >  
> >  	log5(OP_COPY_RANGE, offset, length, dest, FL_NONE);
> >  
> > +	if (dest > file_size)
> > +		memset(good_buf + file_size, '\0', dest - file_size);
> > +	if (dest + length > file_size)
> > +		file_size = dest + length;
> > +
> >  	if (testcalls <= simulatedopcount)
> >  		return;
> >  
> > @@ -1792,10 +1798,6 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
> >  	}
> >  
> >  	memcpy(good_buf + dest, good_buf + offset, length);
> > -	if (dest > file_size)
> > -		memset(good_buf + file_size, '\0', dest - file_size);
> > -	if (dest + length > file_size)
> > -		file_size = dest + length;
> >  }
> >  
> >  #else
> > -- 
> > 2.45.0
> > 
> > 
>