On Mon, Aug 26, 2024 at 10:03:54AM -0400, Brian Foster wrote:
> 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:
>
Err.. I think you can disregard most of this reasoning. I got my wires
crossed between zeroing behavior and pollute_eofpage().
pollute_eofpage() updates the file only, so just blows up in simulation
mode and is clearly wrong. :) That makes things more simple, though I
think the plan below to disable it is the same...
Brian
> 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
> > >
> > >
> >
>
>
