diff mbox series

[3/4] generic/{166,167,333,334,671}: actually fill the filesystem with snapshots

Message ID 171150741593.3286541.18115194618541313905.stgit@frogsfrogsfrogs (mailing list archive)
State New
Headers show
Series [1/4] xfs/270: fix rocompat regex | expand

Commit Message

Darrick J. Wong March 27, 2024, 2:43 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

XFS has this behavior in its reflink implementation where it returns
ENOSPC if one of the AGs that would be involved in the sharing operation
becomes more than 90% full.  As Kent Overstreet points out, that means
the snapshot creator shuts down when the filesystem is only about a
third full.  We could exercise the system harder by not *forcing*
reflink, which will actually fill the filesystem full.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/reflink    |    8 ++++++++
 tests/generic/166 |    2 +-
 tests/generic/167 |    2 +-
 tests/generic/333 |    2 +-
 tests/generic/334 |    2 +-
 tests/generic/671 |    2 +-
 6 files changed, 13 insertions(+), 5 deletions(-)

Comments

Christoph Hellwig March 27, 2024, 4:59 p.m. UTC | #1 
On Tue, Mar 26, 2024 at 07:43:35PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> XFS has this behavior in its reflink implementation where it returns
> ENOSPC if one of the AGs that would be involved in the sharing operation
> becomes more than 90% full.  As Kent Overstreet points out, that means
> the snapshot creator shuts down when the filesystem is only about a
> third full.  We could exercise the system harder by not *forcing*
> reflink, which will actually fill the filesystem full.

All these tests are supposed to test the reflink code, how does
using cp --reflink=auto make sense for that?
Darrick J. Wong March 29, 2024, 11:07 p.m. UTC | #2 
On Wed, Mar 27, 2024 at 09:59:13AM -0700, Christoph Hellwig wrote:
> On Tue, Mar 26, 2024 at 07:43:35PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > XFS has this behavior in its reflink implementation where it returns
> > ENOSPC if one of the AGs that would be involved in the sharing operation
> > becomes more than 90% full.  As Kent Overstreet points out, that means
> > the snapshot creator shuts down when the filesystem is only about a
> > third full.  We could exercise the system harder by not *forcing*
> > reflink, which will actually fill the filesystem full.
> 
> All these tests are supposed to test the reflink code, how does
> using cp --reflink=auto make sense for that?

Hmm.  Well my initial thought was that the snapshot could fall back to
buffered copies of file1 so that we wouldn't abort the test well before
actually filling up the filesystem.

But you're right that my solution smells off -- we want to test reflink
dealing with ENOSPC.  Perhaps the right thing to do is to truncate and
rewrite file1 after a _cp_reflink fails, so that the next time through
the loop we'll be reflinking extents from a (probably less full) AG.

Ok let's see how that does.

--D
Christoph Hellwig March 30, 2024, 5:39 a.m. UTC | #3 
On Fri, Mar 29, 2024 at 04:07:20PM -0700, Darrick J. Wong wrote:
> Hmm.  Well my initial thought was that the snapshot could fall back to
> buffered copies of file1 so that we wouldn't abort the test well before
> actually filling up the filesystem.

Btw, can we please stop sing snaphot in the test description for
reflink copies?  That's a really confusing term to use in this context..

> But you're right that my solution smells off -- we want to test reflink
> dealing with ENOSPC.  Perhaps the right thing to do is to truncate and
> rewrite file1 after a _cp_reflink fails, so that the next time through
> the loop we'll be reflinking extents from a (probably less full) AG.

That does sound better.
Zorro Lang March 30, 2024, 7:14 a.m. UTC | #4 
On Wed, Mar 27, 2024 at 09:59:13AM -0700, Christoph Hellwig wrote:
> On Tue, Mar 26, 2024 at 07:43:35PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > XFS has this behavior in its reflink implementation where it returns
> > ENOSPC if one of the AGs that would be involved in the sharing operation
> > becomes more than 90% full.  As Kent Overstreet points out, that means
> > the snapshot creator shuts down when the filesystem is only about a
> > third full.  We could exercise the system harder by not *forcing*
> > reflink, which will actually fill the filesystem full.
> 
> All these tests are supposed to test the reflink code, how does
> using cp --reflink=auto make sense for that?

/me feel confusion too:)

I'll merge other 3 patches of this patchset at first. Feel free to re-send
this patch in another PATCHSET later.

Thanks,
Zorro

>
diff mbox series

Patch

diff --git a/common/reflink b/common/reflink
index 22adc4449b..8f30dc6784 100644
--- a/common/reflink
+++ b/common/reflink
@@ -226,6 +226,14 @@  _cp_reflink() {
 	cp --reflink=always -p -f "$file1" "$file2"
 }
 
+# Create file2 as a snapshot of file1 via cp and possibly reflink.
+_reflink_snapshot() {
+	file1="$1"
+	file2="$2"
+
+	cp --reflink=auto -p -f "$file1" "$file2"
+}
+
 # Reflink some file1 into file2
 _reflink() {
 	file1="$1"
diff --git a/tests/generic/166 b/tests/generic/166
index 0eb2ec9c3a..941b51b3f1 100755
--- a/tests/generic/166
+++ b/tests/generic/166
@@ -60,7 +60,7 @@  snappy() {
 			sleep 0.01
 			continue;
 		fi
-		out="$(_cp_reflink $testdir/file1 $testdir/snap_$n 2>&1)"
+		out="$(_reflink_snapshot $testdir/file1 $testdir/snap_$n 2>&1)"
 		res=$?
 		echo "$out" | grep -q "No space left" && break
 		test -n "$out" && echo "$out"
diff --git a/tests/generic/167 b/tests/generic/167
index ae5fa5eb1c..3670940825 100755
--- a/tests/generic/167
+++ b/tests/generic/167
@@ -50,7 +50,7 @@  _scratch_cycle_mount
 snappy() {
 	n=0
 	while [ ! -e $finished_file ]; do
-		out="$(_cp_reflink $testdir/file1 $testdir/snap_$n 2>&1)"
+		out="$(_reflink_snapshot $testdir/file1 $testdir/snap_$n 2>&1)"
 		res=$?
 		echo "$out" | grep -q "No space left" && break
 		test -n "$out" && echo "$out"
diff --git a/tests/generic/333 b/tests/generic/333
index bf1967ce29..19e69993a3 100755
--- a/tests/generic/333
+++ b/tests/generic/333
@@ -53,7 +53,7 @@  _scratch_cycle_mount
 snappy() {
 	n=0
 	while [ ! -e $finished_file ]; do
-		out="$(_cp_reflink $testdir/file1 $testdir/snap_$n 2>&1)"
+		out="$(_reflink_snapshot $testdir/file1 $testdir/snap_$n 2>&1)"
 		res=$?
 		echo $out | grep -q "No space left" && break
 		test -n "$out" && echo $out
diff --git a/tests/generic/334 b/tests/generic/334
index b9c14b87ac..1e4d37b415 100755
--- a/tests/generic/334
+++ b/tests/generic/334
@@ -52,7 +52,7 @@  _scratch_cycle_mount
 snappy() {
 	n=0
 	while [ ! -e $finished_file ]; do
-		out="$(_cp_reflink $testdir/file1 $testdir/snap_$n 2>&1)"
+		out="$(_reflink_snapshot $testdir/file1 $testdir/snap_$n 2>&1)"
 		res=$?
 		echo $out | grep -q "No space left" && break
 		test -n "$out" && echo $out
diff --git a/tests/generic/671 b/tests/generic/671
index b6cc0573f3..24ed24e213 100755
--- a/tests/generic/671
+++ b/tests/generic/671
@@ -41,7 +41,7 @@  _scratch_cycle_mount
 snappy() {
 	n=0
 	while [ ! -e $finished_file ]; do
-		out="$(_cp_reflink $testdir/file1 $testdir/snap_$n 2>&1)"
+		out="$(_reflink_snapshot $testdir/file1 $testdir/snap_$n 2>&1)"
 		res=$?
 		echo "$out" | grep -q "No space left" && break
 		test -n "$out" && echo "$out"