On Thu, Aug 22, 2024 at 01:50:19PM -0700, Darrick J. Wong wrote:
> On Thu, Aug 22, 2024 at 10:44:20AM -0400, Brian Foster wrote:
> > In preparation for support for eof page pollution, factor out a file
> > size update helper. This updates the internally tracked file size
> > based on the upcoming operation and zeroes the appropriate range in
> > the good buffer for extending operations.
> >
> > Note that a handful of callers currently make these updates after
> > performing the associated operation. Order is not important to
> > current behavior, but it will be for a follow on patch, so make
> > those calls a bit earlier as well.
> >
> > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > ---
> > ltp/fsx.c | 57 +++++++++++++++++++++++++------------------------------
> > 1 file changed, 26 insertions(+), 31 deletions(-)
> >
> > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > index 2dc59b06..1389c51d 100644
> > --- a/ltp/fsx.c
> > +++ b/ltp/fsx.c
> > @@ -983,6 +983,17 @@ gendata(char *original_buf, char *good_buf, unsigned offset, unsigned size)
> > }
> > }
> >
> > +/*
> > + * Helper to update the tracked file size. If the offset begins beyond current
> > + * EOF, zero the range from EOF to offset in the good buffer.
> > + */
> > +void
> > +update_file_size(unsigned offset, unsigned size)
> > +{
> > + if (offset > file_size)
> > + memset(good_buf + file_size, '\0', offset - file_size);
> > + file_size = offset + size;
> > +}
> >
> > void
> > dowrite(unsigned offset, unsigned size)
> > @@ -1003,10 +1014,8 @@ dowrite(unsigned offset, unsigned size)
> > log4(OP_WRITE, offset, size, FL_NONE);
> >
> > gendata(original_buf, good_buf, offset, size);
> > - if (file_size < offset + size) {
> > - if (file_size < offset)
> > - memset(good_buf + file_size, '\0', offset - file_size);
> > - file_size = offset + size;
> > + if (offset + size > file_size) {
> > + update_file_size(offset, size);
> > if (lite) {
> > warn("Lite file size bug in fsx!");
> > report_failure(149);
> > @@ -1070,10 +1079,8 @@ domapwrite(unsigned offset, unsigned size)
> > log4(OP_MAPWRITE, offset, size, FL_NONE);
> >
> > gendata(original_buf, good_buf, offset, size);
> > - if (file_size < offset + size) {
> > - if (file_size < offset)
> > - memset(good_buf + file_size, '\0', offset - file_size);
> > - file_size = offset + size;
> > + if (offset + size > file_size) {
> > + update_file_size(offset, size);
> > if (lite) {
> > warn("Lite file size bug in fsx!");
> > report_failure(200);
> > @@ -1136,9 +1143,7 @@ dotruncate(unsigned size)
> >
> > log4(OP_TRUNCATE, 0, size, FL_NONE);
> >
> > - if (size > file_size)
> > - memset(good_buf + file_size, '\0', size - file_size);
> > - file_size = size;
> > + update_file_size(size, 0);
> >
> > if (testcalls <= simulatedopcount)
> > return;
> > @@ -1247,6 +1252,9 @@ do_zero_range(unsigned offset, unsigned length, int keep_size)
> > log4(OP_ZERO_RANGE, offset, length,
> > keep_size ? FL_KEEP_SIZE : FL_NONE);
> >
> > + if (end_offset > file_size)
> > + update_file_size(offset, length);
> > +
> > if (testcalls <= simulatedopcount)
> > return;
>
> Don't we only want to do the goodbuf zeroing if we don't bail out due to
> the (testcalls <= simulatedopcount) logic? Same question for
> do_clone_range and do_copy_range.
>
Hm, good question. It's quite possible I busted something there. For
some reason I was thinking this would all be a no-op for that mode but I
hadn't quite thought it through (or tested it).
It looks like this is for the -b "beginning operation number" support,
so you can start a test from somewhere beyond operation 0 of a given
seed/sequence. As such it appears to make changes to incore state and
then write the goodbuf out to the file and truncate it as a final step
before proper operation begins.
Looking at how some of the current operations are handled, I think the
size-related goodbuf zeroing is Ok because we'd expect that part of the
file to remain zeroed, after a truncate up, etc., for example. In fact,
I'm wondering if the current zero range behavior is technically wrong
because if a zero range were to extend the file during simulation, we
don't actually update the in-core state as expected. This might actually
be worth factoring out into a bug fix patch with proper explanation.
WRT the eof pollution behavior during simulation, I'm kind of on the
fence. It's arguably wrong because we're not running the kernel
operations and so there's nothing to verify, but OTOH perhaps we should
be doing the zeroing associated with size changes in-core that should
wipe out the eof pollution. Then again, nothing really verifies this and
if something fails, we end up writing that data out to the test file.
None of that is really the purpose of this mechanism, so I'm leaning
towards calling it wrong and making the following tweaks:
1. Split out another bug fix patch to do size related updates (including
zeroing) consistently during simulation.
2. Update pollute_eofpage() in the next patch to skip out when testcalls
<= simulatedopcount (with some commenting).
Let me know if you think any of that doesn't make sense.
> /me reads the second patch but doesn't quite get it. :/
>
The zeroing tests I was using were basically manual test sequences to do
things like this:
xfs_io -fc "truncate 2k" -c "falloc -k 0 4k" ... -c "mwrite 0 4k" \
-c "truncate 8k" <file>
... which itentionally writes beyond EOF before a truncate up operation
to test whether the zero range in the truncate path DTRT with a dirty
folio over an unwritten block. In a nutshell, I'm just trying to
genericize that sort of test a bit more by doing similar post-eof
mwrites opportunistically in fsx in operations that should be expected
to do similar zeroing in-kernel.
I.e., replace the "truncate 8k" above with any size extending operation
that starts beyond EOF such that we should expect the range from
2k-<endofpage> to be zeroed. Does that explain things better?
> Are you doing this to mirror what the kernel does? A comment here to
> explain why we're doing this differently would help me.
>
Yeah, sort of... it's more like writing a canary value to a small range
of the file but rather than expecting it to remain unmodified, we expect
it to be zeroed by the upcoming operation. Therefore we don't write the
garbage data to goodbuf, because goodbuf should contain zeroes and we
want to detect a data mismatch on subsequent reads if the kernel didn't
do the expected zeroing.
Brian
> --D
>
> >
> > @@ -1263,17 +1271,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 +1535,9 @@ do_clone_range(unsigned offset, unsigned length, unsigned dest)
> >
> > log5(OP_CLONE_RANGE, offset, length, dest, FL_NONE);
> >
> > + if (dest + length > file_size)
> > + update_file_size(dest, length);
> > +
> > if (testcalls <= simulatedopcount)
> > return;
> >
> > @@ -1556,10 +1556,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 +1752,9 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
> >
> > log5(OP_COPY_RANGE, offset, length, dest, FL_NONE);
> >
> > + if (dest + length > file_size)
> > + update_file_size(dest, length);
> > +
> > if (testcalls <= simulatedopcount)
> > return;
> >
> > @@ -1792,10 +1791,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
> > @@ -1846,7 +1841,7 @@ do_preallocate(unsigned offset, unsigned length, int keep_size)
> >
> > if (end_offset > file_size) {
> > memset(good_buf + file_size, '\0', end_offset - file_size);
> > - file_size = end_offset;
> > + update_file_size(offset, length);
> > }
> >
> > if (testcalls <= simulatedopcount)
> > --
> > 2.45.0
> >
> >
>
