| Message ID | 20240828181534.41054-2-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fstests/fsx: test coverage for eof zeroing | expand |
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 > >
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 > > > > >
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
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> --- ltp/fsx.c | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-)