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

["Darrick J. Wong" <djwong@kernel.org>]

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. :(

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