[PATCHSET] fstests: random fixes for v2024.11.17

[PATCHSET] fstests: random fixes for v2024.11.17

[Darrick J. Wong]

Hi all,

Here's the usual odd fixes for fstests.  Most of these are cleanups and
bug fixes that have been aging in my djwong-wtf branch forever.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

With a bit of luck, this should all go splendidly.
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
Commits in this patchset:
 * generic/757: fix various bugs in this test
 * xfs/113: fix failure to corrupt the entire directory
 * xfs/508: fix test for 64k blocksize
 * common/rc: capture dmesg when oom kills happen
 * generic/562: handle ENOSPC while cloning gracefully
 * xfs/163: skip test if we can't shrink due to enospc issues
 * xfs/009: allow logically contiguous preallocations
 * generic/251: use sentinel files to kill the fstrim loop
 * generic/251: constrain runtime via time/load/soak factors
 * common/rc: _scratch_mkfs_sized supports extra arguments
 * xfs/157: do not drop necessary mkfs options
 * xfs/157: fix test failures when MKFS_OPTIONS has -L options
---
 common/rc         |   35 +++++++++++++++++++----------------
 tests/generic/251 |   34 ++++++++++++++++++----------------
 tests/generic/562 |   10 ++++++++--
 tests/generic/757 |    7 ++++++-
 tests/xfs/009     |   29 ++++++++++++++++++++++++++++-
 tests/xfs/113     |   33 +++++++++++++++++++++++++++------
 tests/xfs/157     |   30 +++++++++++++++++++++++++++---
 tests/xfs/163     |    9 ++++++++-
 tests/xfs/508     |    4 ++--
 9 files changed, 143 insertions(+), 48 deletions(-)

[PATCH 01/12] generic/757: fix various bugs in this test

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Fix this test so the check doesn't fail on XFS, and restrict runtime to
100 loops because otherwise this test takes many hours.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 tests/generic/757 |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


diff --git a/tests/generic/757 b/tests/generic/757
index 0ff5a8ac00182b..9d41975bde07bb 100755
--- a/tests/generic/757
+++ b/tests/generic/757
@@ -63,9 +63,14 @@ prev=$(_log_writes_mark_to_entry_number mkfs)
 cur=$(_log_writes_find_next_fua $prev)
 [ -z "$cur" ] && _fail "failed to locate next FUA write"
 
-while [ ! -z "$cur" ]; do
+for ((i = 0; i < 100; i++)); do
 	_log_writes_replay_log_range $cur $SCRATCH_DEV >> $seqres.full
 
+	# xfs_repair won't run if the log is dirty
+	if [ $FSTYP = "xfs" ]; then
+		_scratch_mount
+		_scratch_unmount
+	fi
 	_check_scratch_fs
 
 	prev=$cur

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Zorro Lang]

On Mon, Nov 18, 2024 at 03:01:38PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Fix this test so the check doesn't fail on XFS, and restrict runtime to
> 100 loops because otherwise this test takes many hours.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  tests/generic/757 |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/tests/generic/757 b/tests/generic/757
> index 0ff5a8ac00182b..9d41975bde07bb 100755
> --- a/tests/generic/757
> +++ b/tests/generic/757
> @@ -63,9 +63,14 @@ prev=$(_log_writes_mark_to_entry_number mkfs)
>  cur=$(_log_writes_find_next_fua $prev)
>  [ -z "$cur" ] && _fail "failed to locate next FUA write"
>  
> -while [ ! -z "$cur" ]; do
> +for ((i = 0; i < 100; i++)); do
>  	_log_writes_replay_log_range $cur $SCRATCH_DEV >> $seqres.full
>  
> +	# xfs_repair won't run if the log is dirty
> +	if [ $FSTYP = "xfs" ]; then
> +		_scratch_mount
> +		_scratch_unmount
> +	fi

Hi Darrick,

I didn't merge this patch last week, due to we were still talking
about the "discards" things:

https://lore.kernel.org/fstests/20241115182821.s3pt4wmkueyjggx3@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com/T/#u

Do you think we need to do a force discards at here, or change the
SCRATCH_DEV to dmthin to support discards?

Thanks,
Zorro

>  	_check_scratch_fs
>  
>  	prev=$cur
> 

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Christoph Hellwig]

On Thu, Nov 21, 2024 at 05:56:24PM +0800, Zorro Lang wrote:
> I didn't merge this patch last week, due to we were still talking
> about the "discards" things:
> 
> https://lore.kernel.org/fstests/20241115182821.s3pt4wmkueyjggx3@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com/T/#u
> 
> Do you think we need to do a force discards at here, or change the
> SCRATCH_DEV to dmthin to support discards?

FYI, I'm seeing regular failures with generic/757 when using Darrick's
not yet merged RT rmap support, but only with that.

But the whole discard thing leaves me really confused, and the commit
log in the patch references by the above link doesn't clear that up
either.

Why does dmlogwrites require discard for XFS (and apprently XFS only)?
Note that discard is not required and often does not zero data.  So
if we need data to be zeroed we need to do that explicitly, and
preferably in a way that is obvious.

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Christoph Hellwig]

On Thu, Nov 21, 2024 at 11:05:55AM +0100, Christoph Hellwig wrote:
> But the whole discard thing leaves me really confused, and the commit
> log in the patch references by the above link doesn't clear that up
> either.
> 
> Why does dmlogwrites require discard for XFS (and apprently XFS only)?
> Note that discard is not required and often does not zero data.  So
> if we need data to be zeroed we need to do that explicitly, and
> preferably in a way that is obvious.

Ok, I found the problem.  src/log-writes/log-writes.c does try to
discard to zero, which is broken.  This patch switches it to the
proper zeroout ioctl, which fixes my generic/757 issues, and should
also allow to revert commits to use dm-thin referenced in your link.

diff --git a/src/log-writes/log-writes.c b/src/log-writes/log-writes.c
index aa53473974d9..cb3ac962574c 100644
--- a/src/log-writes/log-writes.c
+++ b/src/log-writes/log-writes.c
@@ -31,7 +31,7 @@ static int discard_range(struct log *log, u64 start, u64 len)
 {
 	u64 range[2] = { start, len };
 
-	if (ioctl(log->replayfd, BLKDISCARD, &range) < 0) {
+	if (ioctl(log->replayfd, BLKZEROOUT, &range) < 0) {
 		if (log_writes_verbose)
 			printf("replay device doesn't support discard, "
 			       "switching to writing zeros\n");

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Christoph Hellwig]

On Thu, Nov 21, 2024 at 11:13:25AM +0100, Christoph Hellwig wrote:
> proper zeroout ioctl, which fixes my generic/757 issues, and should

It turns out then when I extrapolate my shortened 10 iteration run
to the 100 currently in the test it would take ~ 30 minutes.  I'm
not sure that's really a reasonable run time for a single test in
the auto group.

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Darrick J. Wong]

On Thu, Nov 21, 2024 at 11:52:48AM +0100, Christoph Hellwig wrote:
> On Thu, Nov 21, 2024 at 11:13:25AM +0100, Christoph Hellwig wrote:
> > proper zeroout ioctl, which fixes my generic/757 issues, and should
> 
> It turns out then when I extrapolate my shortened 10 iteration run
> to the 100 currently in the test it would take ~ 30 minutes.  I'm
> not sure that's really a reasonable run time for a single test in
> the auto group.

Yeah, perhaps I should adjust this one to use TIME_FACTOR too?

while _soak_loop_running $((10 * TIME_FACTOR)); do
	# timeconsuming stuff
done

(and then you can SOAK_DURATION=10s to limit the runtime)

--D

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Darrick J. Wong]

On Thu, Nov 21, 2024 at 08:33:55AM -0800, Darrick J. Wong wrote:
> On Thu, Nov 21, 2024 at 11:52:48AM +0100, Christoph Hellwig wrote:
> > On Thu, Nov 21, 2024 at 11:13:25AM +0100, Christoph Hellwig wrote:
> > > proper zeroout ioctl, which fixes my generic/757 issues, and should
> > 
> > It turns out then when I extrapolate my shortened 10 iteration run
> > to the 100 currently in the test it would take ~ 30 minutes.  I'm
> > not sure that's really a reasonable run time for a single test in
> > the auto group.
> 
> Yeah, perhaps I should adjust this one to use TIME_FACTOR too?
> 
> while _soak_loop_running $((10 * TIME_FACTOR)); do
> 	# timeconsuming stuff
> done
> 
> (and then you can SOAK_DURATION=10s to limit the runtime)

Oh, that's with your BLKDISCARD -> BLKZEROOUT change applied, isn't it?
On my system, 100 loops takes 96 seconds with discard and 639 seconds
with zeroout.

So yes, we should shift this test to use thinp.  I was going to say that
we should also make _log_writes_init skip the test if the block device
has discard_zeroes_data==0, but apparently it's now hardwired to zero
and (AFAICT) there's no way to tell if unmap actually zeroes even for
software defined devices like thinp where we *know* that works. :(

--D

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Christoph Hellwig]

On Thu, Nov 21, 2024 at 09:19:24AM -0800, Darrick J. Wong wrote:
> Oh, that's with your BLKDISCARD -> BLKZEROOUT change applied, isn't it?
> On my system, 100 loops takes 96 seconds with discard and 639 seconds
> with zeroout.

The discard path is guaranteed to fail after the first iteration, but
that at least happens very quickly..

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Brian Foster]

On Thu, Nov 21, 2024 at 11:05:55AM +0100, Christoph Hellwig wrote:
> On Thu, Nov 21, 2024 at 05:56:24PM +0800, Zorro Lang wrote:
> > I didn't merge this patch last week, due to we were still talking
> > about the "discards" things:
> > 
> > https://lore.kernel.org/fstests/20241115182821.s3pt4wmkueyjggx3@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com/T/#u
> > 
> > Do you think we need to do a force discards at here, or change the
> > SCRATCH_DEV to dmthin to support discards?
> 
> FYI, I'm seeing regular failures with generic/757 when using Darrick's
> not yet merged RT rmap support, but only with that.
> 
> But the whole discard thing leaves me really confused, and the commit
> log in the patch references by the above link doesn't clear that up
> either.
> 
> Why does dmlogwrites require discard for XFS (and apprently XFS only)?
> Note that discard is not required and often does not zero data.  So
> if we need data to be zeroed we need to do that explicitly, and
> preferably in a way that is obvious.
> 

IIRC it was to accommodate the test program, which presumably used
discard for efficiency reasons because it did a lot of context switching
to different point-in-time variations of the fs. If the discard didn't
actually zero the range (depending on the underlying test dev), then at
least on XFS, we'd see odd recovery issues and whatnot from the fs going
forward/back in time.

Therefore the reason for using dm-thin was that it was an easy way to
provide predictable behavior to the test program, where discards punch
out blocks that subsequently return zeroes.

I don't recall all the specifics, but I thought part of the reason for
using discard over explicit zeroing was the latter made the test
impractically slow. I could be misremembering, but if you want to change
it I'd suggest to at least verify runtimes on some of the preexisting
logwrites tests as well.

Brian

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Christoph Hellwig]

On Thu, Nov 21, 2024 at 07:28:09AM -0500, Brian Foster wrote:
> IIRC it was to accommodate the test program, which presumably used
> discard for efficiency reasons because it did a lot of context switching
> to different point-in-time variations of the fs. If the discard didn't
> actually zero the range (depending on the underlying test dev), then at
> least on XFS, we'd see odd recovery issues and whatnot from the fs going
> forward/back in time.

But the fundamental problem is that discard does not actually zero
data.  It sometimes might, but also often doesn't because it's not
part of the contract.  That's why my patch two switch to the zeroing
ioctl is the right thing.  Note that this doesn't mean explicitly
writing zeroes, it still uses zeroing offloads available in the drivers.

> I don't recall all the specifics, but I thought part of the reason for
> using discard over explicit zeroing was the latter made the test
> impractically slow. I could be misremembering, but if you want to change
> it I'd suggest to at least verify runtimes on some of the preexisting
> logwrites tests as well.

I'm all for speeding up tests.  But relying on a unspecified side effect
of an operation and then requiring a driver that implements that side
effect without documenting that isn't really good practice.

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Brian Foster]

On Thu, Nov 21, 2024 at 02:12:39PM +0100, Christoph Hellwig wrote:
> On Thu, Nov 21, 2024 at 07:28:09AM -0500, Brian Foster wrote:
> > IIRC it was to accommodate the test program, which presumably used
> > discard for efficiency reasons because it did a lot of context switching
> > to different point-in-time variations of the fs. If the discard didn't
> > actually zero the range (depending on the underlying test dev), then at
> > least on XFS, we'd see odd recovery issues and whatnot from the fs going
> > forward/back in time.
> 
> But the fundamental problem is that discard does not actually zero
> data.  It sometimes might, but also often doesn't because it's not
> part of the contract.  That's why my patch two switch to the zeroing
> ioctl is the right thing.  Note that this doesn't mean explicitly
> writing zeroes, it still uses zeroing offloads available in the drivers.
> 

Ok. I get that wrt discard.. I'm saying that's why we inserted dm-thin
into the stack, because it had predictable behavior that fell in line
with this particular design quirk of the higher level test program.

I've no issue with changing it so long as we maintain effectiveness of
the test. I'm not familiar enough with the zeroing offloads in the
drivers you mention to know whether that means this test can still run
quickly in the general case, or only with select hardware..?

> > I don't recall all the specifics, but I thought part of the reason for
> > using discard over explicit zeroing was the latter made the test
> > impractically slow. I could be misremembering, but if you want to change
> > it I'd suggest to at least verify runtimes on some of the preexisting
> > logwrites tests as well.
> 
> I'm all for speeding up tests.  But relying on a unspecified side effect
> of an operation and then requiring a driver that implements that side
> effect without documenting that isn't really good practice.
> 

It's a hack to facilitate test coverage. It would obviously need to be
revisited if behavior changed sufficiently to break the test.

I'm not really sure what you're asking for wrt documentation. A quick
scan of the git history shows the first such commit is 65cc9a235919
("generic/482: use thin volume as data device"), the commit log for
which seems to explain the reasoning.

Brian

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Christoph Hellwig]

On Thu, Nov 21, 2024 at 09:11:56AM -0500, Brian Foster wrote:
> > I'm all for speeding up tests.  But relying on a unspecified side effect
> > of an operation and then requiring a driver that implements that side
> > effect without documenting that isn't really good practice.
> > 
> 
> It's a hack to facilitate test coverage. It would obviously need to be
> revisited if behavior changed sufficiently to break the test.
> 
> I'm not really sure what you're asking for wrt documentation. A quick
> scan of the git history shows the first such commit is 65cc9a235919
> ("generic/482: use thin volume as data device"), the commit log for
> which seems to explain the reasoning.

A comment on _log_writes_init that it must only be used by dm-thin
because it relies on the undocumented behavior that dm-trim zeroes
all blocks discarded.

Or even better my moving the dm-think setup boilerplate into the log
writes helpers, so that it gets done automatically.

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Brian Foster]

On Fri, Nov 22, 2024 at 01:31:33PM +0100, Christoph Hellwig wrote:
> On Thu, Nov 21, 2024 at 09:11:56AM -0500, Brian Foster wrote:
> > > I'm all for speeding up tests.  But relying on a unspecified side effect
> > > of an operation and then requiring a driver that implements that side
> > > effect without documenting that isn't really good practice.
> > > 
> > 
> > It's a hack to facilitate test coverage. It would obviously need to be
> > revisited if behavior changed sufficiently to break the test.
> > 
> > I'm not really sure what you're asking for wrt documentation. A quick
> > scan of the git history shows the first such commit is 65cc9a235919
> > ("generic/482: use thin volume as data device"), the commit log for
> > which seems to explain the reasoning.
> 
> A comment on _log_writes_init that it must only be used by dm-thin
> because it relies on the undocumented behavior that dm-trim zeroes
> all blocks discarded.
> 
> Or even better my moving the dm-think setup boilerplate into the log
> writes helpers, so that it gets done automatically.
> 

A related idea might be to incorporate your BLKZEROOUT fix so the core
tool is fundamentally correct, but then wrap the existing discard
behavior in a param or something that the dm-thin oriented tests can
pass to enable it as a fast zero hack/optimization.

But that all seems reasonable to me either way. I'm not sure that's
something I would have fully abstracted into the logwrites stuff
initially, but here we are ~5 years later and it seems pretty much every
additional logwrites test has wanted the same treatment. If whoever
wants to convert this newer test over wants to start by refactoring
things that way, that sounds like a welcome cleanup to me.

Brian

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Darrick J. Wong]

On Fri, Nov 22, 2024 at 08:49:42AM -0500, Brian Foster wrote:
> On Fri, Nov 22, 2024 at 01:31:33PM +0100, Christoph Hellwig wrote:
> > On Thu, Nov 21, 2024 at 09:11:56AM -0500, Brian Foster wrote:
> > > > I'm all for speeding up tests.  But relying on a unspecified side effect
> > > > of an operation and then requiring a driver that implements that side
> > > > effect without documenting that isn't really good practice.
> > > > 
> > > 
> > > It's a hack to facilitate test coverage. It would obviously need to be
> > > revisited if behavior changed sufficiently to break the test.
> > > 
> > > I'm not really sure what you're asking for wrt documentation. A quick
> > > scan of the git history shows the first such commit is 65cc9a235919
> > > ("generic/482: use thin volume as data device"), the commit log for
> > > which seems to explain the reasoning.
> > 
> > A comment on _log_writes_init that it must only be used by dm-thin
> > because it relies on the undocumented behavior that dm-trim zeroes
> > all blocks discarded.
> > 
> > Or even better my moving the dm-think setup boilerplate into the log
> > writes helpers, so that it gets done automatically.
> > 
> 
> A related idea might be to incorporate your BLKZEROOUT fix so the core
> tool is fundamentally correct, but then wrap the existing discard
> behavior in a param or something that the dm-thin oriented tests can
> pass to enable it as a fast zero hack/optimization.
> 
> But that all seems reasonable to me either way. I'm not sure that's
> something I would have fully abstracted into the logwrites stuff
> initially, but here we are ~5 years later and it seems pretty much every
> additional logwrites test has wanted the same treatment. If whoever
> wants to convert this newer test over wants to start by refactoring
> things that way, that sounds like a welcome cleanup to me.

Ugh, I just want to fix this stupid test and move on with the bugfixes,
not refactor every logwrites user in the codebase just to reduce one
test's runtime from hours to 90s.

It's not as simple as making the logwrites init function set up thinp on
its own, because there's at least one test out there (generic/470) that
takes care of its own discarding, and then there's whatever the strange
stuff that the tests/btrfs/ users do -- it looks fairly simple, but I
don't really want to go digging into that just to make sure I didn't
break their testing.

I'll send what I have currently, which adds a warning about running
logwrites on a device that supports discard but isn't thinp... in
addition to fixing the xfs log recovery thing, and in addition to fixing
the loop duration.

I guess I can add yet another patch to switch the replay program to use
BLKDISCARD if the _init function thinks it's ok, but seriously... you
guys need to send start sending patches implementing the new
functionality that you suggest.

--D

> Brian
> 
> 

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Christoph Hellwig]

On Fri, Nov 22, 2024 at 08:13:47AM -0800, Darrick J. Wong wrote:
> I guess I can add yet another patch to switch the replay program to use
> BLKDISCARD if the _init function thinks it's ok, but seriously... you
> guys need to send start sending patches implementing the new
> functionality that you suggest.

I can look into the refactoring and the comments, I wasn't planning
to offload the work onto you, just making sure we have robust
infrastructure.

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Brian Foster]

On Fri, Nov 22, 2024 at 08:13:47AM -0800, Darrick J. Wong wrote:
> On Fri, Nov 22, 2024 at 08:49:42AM -0500, Brian Foster wrote:
> > On Fri, Nov 22, 2024 at 01:31:33PM +0100, Christoph Hellwig wrote:
> > > On Thu, Nov 21, 2024 at 09:11:56AM -0500, Brian Foster wrote:
> > > > > I'm all for speeding up tests.  But relying on a unspecified side effect
> > > > > of an operation and then requiring a driver that implements that side
> > > > > effect without documenting that isn't really good practice.
> > > > > 
> > > > 
> > > > It's a hack to facilitate test coverage. It would obviously need to be
> > > > revisited if behavior changed sufficiently to break the test.
> > > > 
> > > > I'm not really sure what you're asking for wrt documentation. A quick
> > > > scan of the git history shows the first such commit is 65cc9a235919
> > > > ("generic/482: use thin volume as data device"), the commit log for
> > > > which seems to explain the reasoning.
> > > 
> > > A comment on _log_writes_init that it must only be used by dm-thin
> > > because it relies on the undocumented behavior that dm-trim zeroes
> > > all blocks discarded.
> > > 
> > > Or even better my moving the dm-think setup boilerplate into the log
> > > writes helpers, so that it gets done automatically.
> > > 
> > 
> > A related idea might be to incorporate your BLKZEROOUT fix so the core
> > tool is fundamentally correct, but then wrap the existing discard
> > behavior in a param or something that the dm-thin oriented tests can
> > pass to enable it as a fast zero hack/optimization.
> > 
> > But that all seems reasonable to me either way. I'm not sure that's
> > something I would have fully abstracted into the logwrites stuff
> > initially, but here we are ~5 years later and it seems pretty much every
> > additional logwrites test has wanted the same treatment. If whoever
> > wants to convert this newer test over wants to start by refactoring
> > things that way, that sounds like a welcome cleanup to me.
> 
> Ugh, I just want to fix this stupid test and move on with the bugfixes,
> not refactor every logwrites user in the codebase just to reduce one
> test's runtime from hours to 90s.
> 
> It's not as simple as making the logwrites init function set up thinp on
> its own, because there's at least one test out there (generic/470) that
> takes care of its own discarding, and then there's whatever the strange
> stuff that the tests/btrfs/ users do -- it looks fairly simple, but I
> don't really want to go digging into that just to make sure I didn't
> break their testing.
> 
> I'll send what I have currently, which adds a warning about running
> logwrites on a device that supports discard but isn't thinp... in
> addition to fixing the xfs log recovery thing, and in addition to fixing
> the loop duration.
> 
> I guess I can add yet another patch to switch the replay program to use
> BLKDISCARD if the _init function thinks it's ok, but seriously... you
> guys need to send start sending patches implementing the new
> functionality that you suggest.
> 

Sorry, I should have been more clear. I certainly don't insist on it as
an immediate change or to gatekeep the current patch. I'm just acking
the idea, and I think it's perfectly fair to say "more time consuming
than I have time for right now" if you planned to just fixup the test
itself. I may get to it opportunistically someday, or if hch cares
enough about it he's certainly capable of picking it up sooner.

For future reference, I'm generally not trying to tell people what to do
with their patches or force work on people, etc. I realize we have a
tendency to do that. I don't like it either. It would be nice if we had
a clearer way to express/discuss an idea without implying it as a
demand.

Brian

> --D
> 
> > Brian
> > 
> > 
> 

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Darrick J. Wong]

On Fri, Nov 22, 2024 at 11:33:24AM -0500, Brian Foster wrote:
> On Fri, Nov 22, 2024 at 08:13:47AM -0800, Darrick J. Wong wrote:
> > On Fri, Nov 22, 2024 at 08:49:42AM -0500, Brian Foster wrote:
> > > On Fri, Nov 22, 2024 at 01:31:33PM +0100, Christoph Hellwig wrote:
> > > > On Thu, Nov 21, 2024 at 09:11:56AM -0500, Brian Foster wrote:
> > > > > > I'm all for speeding up tests.  But relying on a unspecified side effect
> > > > > > of an operation and then requiring a driver that implements that side
> > > > > > effect without documenting that isn't really good practice.
> > > > > > 
> > > > > 
> > > > > It's a hack to facilitate test coverage. It would obviously need to be
> > > > > revisited if behavior changed sufficiently to break the test.
> > > > > 
> > > > > I'm not really sure what you're asking for wrt documentation. A quick
> > > > > scan of the git history shows the first such commit is 65cc9a235919
> > > > > ("generic/482: use thin volume as data device"), the commit log for
> > > > > which seems to explain the reasoning.
> > > > 
> > > > A comment on _log_writes_init that it must only be used by dm-thin
> > > > because it relies on the undocumented behavior that dm-trim zeroes
> > > > all blocks discarded.
> > > > 
> > > > Or even better my moving the dm-think setup boilerplate into the log
> > > > writes helpers, so that it gets done automatically.
> > > > 
> > > 
> > > A related idea might be to incorporate your BLKZEROOUT fix so the core
> > > tool is fundamentally correct, but then wrap the existing discard
> > > behavior in a param or something that the dm-thin oriented tests can
> > > pass to enable it as a fast zero hack/optimization.
> > > 
> > > But that all seems reasonable to me either way. I'm not sure that's
> > > something I would have fully abstracted into the logwrites stuff
> > > initially, but here we are ~5 years later and it seems pretty much every
> > > additional logwrites test has wanted the same treatment. If whoever
> > > wants to convert this newer test over wants to start by refactoring
> > > things that way, that sounds like a welcome cleanup to me.
> > 
> > Ugh, I just want to fix this stupid test and move on with the bugfixes,
> > not refactor every logwrites user in the codebase just to reduce one
> > test's runtime from hours to 90s.
> > 
> > It's not as simple as making the logwrites init function set up thinp on
> > its own, because there's at least one test out there (generic/470) that
> > takes care of its own discarding, and then there's whatever the strange
> > stuff that the tests/btrfs/ users do -- it looks fairly simple, but I
> > don't really want to go digging into that just to make sure I didn't
> > break their testing.
> > 
> > I'll send what I have currently, which adds a warning about running
> > logwrites on a device that supports discard but isn't thinp... in
> > addition to fixing the xfs log recovery thing, and in addition to fixing
> > the loop duration.
> > 
> > I guess I can add yet another patch to switch the replay program to use
> > BLKDISCARD if the _init function thinks it's ok, but seriously... you
> > guys need to send start sending patches implementing the new
> > functionality that you suggest.
> > 
> 
> Sorry, I should have been more clear. I certainly don't insist on it as
> an immediate change or to gatekeep the current patch. I'm just acking
> the idea, and I think it's perfectly fair to say "more time consuming
> than I have time for right now" if you planned to just fixup the test
> itself. I may get to it opportunistically someday, or if hch cares
> enough about it he's certainly capable of picking it up sooner.
> 
> For future reference, I'm generally not trying to tell people what to do
> with their patches or force work on people, etc. I realize we have a
> tendency to do that. I don't like it either. It would be nice if we had
> a clearer way to express/discuss an idea without implying it as a
> demand.

/me suggests

"Here's something that we ought to do, though as a separate patchset:
clean up all the $fubar to be $less_fubar.  In the meantime this patch
is good enough for now."

--D

> Brian
> 
> > --D
> > 
> > > Brian
> > > 
> > > 
> > 
> 

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Darrick J. Wong]

On Thu, Nov 21, 2024 at 07:28:09AM -0500, Brian Foster wrote:
> On Thu, Nov 21, 2024 at 11:05:55AM +0100, Christoph Hellwig wrote:
> > On Thu, Nov 21, 2024 at 05:56:24PM +0800, Zorro Lang wrote:
> > > I didn't merge this patch last week, due to we were still talking
> > > about the "discards" things:
> > > 
> > > https://lore.kernel.org/fstests/20241115182821.s3pt4wmkueyjggx3@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com/T/#u
> > > 
> > > Do you think we need to do a force discards at here, or change the
> > > SCRATCH_DEV to dmthin to support discards?
> > 
> > FYI, I'm seeing regular failures with generic/757 when using Darrick's
> > not yet merged RT rmap support, but only with that.
> > 
> > But the whole discard thing leaves me really confused, and the commit
> > log in the patch references by the above link doesn't clear that up
> > either.
> > 
> > Why does dmlogwrites require discard for XFS (and apprently XFS only)?
> > Note that discard is not required and often does not zero data.  So
> > if we need data to be zeroed we need to do that explicitly, and
> > preferably in a way that is obvious.
> > 
> 
> IIRC it was to accommodate the test program, which presumably used
> discard for efficiency reasons because it did a lot of context switching
> to different point-in-time variations of the fs. If the discard didn't
> actually zero the range (depending on the underlying test dev), then at
> least on XFS, we'd see odd recovery issues and whatnot from the fs going
> forward/back in time.

Yes, that's my recollection too -- performing a logwrite replay of an
old mark means that you can end up with blocks with the correct fs uuid
but an LSN that's higher than anything in the log.  Recovery will then
skip the block replay, which is not correct.

I suppose we could fix log recovery to treat incoming block LSNs that
are higher than the log head as if there were no block contents at all.
OTOH going backwards in time isn't usually a concern...right?

> Therefore the reason for using dm-thin was that it was an easy way to
> provide predictable behavior to the test program, where discards punch
> out blocks that subsequently return zeroes.

Yep.  The test needs to reset the block device to a zeroed state.
Discards get us there quickly, but only if discard_zeroes_data==1.
Hence bolting dm-thinp (where this is guaranteed) onto the logwrites
tests.

> I don't recall all the specifics, but I thought part of the reason for
> using discard over explicit zeroing was the latter made the test
> impractically slow. I could be misremembering, but if you want to change
> it I'd suggest to at least verify runtimes on some of the preexisting
> logwrites tests as well.

Not sure -- I think BLKZEROOUT will cause allocations and real disk
writes if we're not careful.

--D

> Brian
> 
> 

Re: [PATCH 01/12] generic/757: fix various bugs in this test

[Christoph Hellwig]

On Thu, Nov 21, 2024 at 08:04:15AM -0800, Darrick J. Wong wrote:
> > IIRC it was to accommodate the test program, which presumably used
> > discard for efficiency reasons because it did a lot of context switching
> > to different point-in-time variations of the fs. If the discard didn't
> > actually zero the range (depending on the underlying test dev), then at
> > least on XFS, we'd see odd recovery issues and whatnot from the fs going
> > forward/back in time.
> 
> Yes, that's my recollection too -- performing a logwrite replay of an
> old mark means that you can end up with blocks with the correct fs uuid
> but an LSN that's higher than anything in the log.  Recovery will then
> skip the block replay, which is not correct.
> 
> I suppose we could fix log recovery to treat incoming block LSNs that
> are higher than the log head as if there were no block contents at all.
> OTOH going backwards in time isn't usually a concern...right?

It's probably the best we can do.  Recover as far as everything validated
and then give up.

> 
> > Therefore the reason for using dm-thin was that it was an easy way to
> > provide predictable behavior to the test program, where discards punch
> > out blocks that subsequently return zeroes.
> 
> Yep.  The test needs to reset the block device to a zeroed state.
> Discards get us there quickly, but only if discard_zeroes_data==1.
> Hence bolting dm-thinp (where this is guaranteed) onto the logwrites
> tests.

discard_zeroes_data was unfortunately always broken as no standard
gives you any such guarantee.  The best you get is a guarantee that
it returns zeroes if it actually deallocated the block, but if it
deallocates a given block or not is a black box.

> 
> > I don't recall all the specifics, but I thought part of the reason for
> > using discard over explicit zeroing was the latter made the test
> > impractically slow. I could be misremembering, but if you want to change
> > it I'd suggest to at least verify runtimes on some of the preexisting
> > logwrites tests as well.
> 
> Not sure -- I think BLKZEROOUT will cause allocations and real disk
> writes if we're not careful.

If the device reports a queue/write_zeroes_max_bytes it supports a hardware
offload.  That could still write zeroes to the media if the device
is stupid enough, but hopefully not many are.

[PATCH 02/12] xfs/113: fix failure to corrupt the entire directory

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

This test tries to corrupt the data blocks of a directory, but it
doesn't take into account the fact that __populate_check_xfs_dir can
remove enough entries to cause sparse holes in the directory.  If that
happens, this "file data block is unmapped" logic will cause the
corruption loop to exit early.  Then we can add to the directory, which
causes the test to fail.

Instead, create a list of mappable dir block offsets, and run 100
corruptions at a time to reduce the amount of time we spend initializing
xfs_db.  This fixes the regressions that I see with 32k/64k block sizes.

Cc: <fstests@vger.kernel.org> # v2022.05.01
Fixes: c8e6dbc8812653 ("xfs: test directory metadata corruption checking and repair")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/113 |   33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)


diff --git a/tests/xfs/113 b/tests/xfs/113
index 094ab71f2aefec..22ac8c3fd51b80 100755
--- a/tests/xfs/113
+++ b/tests/xfs/113
@@ -52,13 +52,34 @@ _scratch_xfs_repair -n >> $seqres.full 2>&1 || _fail "xfs_repair should not fail
 echo "+ check dir"
 __populate_check_xfs_dir "${inode}" btree
 
+dir_data_offsets() {
+	_scratch_xfs_db -c "inode ${inode}" -c 'bmap' | \
+		awk -v leaf_lblk=$leaf_lblk \
+		'{
+			if ($3 >= leaf_lblk)
+				exit;
+			for (i = 0; i < $8; i++)
+				printf("%d\n", $3 + i);
+		}'
+}
+
 echo "+ corrupt dir"
-loff=0
-while true; do
-	_scratch_xfs_db -x -c "inode ${inode}" -c "dblock ${loff}" -c "stack" | grep -q 'file data block is unmapped' && break
-	_scratch_xfs_db -x -c "inode ${inode}" -c "dblock ${loff}" -c "stack" -c "blocktrash -x 32 -y $((blksz * 8)) -z ${FUZZ_ARGS}" >> $seqres.full
-	loff="$((loff + 1))"
-done
+subcommands=()
+while read loff; do
+	# run 100 commands at a time
+	if [ "${#subcommands[@]}" -lt 600 ]; then
+		subcommands+=(-c "inode ${inode}")
+		subcommands+=(-c "dblock ${loff}")
+		subcommands+=(-c "blocktrash -x 32 -y $((blksz * 8)) -z ${FUZZ_ARGS}")
+		continue
+	fi
+
+	_scratch_xfs_db -x "${subcommands[@]}" >> $seqres.full
+	subcommands=()
+done < <(dir_data_offsets)
+if [ "${#subcommands[@]}" -gt 0 ]; then
+	_scratch_xfs_db -x "${subcommands[@]}" >> $seqres.full
+fi
 
 echo "+ mount image && modify dir"
 if _try_scratch_mount >> $seqres.full 2>&1; then

[PATCH 03/12] xfs/508: fix test for 64k blocksize

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

It turns out that icreate transactions will try to reserve quite a bit
of space on a 64k fsblock filesystem -- enough to handle the worst case
parent directory expansion, a new inode chunk, and these days a parent
pointer as well.  This can work out to quite a bit of space:

fsblock		reservation
1k		172K
4k		368K
16k		1136K
64k		3650K

Unfortunately, this test sets its block quota limits at 1-2MB, so we
can't even create a child file.  Bump the limits up by 10x so that this
test will pass even if there's more metadata size creep in the future.

Fixes: f769a923f576df ("xfs: project quota ineritance flag test")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/508 |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/tests/xfs/508 b/tests/xfs/508
index ee1a0371db7d6d..1bd13e98c9f641 100755
--- a/tests/xfs/508
+++ b/tests/xfs/508
@@ -44,7 +44,7 @@ do_quota_nospc()
 	local exp=$2
 
 	echo "Write $file, expect $exp:" | _filter_scratch
-	$XFS_IO_PROG -t -f -c "pwrite 0 5m" $file 2>&1 >/dev/null | \
+	$XFS_IO_PROG -t -f -c "pwrite 0 50m" $file 2>&1 >/dev/null | \
 		_filter_xfs_io_error
 	rm -f $file
 }
@@ -56,7 +56,7 @@ _require_prjquota $SCRATCH_DEV
 
 mkdir $SCRATCH_MNT/dir
 $QUOTA_CMD -x -c 'project -s test' $SCRATCH_MNT >>$seqres.full 2>&1
-$QUOTA_CMD -x -c 'limit -p bsoft=1m bhard=2m test' $SCRATCH_MNT
+$QUOTA_CMD -x -c 'limit -p bsoft=10m bhard=20m test' $SCRATCH_MNT
 
 # test the Project inheritance bit is a directory only flag, and it's set on
 # directory by default. Expect no complain about "project inheritance flag is

[PATCH 04/12] common/rc: capture dmesg when oom kills happen

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Capture the dmesg output if the OOM killer is invoked during fstests.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/rc |    1 +
 1 file changed, 1 insertion(+)


diff --git a/common/rc b/common/rc
index 2ee46e5101e168..70a0f1d1c6acd9 100644
--- a/common/rc
+++ b/common/rc
@@ -4538,6 +4538,7 @@ _check_dmesg()
 	     -e "INFO: possible circular locking dependency detected" \
 	     -e "general protection fault:" \
 	     -e "BUG .* remaining" \
+	     -e "oom-kill" \
 	     -e "UBSAN:" \
 	     $seqres.dmesg
 	if [ $? -eq 0 ]; then

[PATCH 05/12] generic/562: handle ENOSPC while cloning gracefully

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

This test creates a couple of patterned files on a tiny filesystem,
fragments the free space, clones one patterned file to the other, and
checks that the entire file was cloned.

However, this test doesn't work on a 64k fsblock filesystem because
we've used up all the free space reservation for the rmapbt, and that
causes the FICLONE to error out with ENOSPC partway through.  Hence we
need to detect the ENOSPC and _notrun the test.

That said, it turns out that XFS has been silently dropping error codes
if we managed to make some progress cloning extents.  That's ok if the
operation has REMAP_FILE_CAN_SHORTEN like copy_file_range does, but
FICLONE/FICLONERANGE do not permit partial results, so the dropped error
codes is actually an error.

Therefore, this testcase now becomes a regression test for the patch to
fix that.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/generic/562 |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)


diff --git a/tests/generic/562 b/tests/generic/562
index 91360c4154a6a2..62899945003513 100755
--- a/tests/generic/562
+++ b/tests/generic/562
@@ -15,6 +15,9 @@ _begin_fstest auto clone punch
 . ./common/filter
 . ./common/reflink
 
+test "$FSTYP" = "xfs" && \
+	_fixed_by_kernel_commit XXXXXXXXXX "xfs: don't drop errno values when we fail to ficlone the entire range"
+
 _require_scratch_reflink
 _require_test_program "punch-alternating"
 _require_xfs_io_command "fpunch"
@@ -48,8 +51,11 @@ while true; do
 done
 
 # Now clone file bar into file foo. This is supposed to succeed and not fail
-# with ENOSPC for example.
-_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full
+# with ENOSPC for example.  However, XFS will sometimes run out of space.
+_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full 2> $tmp.err
+cat $tmp.err
+grep -q 'No space left on device' $tmp.err && \
+	_notrun "ran out of space while cloning"
 
 # Unmount and mount the filesystem again to verify the operation was durably
 # persisted.

Re: [PATCH 05/12] generic/562: handle ENOSPC while cloning gracefully

[Filipe Manana]

On Mon, Nov 18, 2024 at 11:03 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> From: Darrick J. Wong <djwong@kernel.org>
>
> This test creates a couple of patterned files on a tiny filesystem,
> fragments the free space, clones one patterned file to the other, and
> checks that the entire file was cloned.
>
> However, this test doesn't work on a 64k fsblock filesystem because
> we've used up all the free space reservation for the rmapbt, and that
> causes the FICLONE to error out with ENOSPC partway through.  Hence we
> need to detect the ENOSPC and _notrun the test.
>
> That said, it turns out that XFS has been silently dropping error codes
> if we managed to make some progress cloning extents.  That's ok if the
> operation has REMAP_FILE_CAN_SHORTEN like copy_file_range does, but
> FICLONE/FICLONERANGE do not permit partial results, so the dropped error
> codes is actually an error.
>
> Therefore, this testcase now becomes a regression test for the patch to
> fix that.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/generic/562 |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
>
> diff --git a/tests/generic/562 b/tests/generic/562
> index 91360c4154a6a2..62899945003513 100755
> --- a/tests/generic/562
> +++ b/tests/generic/562
> @@ -15,6 +15,9 @@ _begin_fstest auto clone punch
>  . ./common/filter
>  . ./common/reflink
>
> +test "$FSTYP" = "xfs" && \
> +       _fixed_by_kernel_commit XXXXXXXXXX "xfs: don't drop errno values when we fail to ficlone the entire range"
> +
>  _require_scratch_reflink
>  _require_test_program "punch-alternating"
>  _require_xfs_io_command "fpunch"
> @@ -48,8 +51,11 @@ while true; do
>  done
>
>  # Now clone file bar into file foo. This is supposed to succeed and not fail
> -# with ENOSPC for example.
> -_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full
> +# with ENOSPC for example.  However, XFS will sometimes run out of space.
> +_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full 2> $tmp.err
> +cat $tmp.err
> +grep -q 'No space left on device' $tmp.err && \
> +       _notrun "ran out of space while cloning"

This defeats the original purpose of the test, which was to verify
btrfs didn't fail with -ENOSPC (or any other error).

If XFS has an ENOSPC issue in some cases, and it's not fixable, why
not make it _notrun only if it's XFS that is being tested?

Thanks.


>
>  # Unmount and mount the filesystem again to verify the operation was durably
>  # persisted.
>
>

Re: [PATCH 05/12] generic/562: handle ENOSPC while cloning gracefully

[Darrick J. Wong]

On Tue, Nov 19, 2024 at 12:17:56AM +0000, Filipe Manana wrote:
> On Mon, Nov 18, 2024 at 11:03 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > This test creates a couple of patterned files on a tiny filesystem,
> > fragments the free space, clones one patterned file to the other, and
> > checks that the entire file was cloned.
> >
> > However, this test doesn't work on a 64k fsblock filesystem because
> > we've used up all the free space reservation for the rmapbt, and that
> > causes the FICLONE to error out with ENOSPC partway through.  Hence we
> > need to detect the ENOSPC and _notrun the test.
> >
> > That said, it turns out that XFS has been silently dropping error codes
> > if we managed to make some progress cloning extents.  That's ok if the
> > operation has REMAP_FILE_CAN_SHORTEN like copy_file_range does, but
> > FICLONE/FICLONERANGE do not permit partial results, so the dropped error
> > codes is actually an error.
> >
> > Therefore, this testcase now becomes a regression test for the patch to
> > fix that.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/generic/562 |   10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> >
> > diff --git a/tests/generic/562 b/tests/generic/562
> > index 91360c4154a6a2..62899945003513 100755
> > --- a/tests/generic/562
> > +++ b/tests/generic/562
> > @@ -15,6 +15,9 @@ _begin_fstest auto clone punch
> >  . ./common/filter
> >  . ./common/reflink
> >
> > +test "$FSTYP" = "xfs" && \
> > +       _fixed_by_kernel_commit XXXXXXXXXX "xfs: don't drop errno values when we fail to ficlone the entire range"
> > +
> >  _require_scratch_reflink
> >  _require_test_program "punch-alternating"
> >  _require_xfs_io_command "fpunch"
> > @@ -48,8 +51,11 @@ while true; do
> >  done
> >
> >  # Now clone file bar into file foo. This is supposed to succeed and not fail
> > -# with ENOSPC for example.
> > -_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full
> > +# with ENOSPC for example.  However, XFS will sometimes run out of space.
> > +_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full 2> $tmp.err
> > +cat $tmp.err
> > +grep -q 'No space left on device' $tmp.err && \
> > +       _notrun "ran out of space while cloning"
> 
> This defeats the original purpose of the test, which was to verify
> btrfs didn't fail with -ENOSPC (or any other error).
> 
> If XFS has an ENOSPC issue in some cases, and it's not fixable, why
> not make it _notrun only if it's XFS that is being tested?

Ok, will do.  In the case of xfs, we don't let you share data if the
allocation group it's in is more than about 90% full.

--D

> Thanks.
> 
> 
> >
> >  # Unmount and mount the filesystem again to verify the operation was durably
> >  # persisted.
> >
> >
> 

Re: [PATCH 05/12] generic/562: handle ENOSPC while cloning gracefully

[Darrick J. Wong]

On Mon, Nov 18, 2024 at 04:31:31PM -0800, Darrick J. Wong wrote:
> On Tue, Nov 19, 2024 at 12:17:56AM +0000, Filipe Manana wrote:
> > On Mon, Nov 18, 2024 at 11:03 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > From: Darrick J. Wong <djwong@kernel.org>
> > >
> > > This test creates a couple of patterned files on a tiny filesystem,
> > > fragments the free space, clones one patterned file to the other, and
> > > checks that the entire file was cloned.
> > >
> > > However, this test doesn't work on a 64k fsblock filesystem because
> > > we've used up all the free space reservation for the rmapbt, and that
> > > causes the FICLONE to error out with ENOSPC partway through.  Hence we
> > > need to detect the ENOSPC and _notrun the test.
> > >
> > > That said, it turns out that XFS has been silently dropping error codes
> > > if we managed to make some progress cloning extents.  That's ok if the
> > > operation has REMAP_FILE_CAN_SHORTEN like copy_file_range does, but
> > > FICLONE/FICLONERANGE do not permit partial results, so the dropped error
> > > codes is actually an error.
> > >
> > > Therefore, this testcase now becomes a regression test for the patch to
> > > fix that.
> > >
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  tests/generic/562 |   10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > >
> > > diff --git a/tests/generic/562 b/tests/generic/562
> > > index 91360c4154a6a2..62899945003513 100755
> > > --- a/tests/generic/562
> > > +++ b/tests/generic/562
> > > @@ -15,6 +15,9 @@ _begin_fstest auto clone punch
> > >  . ./common/filter
> > >  . ./common/reflink
> > >
> > > +test "$FSTYP" = "xfs" && \
> > > +       _fixed_by_kernel_commit XXXXXXXXXX "xfs: don't drop errno values when we fail to ficlone the entire range"
> > > +
> > >  _require_scratch_reflink
> > >  _require_test_program "punch-alternating"
> > >  _require_xfs_io_command "fpunch"
> > > @@ -48,8 +51,11 @@ while true; do
> > >  done
> > >
> > >  # Now clone file bar into file foo. This is supposed to succeed and not fail
> > > -# with ENOSPC for example.
> > > -_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full
> > > +# with ENOSPC for example.  However, XFS will sometimes run out of space.
> > > +_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full 2> $tmp.err
> > > +cat $tmp.err
> > > +grep -q 'No space left on device' $tmp.err && \
> > > +       _notrun "ran out of space while cloning"
> > 
> > This defeats the original purpose of the test, which was to verify
> > btrfs didn't fail with -ENOSPC (or any other error).
> > 
> > If XFS has an ENOSPC issue in some cases, and it's not fixable, why
> > not make it _notrun only if it's XFS that is being tested?
> 
> Ok, will do.  In the case of xfs, we don't let you share data if the
> allocation group it's in is more than about 90% full.

Er... scratch that response :)

It's true that XFS doesn't let you share data if the AG is more than 90%
full, but this test exposed a data corruption vector in 4.20-6.12 if you
did run out of space.  Granted you'd only hit that if you had 64k block
size and a fairly small disk (~33GB) but still, that's reason enough to
keep running on XFS even if it runs out of space.  ENOSPC is better than
data corruption.

--D

> --D
> 
> > Thanks.
> > 
> > 
> > >
> > >  # Unmount and mount the filesystem again to verify the operation was durably
> > >  # persisted.
> > >
> > >
> > 
> 

[PATCH 06/12] xfs/163: skip test if we can't shrink due to enospc issues

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If this test fails due to insufficient space, skip this test.  This can
happen if a realtime volume is enabled on the filesystem and we cannot
shrink due to the rtbitmap.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/163 |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)


diff --git a/tests/xfs/163 b/tests/xfs/163
index 2bd94060222f96..75c3113dc2fd03 100755
--- a/tests/xfs/163
+++ b/tests/xfs/163
@@ -17,13 +17,20 @@ _begin_fstest auto quick growfs shrinkfs
 
 test_shrink()
 {
-	$XFS_GROWFS_PROG -D"$1" $SCRATCH_MNT >> $seqres.full 2>&1
+	$XFS_GROWFS_PROG -D"$1" $SCRATCH_MNT &> $tmp.growfs
 	ret=$?
 
 	_scratch_unmount
 	_check_scratch_fs
 	_scratch_mount
 
+	# If we couldn't shrink the filesystem due to lack of space, we're
+	# done with this test.
+	[ $1 -ne $dblocks ] && \
+		grep -q 'No space left on device' $tmp.growfs && \
+		_notrun "Could not shrink due to lack of space"
+	cat $tmp.growfs >> $seqres.full
+
 	$XFS_INFO_PROG $SCRATCH_MNT 2>&1 | _filter_mkfs 2>$tmp.growfs >/dev/null
 	. $tmp.growfs
 	[ $ret -eq 0 -a $1 -eq $dblocks ]

Re: [PATCH 06/12] xfs/163: skip test if we can't shrink due to enospc issues

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 07/12] xfs/009: allow logically contiguous preallocations

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

The new rtgroups feature implements a simplistic rotor to pick the
rtgroup for an initial allocation to a file.  This causes test failures
if the preallocations are spread across two rtgroups, which happens if
there are more subtests than rtgroups.

One way to fix this would be to reset the rotor then each subtest starts
allocating from rtgroup 0, but the only way to do that is to cycle the
scratch mount, which is a bit gross.

Instead, report logically contiguous mappings as a single mapping even
if the physical space is not contiguous.  Unfortunately, there's not
enough context in the comments to know if the test actually was checking
for physical contiguity?  Or if this is just an exerciser of the old
preallocation calls, and it's fine as long as the file ranges are mapped
(or unmapped) as desired.

Messing with some awk is a lot cheaper than umount/mount cycling.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 tests/xfs/009 |   29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)


diff --git a/tests/xfs/009 b/tests/xfs/009
index dde505f079f4f8..bb42ce32490df5 100755
--- a/tests/xfs/009
+++ b/tests/xfs/009
@@ -49,13 +49,26 @@ _filesize()
 _block_filter()
 {
 	$AWK_PROG -v bsize="$bsize" '
+	BEGIN {
+		br_pos = 0
+		br_len = 0
+	}
+	function dump_blockrange() {
+		if (br_len == 0)
+			return
+		printf("        [%d,%d]: BLOCKRANGE\n", br_pos, br_len)
+		br_pos = 0
+		br_len = 0
+	}
 	/blocksize/ {
+		dump_blockrange()
 		printf("    blocksize BSIZE\n")
 
 		next
 	}
 
 	/CMD/ {
+		dump_blockrange()
 		split($3, off, "=")
 		offset = strtonum(off[2])
 		if (offset != -1)
@@ -72,6 +85,7 @@ _block_filter()
 	}
 
 	/MAP/ {
+		dump_blockrange()
 		split($2, off, "=")
 		offset = strtonum(off[2])
 		if (offset != -1)
@@ -90,6 +104,7 @@ _block_filter()
 	}
 
 	/TRUNCATE/ {
+		dump_blockrange()
 		split($2, off, "=")
 		offset = strtonum(off[2]) / bsize
 
@@ -99,16 +114,28 @@ _block_filter()
 	}
 
 	/\[[0-9]+,[0-9]+\]:/ {
-		printf("        %s BLOCKRANGE\n", $1)
+		rangestr = gensub(/\[([0-9]+),([0-9]+)\]:/, "\\1,\\2", "g", $1);
+		split(rangestr, off, ",")
+		if (br_pos + br_len == off[1]) {
+			br_len += off[2];
+		} else {
+			dump_blockrange()
+			br_pos = off[1];
+			br_len = off[2];
+		}
 
 		next
 	}
 
 	{
+		dump_blockrange()
 		print
 
 		next
 	}
+	END {
+		dump_blockrange()
+	}
 	'
 }
 

Re: [PATCH 07/12] xfs/009: allow logically contiguous preallocations

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 08/12] generic/251: use sentinel files to kill the fstrim loop

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Apparently the subshell kill doesn't always take, and then the test runs
for hours and hours because nothing stops it.  Instead, use a sentinel
file to detect when fstrim_loop should stop execing background fstrims.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 tests/generic/251 |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)


diff --git a/tests/generic/251 b/tests/generic/251
index b432fb11937911..d59e91c3e0a33a 100755
--- a/tests/generic/251
+++ b/tests/generic/251
@@ -125,12 +125,15 @@ fstrim_loop()
 			wait $fpid
 		fi
 		while [ $start -lt $fsize ] ; do
+			test -s $tmp.fstrim_loop || break
 			$FSTRIM_PROG -m ${minlen}k -o ${start}k -l ${step}k $SCRATCH_MNT &
 			fpid=$!
 			wait $fpid
 			start=$(( $start + $step ))
 		done
+		test -s $tmp.fstrim_loop || break
 	done
+	rm -f $tmp.fstrim_loop
 }
 
 function check_sums() {
@@ -188,6 +191,7 @@ find -P . -xdev -type f -print0 | xargs -0 md5sum | sort -o $tmp/content.sums
 
 echo -n "Running the test: "
 pids=""
+echo run > $tmp.fstrim_loop
 fstrim_loop &
 fstrim_pid=$!
 p=1
@@ -199,8 +203,10 @@ done
 echo "done."
 
 wait $pids
-kill $fstrim_pid
-wait $fstrim_pid
+truncate -s 0 $tmp.fstrim_loop
+while test -e $tmp.fstrim_loop; do
+	sleep 1
+done
 
 status=0
 

Re: [PATCH 08/12] generic/251: use sentinel files to kill the fstrim loop

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 09/12] generic/251: constrain runtime via time/load/soak factors

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

On my test fleet, this test can run for well in excess of 20 minutes:

   613 generic/251
   616 generic/251
   624 generic/251
   630 generic/251
   634 generic/251
   652 generic/251
   675 generic/251
   749 generic/251
   777 generic/251
   808 generic/251
   832 generic/251
   946 generic/251
  1082 generic/251
  1221 generic/251
  1241 generic/251
  1254 generic/251
  1305 generic/251
  1366 generic/251
  1646 generic/251
  1936 generic/251
  1952 generic/251
  2358 generic/251
  4359 generic/251
  5325 generic/251
 34046 generic/251

because it hardcodes 20 threads and 10 copies.  It's not great to have a
test that results in a significant fraction of the total test runtime.
Fix the looping and load on this test to use LOAD and TIME_FACTOR to
scale up its operations, along with the usual SOAK_DURATION override.
That brings the default runtime down to less than a minute.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 tests/generic/251 |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)


diff --git a/tests/generic/251 b/tests/generic/251
index d59e91c3e0a33a..b4ddda10cef403 100755
--- a/tests/generic/251
+++ b/tests/generic/251
@@ -15,7 +15,6 @@ _begin_fstest ioctl trim auto
 tmp=`mktemp -d`
 trap "_cleanup; exit \$status" 0 1 3
 trap "_destroy; exit \$status" 2 15
-chpid=0
 mypid=$$
 
 # Import common functions.
@@ -151,29 +150,28 @@ function check_sums() {
 
 function run_process() {
 	local p=$1
-	repeat=10
+	if [ -n "$SOAK_DURATION" ]; then
+		local duration="$SOAK_DURATION"
+	else
+		local duration="$((30 * TIME_FACTOR))"
+	fi
+	local stopat="$(( $(date +%s) + duration))"
 
-	sleep $((5*$p))s &
-	export chpid=$! && wait $chpid &> /dev/null
-	chpid=0
-
-	while [ $repeat -gt 0 ]; do
+	sleep $((5*$p))s
 
+	while [ "$(date +%s)" -lt "$stopat" ]; do
 		# Remove old directories.
 		rm -rf $SCRATCH_MNT/$p
-		export chpid=$! && wait $chpid &> /dev/null
 
 		# Copy content -> partition.
 		mkdir $SCRATCH_MNT/$p
 		cp -axT $content/ $SCRATCH_MNT/$p/
-		export chpid=$! && wait $chpid &> /dev/null
 
 		check_sums
-		repeat=$(( $repeat - 1 ))
 	done
 }
 
-nproc=20
+nproc=$((4 * LOAD_FACTOR))
 
 # Copy $here to the scratch fs and make coipes of the replica.  The fstests
 # output (and hence $seqres.full) could be in $here, so we need to snapshot
@@ -194,11 +192,9 @@ pids=""
 echo run > $tmp.fstrim_loop
 fstrim_loop &
 fstrim_pid=$!
-p=1
-while [ $p -le $nproc ]; do
+for ((p = 1; p < nproc; p++)); do
 	run_process $p &
 	pids="$pids $!"
-	p=$(($p+1))
 done
 echo "done."
 

Re: [PATCH 09/12] generic/251: constrain runtime via time/load/soak factors

[Dave Chinner]

On Mon, Nov 18, 2024 at 03:03:43PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> On my test fleet, this test can run for well in excess of 20 minutes:
> 
>    613 generic/251
>    616 generic/251
>    624 generic/251
>    630 generic/251
>    634 generic/251
>    652 generic/251
>    675 generic/251
>    749 generic/251
>    777 generic/251
>    808 generic/251
>    832 generic/251
>    946 generic/251
>   1082 generic/251
>   1221 generic/251
>   1241 generic/251
>   1254 generic/251
>   1305 generic/251
>   1366 generic/251
>   1646 generic/251
>   1936 generic/251
>   1952 generic/251
>   2358 generic/251
>   4359 generic/251
>   5325 generic/251
>  34046 generic/251
> 
> because it hardcodes 20 threads and 10 copies.  It's not great to have a
> test that results in a significant fraction of the total test runtime.
> Fix the looping and load on this test to use LOAD and TIME_FACTOR to
> scale up its operations, along with the usual SOAK_DURATION override.
> That brings the default runtime down to less than a minute.
> 
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>

Question for you: Does your $here directory contain a .git subdir?

One of the causes of long runtime for me has been that $here might
only contain 30MB of files, but the .git subdir balloons to several
hundred MB over time, resulting is really long runtimes because it's
copying GBs of data from the .git subdir.

I have this patch in my tree:

--- a/tests/generic/251
+++ b/tests/generic/251
@@ -175,9 +175,12 @@ nproc=20
 # Copy $here to the scratch fs and make coipes of the replica.  The fstests
 # output (and hence $seqres.full) could be in $here, so we need to snapshot
 # $here before computing file checksums.
+#
+# $here/* as the files to copy so we avoid any .git directory that might be
+# much, much larger than the rest of the fstests source tree we are copying.
 content=$SCRATCH_MNT/orig
 mkdir -p $content
-cp -axT $here/ $content/
+cp -ax $here/* $content/

 mkdir -p $tmp

And that's made the runtime drop from (typically) 10-15 minutes
down to around 5 minutes....

Does this have any impact on the runtime on your test systems?

-Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 09/12] generic/251: constrain runtime via time/load/soak factors

[Christoph Hellwig]

On Tue, Nov 19, 2024 at 12:45:05PM +1100, Dave Chinner wrote:
> Question for you: Does your $here directory contain a .git subdir?
> 
> One of the causes of long runtime for me has been that $here might
> only contain 30MB of files, but the .git subdir balloons to several
> hundred MB over time, resulting is really long runtimes because it's
> copying GBs of data from the .git subdir.

Or the results/ directory when run in a persistent test VM like the
one for quick runs on my laptop.  I currently need to persistently
purge that for just this test.

> 
> I have this patch in my tree:
> 
> --- a/tests/generic/251
> +++ b/tests/generic/251
> @@ -175,9 +175,12 @@ nproc=20
>  # Copy $here to the scratch fs and make coipes of the replica.  The fstests
>  # output (and hence $seqres.full) could be in $here, so we need to snapshot
>  # $here before computing file checksums.
> +#
> +# $here/* as the files to copy so we avoid any .git directory that might be
> +# much, much larger than the rest of the fstests source tree we are copying.
>  content=$SCRATCH_MNT/orig
>  mkdir -p $content
> -cp -axT $here/ $content/
> +cp -ax $here/* $content/

Maybe we just need a way to generate more predictable file system
content?

Re: [PATCH 09/12] generic/251: constrain runtime via time/load/soak factors

[Darrick J. Wong]

On Mon, Nov 18, 2024 at 10:13:23PM -0800, Christoph Hellwig wrote:
> On Tue, Nov 19, 2024 at 12:45:05PM +1100, Dave Chinner wrote:
> > Question for you: Does your $here directory contain a .git subdir?
> > 
> > One of the causes of long runtime for me has been that $here might
> > only contain 30MB of files, but the .git subdir balloons to several
> > hundred MB over time, resulting is really long runtimes because it's
> > copying GBs of data from the .git subdir.
> 
> Or the results/ directory when run in a persistent test VM like the
> one for quick runs on my laptop.  I currently need to persistently
> purge that for just this test.
> 
> > 
> > I have this patch in my tree:
> > 
> > --- a/tests/generic/251
> > +++ b/tests/generic/251
> > @@ -175,9 +175,12 @@ nproc=20
> >  # Copy $here to the scratch fs and make coipes of the replica.  The fstests
> >  # output (and hence $seqres.full) could be in $here, so we need to snapshot
> >  # $here before computing file checksums.
> > +#
> > +# $here/* as the files to copy so we avoid any .git directory that might be
> > +# much, much larger than the rest of the fstests source tree we are copying.
> >  content=$SCRATCH_MNT/orig
> >  mkdir -p $content
> > -cp -axT $here/ $content/
> > +cp -ax $here/* $content/
> 
> Maybe we just need a way to generate more predictable file system
> content?

How about running fsstress for ~50000ops or so, to generate some test
files and directory tree?

--D

Re: [PATCH 09/12] generic/251: constrain runtime via time/load/soak factors

[Dave Chinner]

On Tue, Nov 19, 2024 at 07:45:20AM -0800, Darrick J. Wong wrote:
> On Mon, Nov 18, 2024 at 10:13:23PM -0800, Christoph Hellwig wrote:
> > On Tue, Nov 19, 2024 at 12:45:05PM +1100, Dave Chinner wrote:
> > > Question for you: Does your $here directory contain a .git subdir?
> > > 
> > > One of the causes of long runtime for me has been that $here might
> > > only contain 30MB of files, but the .git subdir balloons to several
> > > hundred MB over time, resulting is really long runtimes because it's
> > > copying GBs of data from the .git subdir.
> > 
> > Or the results/ directory when run in a persistent test VM like the
> > one for quick runs on my laptop.  I currently need to persistently
> > purge that for just this test.

Yeah, I use persistent VMs and that's why the .git dir grows...

> > > --- a/tests/generic/251
> > > +++ b/tests/generic/251
> > > @@ -175,9 +175,12 @@ nproc=20
> > >  # Copy $here to the scratch fs and make coipes of the replica.  The fstests
> > >  # output (and hence $seqres.full) could be in $here, so we need to snapshot
> > >  # $here before computing file checksums.
> > > +#
> > > +# $here/* as the files to copy so we avoid any .git directory that might be
> > > +# much, much larger than the rest of the fstests source tree we are copying.
> > >  content=$SCRATCH_MNT/orig
> > >  mkdir -p $content
> > > -cp -axT $here/ $content/
> > > +cp -ax $here/* $content/
> > 
> > Maybe we just need a way to generate more predictable file system
> > content?
> 
> How about running fsstress for ~50000ops or so, to generate some test
> files and directory tree?

Do we even need to do that? It's a set of small files distributed
over a few directories. There are few large files in the mix, so we
could just create a heap of 1-4 block files across a dozen or so
directories and get the same sort of data set to copy.

And given this observation, if we are generating the data set in the
first place, why use cp to copy it every time? Why not just have
each thread generate the data set on the fly?

# create a directory structure with numdirs directories and numfiles
# files per directory. Files are 0-3 blocks in length, space is
# allocated by fallocate to avoid needing to write data. Files are
# created concurrently across directories to create the data set as
# fast as possible.
create_files()
{
	local numdirs=$1
	local numfiles=$2
	local basedir=$3

	for ((i=0; i<$numdirs; i++)); do
		mkdir -p $basedir/$i
		for ((j=0; j<$numfiles; j++); do
			local len=$((RANDOM % 4))
			$XFS_IO_PROG -fc "falloc 0 ${len}b" $basedir/$i/$j
		done &
	done
	wait
}

-Dave

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 09/12] generic/251: constrain runtime via time/load/soak factors

[Darrick J. Wong]

On Wed, Nov 20, 2024 at 08:04:43AM +1100, Dave Chinner wrote:
> On Tue, Nov 19, 2024 at 07:45:20AM -0800, Darrick J. Wong wrote:
> > On Mon, Nov 18, 2024 at 10:13:23PM -0800, Christoph Hellwig wrote:
> > > On Tue, Nov 19, 2024 at 12:45:05PM +1100, Dave Chinner wrote:
> > > > Question for you: Does your $here directory contain a .git subdir?
> > > > 
> > > > One of the causes of long runtime for me has been that $here might
> > > > only contain 30MB of files, but the .git subdir balloons to several
> > > > hundred MB over time, resulting is really long runtimes because it's
> > > > copying GBs of data from the .git subdir.
> > > 
> > > Or the results/ directory when run in a persistent test VM like the
> > > one for quick runs on my laptop.  I currently need to persistently
> > > purge that for just this test.
> 
> Yeah, I use persistent VMs and that's why the .git dir grows...
> 
> > > > --- a/tests/generic/251
> > > > +++ b/tests/generic/251
> > > > @@ -175,9 +175,12 @@ nproc=20
> > > >  # Copy $here to the scratch fs and make coipes of the replica.  The fstests
> > > >  # output (and hence $seqres.full) could be in $here, so we need to snapshot
> > > >  # $here before computing file checksums.
> > > > +#
> > > > +# $here/* as the files to copy so we avoid any .git directory that might be
> > > > +# much, much larger than the rest of the fstests source tree we are copying.
> > > >  content=$SCRATCH_MNT/orig
> > > >  mkdir -p $content
> > > > -cp -axT $here/ $content/
> > > > +cp -ax $here/* $content/
> > > 
> > > Maybe we just need a way to generate more predictable file system
> > > content?
> > 
> > How about running fsstress for ~50000ops or so, to generate some test
> > files and directory tree?
> 
> Do we even need to do that? It's a set of small files distributed
> over a few directories. There are few large files in the mix, so we
> could just create a heap of 1-4 block files across a dozen or so
> directories and get the same sort of data set to copy.
> 
> And given this observation, if we are generating the data set in the
> first place, why use cp to copy it every time? Why not just have
> each thread generate the data set on the fly?

run_process compares the copies to the original to try to discover
places where written blocks got discarded, so they actually do need to
be copies.

/me suspects that this test is kinda bogus if the block device doesn't
set discard_zeroes_data because it won't trip on discard errors for
crappy sata ssds that don't actually clear the remapping tables until
minutes later.

--D

> # create a directory structure with numdirs directories and numfiles
> # files per directory. Files are 0-3 blocks in length, space is
> # allocated by fallocate to avoid needing to write data. Files are
> # created concurrently across directories to create the data set as
> # fast as possible.
> create_files()
> {
> 	local numdirs=$1
> 	local numfiles=$2
> 	local basedir=$3
> 
> 	for ((i=0; i<$numdirs; i++)); do
> 		mkdir -p $basedir/$i
> 		for ((j=0; j<$numfiles; j++); do
> 			local len=$((RANDOM % 4))
> 			$XFS_IO_PROG -fc "falloc 0 ${len}b" $basedir/$i/$j
> 		done &
> 	done
> 	wait
> }
> 
> -Dave
> 
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 09/12] generic/251: constrain runtime via time/load/soak factors

[Darrick J. Wong]

On Tue, Nov 19, 2024 at 12:45:05PM +1100, Dave Chinner wrote:
> On Mon, Nov 18, 2024 at 03:03:43PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > On my test fleet, this test can run for well in excess of 20 minutes:
> > 
> >    613 generic/251
> >    616 generic/251
> >    624 generic/251
> >    630 generic/251
> >    634 generic/251
> >    652 generic/251
> >    675 generic/251
> >    749 generic/251
> >    777 generic/251
> >    808 generic/251
> >    832 generic/251
> >    946 generic/251
> >   1082 generic/251
> >   1221 generic/251
> >   1241 generic/251
> >   1254 generic/251
> >   1305 generic/251
> >   1366 generic/251
> >   1646 generic/251
> >   1936 generic/251
> >   1952 generic/251
> >   2358 generic/251
> >   4359 generic/251
> >   5325 generic/251
> >  34046 generic/251
> > 
> > because it hardcodes 20 threads and 10 copies.  It's not great to have a
> > test that results in a significant fraction of the total test runtime.
> > Fix the looping and load on this test to use LOAD and TIME_FACTOR to
> > scale up its operations, along with the usual SOAK_DURATION override.
> > That brings the default runtime down to less than a minute.
> > 
> > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> 
> Question for you: Does your $here directory contain a .git subdir?
> 
> One of the causes of long runtime for me has been that $here might
> only contain 30MB of files, but the .git subdir balloons to several
> hundred MB over time, resulting is really long runtimes because it's
> copying GBs of data from the .git subdir.
> 
> I have this patch in my tree:
> 
> --- a/tests/generic/251
> +++ b/tests/generic/251
> @@ -175,9 +175,12 @@ nproc=20
>  # Copy $here to the scratch fs and make coipes of the replica.  The fstests
>  # output (and hence $seqres.full) could be in $here, so we need to snapshot
>  # $here before computing file checksums.
> +#
> +# $here/* as the files to copy so we avoid any .git directory that might be
> +# much, much larger than the rest of the fstests source tree we are copying.
>  content=$SCRATCH_MNT/orig
>  mkdir -p $content
> -cp -axT $here/ $content/
> +cp -ax $here/* $content/
> 
>  mkdir -p $tmp
> 
> And that's made the runtime drop from (typically) 10-15 minutes
> down to around 5 minutes....
> 
> Does this have any impact on the runtime on your test systems?

Nope, I do vpath builds (sort of) so there's no .git history getting
sucked up by generic/251.  The fstests directory on the test VMs is
~34MB spread across ~4800 files.

--D

> -Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
> 

[PATCH 10/12] common/rc: _scratch_mkfs_sized supports extra arguments

[Darrick J. Wong]

From: Zorro Lang <zlang@kernel.org>

To give more arguments to _scratch_mkfs_sized, we generally do as:

  MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size

to give "-L oldlabel" to it. But if _scratch_mkfs_sized fails, it
will get rid of the whole MKFS_OPTIONS and try to mkfs again.
Likes:

  ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
  ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **

But that's not the fault of "-L oldlabel". So for keeping the mkfs
options ("-L oldlabel") we need, we'd better to let the
scratch_mkfs_sized to support extra arguments, rather than using
global MKFS_OPTIONS.

Signed-off-by: Zorro Lang <zlang@kernel.org>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
[djwong: fix string quoting issues]
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 common/rc |   34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)


diff --git a/common/rc b/common/rc
index 70a0f1d1c6acd9..6acacbe4c88eea 100644
--- a/common/rc
+++ b/common/rc
@@ -1026,11 +1026,13 @@ _small_fs_size_mb()
 }
 
 # Create fs of certain size on scratch device
-# _try_scratch_mkfs_sized <size in bytes> [optional blocksize]
+# _try_scratch_mkfs_sized <size in bytes> [optional blocksize] [other options]
 _try_scratch_mkfs_sized()
 {
 	local fssize=$1
-	local blocksize=$2
+	shift
+	local blocksize=$1
+	shift
 	local def_blksz
 	local blocksize_opt
 	local rt_ops
@@ -1094,10 +1096,10 @@ _try_scratch_mkfs_sized()
 		# don't override MKFS_OPTIONS that set a block size.
 		echo $MKFS_OPTIONS |grep -E -q "b\s*size="
 		if [ $? -eq 0 ]; then
-			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops
+			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops "$@"
 		else
 			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops \
-				-b size=$blocksize
+				-b size=$blocksize "$@"
 		fi
 		;;
 	ext2|ext3|ext4)
@@ -1108,7 +1110,7 @@ _try_scratch_mkfs_sized()
 				_notrun "Could not make scratch logdev"
 			MKFS_OPTIONS="$MKFS_OPTIONS -J device=$SCRATCH_LOGDEV"
 		fi
-		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
+		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
 		;;
 	gfs2)
 		# mkfs.gfs2 doesn't automatically shrink journal files on small
@@ -1123,13 +1125,13 @@ _try_scratch_mkfs_sized()
 			(( journal_size >= min_journal_size )) || journal_size=$min_journal_size
 			MKFS_OPTIONS="-J $journal_size $MKFS_OPTIONS"
 		fi
-		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize $SCRATCH_DEV $blocks
+		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize "$@" $SCRATCH_DEV $blocks
 		;;
 	ocfs2)
-		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
+		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
 		;;
 	udf)
-		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
+		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
 		;;
 	btrfs)
 		local mixed_opt=
@@ -1137,33 +1139,33 @@ _try_scratch_mkfs_sized()
 		# the device is not zoned. Ref: btrfs-progs: btrfs_min_dev_size()
 		(( fssize < $((256 * 1024 * 1024)) )) &&
 			! _scratch_btrfs_is_zoned && mixed_opt='--mixed'
-		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV
+		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize "$@" $SCRATCH_DEV
 		;;
 	jfs)
-		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS $SCRATCH_DEV $blocks
+		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS "$@" $SCRATCH_DEV $blocks
 		;;
 	reiserfs)
-		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
+		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
 		;;
 	reiser4)
 		# mkfs.resier4 requires size in KB as input for creating filesystem
-		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize $SCRATCH_DEV \
+		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize "$@" $SCRATCH_DEV \
 				   `expr $fssize / 1024`
 		;;
 	f2fs)
 		# mkfs.f2fs requires # of sectors as an input for the size
 		local sector_size=`blockdev --getss $SCRATCH_DEV`
-		$MKFS_F2FS_PROG $MKFS_OPTIONS $SCRATCH_DEV `expr $fssize / $sector_size`
+		$MKFS_F2FS_PROG $MKFS_OPTIONS "$@" $SCRATCH_DEV `expr $fssize / $sector_size`
 		;;
 	tmpfs)
 		local free_mem=`_free_memory_bytes`
 		if [ "$free_mem" -lt "$fssize" ] ; then
 		   _notrun "Not enough memory ($free_mem) for tmpfs with $fssize bytes"
 		fi
-		export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
+		export MOUNT_OPTIONS="-o size=$fssize "$@" $TMPFS_MOUNT_OPTIONS"
 		;;
 	bcachefs)
-		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt $SCRATCH_DEV
+		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt "$@" $SCRATCH_DEV
 		;;
 	*)
 		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized"
@@ -1173,7 +1175,7 @@ _try_scratch_mkfs_sized()
 
 _scratch_mkfs_sized()
 {
-	_try_scratch_mkfs_sized $* || _notrun "_scratch_mkfs_sized failed with ($*)"
+	_try_scratch_mkfs_sized "$@" || _notrun "_scratch_mkfs_sized failed with ($*)"
 }
 
 # Emulate an N-data-disk stripe w/ various stripe units

[PATCH 11/12] xfs/157: do not drop necessary mkfs options

[Darrick J. Wong]

From: Zorro Lang <zlang@kernel.org>

To give the test option "-L oldlabel" to _scratch_mkfs_sized, xfs/157
does:

  MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size

but the _scratch_mkfs_sized trys to keep the $fs_size, when mkfs
fails with incompatible $MKFS_OPTIONS options, likes this:

  ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
  ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **

but the "-L oldlabel" is necessary, we shouldn't drop it. To avoid
that, we give the "-L oldlabel" to _scratch_mkfs_sized through
function parameters, not through global MKFS_OPTIONS.

Signed-off-by: Zorro Lang <zlang@kernel.org>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
[djwong: fix more string quoting issues]
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 tests/xfs/157 |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


diff --git a/tests/xfs/157 b/tests/xfs/157
index 9b5badbaeb3c76..e102a5a10abe4b 100755
--- a/tests/xfs/157
+++ b/tests/xfs/157
@@ -66,8 +66,7 @@ scenario() {
 }
 
 check_label() {
-	MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \
-		>> $seqres.full
+	_scratch_mkfs_sized "$fs_size" "" -L oldlabel >> $seqres.full 2>&1
 	_scratch_xfs_db -c label
 	_scratch_xfs_admin -L newlabel "$@" >> $seqres.full
 	_scratch_xfs_db -c label

Re: [PATCH 11/12] xfs/157: do not drop necessary mkfs options

[Zorro Lang]

On Mon, Nov 18, 2024 at 03:04:15PM -0800, Darrick J. Wong wrote:
> From: Zorro Lang <zlang@kernel.org>
> 
> To give the test option "-L oldlabel" to _scratch_mkfs_sized, xfs/157
> does:
> 
>   MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size
> 
> but the _scratch_mkfs_sized trys to keep the $fs_size, when mkfs
> fails with incompatible $MKFS_OPTIONS options, likes this:
> 
>   ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
>   ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **
> 
> but the "-L oldlabel" is necessary, we shouldn't drop it. To avoid
> that, we give the "-L oldlabel" to _scratch_mkfs_sized through
> function parameters, not through global MKFS_OPTIONS.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> [djwong: fix more string quoting issues]
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---

Oh, you helped to merge the patchset at here. Thanks :)

>  tests/xfs/157 |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> 
> diff --git a/tests/xfs/157 b/tests/xfs/157
> index 9b5badbaeb3c76..e102a5a10abe4b 100755
> --- a/tests/xfs/157
> +++ b/tests/xfs/157
> @@ -66,8 +66,7 @@ scenario() {
>  }
>  
>  check_label() {
> -	MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \
> -		>> $seqres.full
> +	_scratch_mkfs_sized "$fs_size" "" -L oldlabel >> $seqres.full 2>&1
>  	_scratch_xfs_db -c label
>  	_scratch_xfs_admin -L newlabel "$@" >> $seqres.full
>  	_scratch_xfs_db -c label
> 

[PATCH 12/12] xfs/157: fix test failures when MKFS_OPTIONS has -L options

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Zorro reports that this test fails if the test runner set an -L (label)
option in MKFS_OPTIONS.  Fix the test to work around this with a bunch
of horrid sed filtering magic.  It's probably not *critical* to make
this test test work with random labels, but it'd be nice not to lose
them.

Cc: <fstests@vger.kernel.org> # v2024.10.14
Fixes: 2f7e1b8a6f09b6 ("xfs/157,xfs/547,xfs/548: switch to using _scratch_mkfs_sized")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/157 |   29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)


diff --git a/tests/xfs/157 b/tests/xfs/157
index e102a5a10abe4b..0c21786e389695 100755
--- a/tests/xfs/157
+++ b/tests/xfs/157
@@ -65,9 +65,34 @@ scenario() {
 	SCRATCH_RTDEV=$orig_rtdev
 }
 
+extract_mkfs_label() {
+	set -- $MKFS_OPTIONS
+	local in_l
+
+	for arg in "$@"; do
+		if [ "$in_l" = "1" ]; then
+			echo "$arg"
+			return 0
+		elif [ "$arg" = "-L" ]; then
+			in_l=1
+		fi
+	done
+	return 1
+}
+
 check_label() {
-	_scratch_mkfs_sized "$fs_size" "" -L oldlabel >> $seqres.full 2>&1
-	_scratch_xfs_db -c label
+	local existing_label
+	local filter
+
+	# Handle -L somelabel being set in MKFS_OPTIONS
+	if existing_label="$(extract_mkfs_label)"; then
+		filter=(sed -e "s|$existing_label|oldlabel|g")
+		_scratch_mkfs_sized $fs_size >> $seqres.full
+	else
+		filter=(cat)
+		_scratch_mkfs_sized "$fs_size" "" -L oldlabel >> $seqres.full 2>&1
+	fi
+	_scratch_xfs_db -c label | "${filter[@]}"
 	_scratch_xfs_admin -L newlabel "$@" >> $seqres.full
 	_scratch_xfs_db -c label
 	_scratch_xfs_repair -n &>> $seqres.full || echo "Check failed?"

Re: [PATCH 12/12] xfs/157: fix test failures when MKFS_OPTIONS has -L options

[Zorro Lang]

On Mon, Nov 18, 2024 at 03:04:31PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Zorro reports that this test fails if the test runner set an -L (label)
> option in MKFS_OPTIONS.  Fix the test to work around this with a bunch

I didn't hit the xfs/157 failure by setting "-L label" in MKFS_OPTIONS,
I set MKFS_OPTIONS="-m rmapbt=1" in local.config, then "-m rmapbt=1" is
conflict with rtdev, that cause the "-L oldlabel" be dropped by
_scratch_mkfs_sized.

I don't mind having this "xfs/157 enhancement" patch. But as we've talked,
I don't think any testers would like to write MKFS_OPTIONS="-L label" in
local.config. So this patch might not be necessary. What do you think?

Thanks,
Zorro

> of horrid sed filtering magic.  It's probably not *critical* to make
> this test test work with random labels, but it'd be nice not to lose
> them.
> 
> Cc: <fstests@vger.kernel.org> # v2024.10.14
> Fixes: 2f7e1b8a6f09b6 ("xfs/157,xfs/547,xfs/548: switch to using _scratch_mkfs_sized")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/xfs/157 |   29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/tests/xfs/157 b/tests/xfs/157
> index e102a5a10abe4b..0c21786e389695 100755
> --- a/tests/xfs/157
> +++ b/tests/xfs/157
> @@ -65,9 +65,34 @@ scenario() {
>  	SCRATCH_RTDEV=$orig_rtdev
>  }
>  
> +extract_mkfs_label() {
> +	set -- $MKFS_OPTIONS
> +	local in_l
> +
> +	for arg in "$@"; do
> +		if [ "$in_l" = "1" ]; then
> +			echo "$arg"
> +			return 0
> +		elif [ "$arg" = "-L" ]; then
> +			in_l=1
> +		fi
> +	done
> +	return 1
> +}
> +
>  check_label() {
> -	_scratch_mkfs_sized "$fs_size" "" -L oldlabel >> $seqres.full 2>&1
> -	_scratch_xfs_db -c label
> +	local existing_label
> +	local filter
> +
> +	# Handle -L somelabel being set in MKFS_OPTIONS
> +	if existing_label="$(extract_mkfs_label)"; then
> +		filter=(sed -e "s|$existing_label|oldlabel|g")
> +		_scratch_mkfs_sized $fs_size >> $seqres.full
> +	else
> +		filter=(cat)
> +		_scratch_mkfs_sized "$fs_size" "" -L oldlabel >> $seqres.full 2>&1
> +	fi
> +	_scratch_xfs_db -c label | "${filter[@]}"
>  	_scratch_xfs_admin -L newlabel "$@" >> $seqres.full
>  	_scratch_xfs_db -c label
>  	_scratch_xfs_repair -n &>> $seqres.full || echo "Check failed?"
> 

Re: [PATCH 12/12] xfs/157: fix test failures when MKFS_OPTIONS has -L options

[Darrick J. Wong]

On Thu, Nov 21, 2024 at 06:17:12PM +0800, Zorro Lang wrote:
> On Mon, Nov 18, 2024 at 03:04:31PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Zorro reports that this test fails if the test runner set an -L (label)
> > option in MKFS_OPTIONS.  Fix the test to work around this with a bunch
> 
> I didn't hit the xfs/157 failure by setting "-L label" in MKFS_OPTIONS,
> I set MKFS_OPTIONS="-m rmapbt=1" in local.config, then "-m rmapbt=1" is
> conflict with rtdev, that cause the "-L oldlabel" be dropped by
> _scratch_mkfs_sized.
> 
> I don't mind having this "xfs/157 enhancement" patch. But as we've talked,
> I don't think any testers would like to write MKFS_OPTIONS="-L label" in
> local.config. So this patch might not be necessary. What do you think?

Yeah, I guess I will drop it then.

--D

> Thanks,
> Zorro
> 
> > of horrid sed filtering magic.  It's probably not *critical* to make
> > this test test work with random labels, but it'd be nice not to lose
> > them.
> > 
> > Cc: <fstests@vger.kernel.org> # v2024.10.14
> > Fixes: 2f7e1b8a6f09b6 ("xfs/157,xfs/547,xfs/548: switch to using _scratch_mkfs_sized")
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/xfs/157 |   29 +++++++++++++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/tests/xfs/157 b/tests/xfs/157
> > index e102a5a10abe4b..0c21786e389695 100755
> > --- a/tests/xfs/157
> > +++ b/tests/xfs/157
> > @@ -65,9 +65,34 @@ scenario() {
> >  	SCRATCH_RTDEV=$orig_rtdev
> >  }
> >  
> > +extract_mkfs_label() {
> > +	set -- $MKFS_OPTIONS
> > +	local in_l
> > +
> > +	for arg in "$@"; do
> > +		if [ "$in_l" = "1" ]; then
> > +			echo "$arg"
> > +			return 0
> > +		elif [ "$arg" = "-L" ]; then
> > +			in_l=1
> > +		fi
> > +	done
> > +	return 1
> > +}
> > +
> >  check_label() {
> > -	_scratch_mkfs_sized "$fs_size" "" -L oldlabel >> $seqres.full 2>&1
> > -	_scratch_xfs_db -c label
> > +	local existing_label
> > +	local filter
> > +
> > +	# Handle -L somelabel being set in MKFS_OPTIONS
> > +	if existing_label="$(extract_mkfs_label)"; then
> > +		filter=(sed -e "s|$existing_label|oldlabel|g")
> > +		_scratch_mkfs_sized $fs_size >> $seqres.full
> > +	else
> > +		filter=(cat)
> > +		_scratch_mkfs_sized "$fs_size" "" -L oldlabel >> $seqres.full 2>&1
> > +	fi
> > +	_scratch_xfs_db -c label | "${filter[@]}"
> >  	_scratch_xfs_admin -L newlabel "$@" >> $seqres.full
> >  	_scratch_xfs_db -c label
> >  	_scratch_xfs_repair -n &>> $seqres.full || echo "Check failed?"
> > 
> 
>