Re: [PATCH v2 2/5] generic: enable swapfile tests on Btrfs
On Tue, May 22, 2018 at 02:41:45PM +0800, Eryu Guan wrote: > On Wed, May 16, 2018 at 01:38:46PM -0700, Omar Sandoval wrote: > > From: Omar Sandoval > > > > Commit 8c96cfbfe530 ("generic/35[67]: disable swapfile tests on Btrfs") > > disabled the swapfile tests on Btrfs because it did not support > > swapfiles at the time. Now that we're adding support, we want these > > So currently btrfs has no swapfile support yet, and tests still _notrun > with current v4.17-rc5 kernel? Just want to make sure that's expected. Yes, that's correct. > > tests to run, but they don't. _require_scratch_swapfile always fails for > > Btrfs because swapfiles on Btrfs must be set to nocow. After fixing > > that, generic/356 and generic/357 fail for the same reason. After fixing > > _that_, both tests still fail because we don't allow reflinking a > > non-checksummed extent (which nocow implies) to a checksummed extent. > > Add a helper for formatting a swap file which does the chattr, and > > chattr the second file, which gets these tests running on kernels > > supporting Btrfs swapfiles. > > > > Signed-off-by: Omar Sandoval > > --- > > common/rc | 15 --- > > tests/generic/356 | 7 --- > > tests/generic/357 | 6 +++--- > > 3 files changed, 19 insertions(+), 9 deletions(-) > > > > diff --git a/common/rc b/common/rc > > index ffe53236..814b8b5c 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -,6 +,17 @@ _require_odirect() > > rm -f $testfile 2>&1 > /dev/null > > } > > > > +_format_swapfile() { > > + local fname="$1" > > + local sz="$2" > > + > > + touch "$fname" > > Better to remove $fname first, just in case it's an existing file with > some blocks allocated, as chattr(1) says > > "Note: For btrfs, the 'C' flag should be set on new or empty files. If > it is set on a file which already has data blocks, it is undefined when > the blocks assigned to the file will be fully stable" Good point. > > + chmod 0600 "$fname" > > + $CHATTR_PROG +C "$fname" > /dev/null 2>&1 > > It'd be good to have some comments on this chattr +C. Will do. > > + _pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full > > + mkswap -U 27376b42-ff65-42ca-919f-6c9b62292a5c "$fname" >> $seqres.full > > Is the label really needed? Nope, doesn't look like it. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/5] generic: enable swapfile tests on Btrfs
On Wed, May 16, 2018 at 01:38:46PM -0700, Omar Sandoval wrote: > From: Omar Sandoval > > Commit 8c96cfbfe530 ("generic/35[67]: disable swapfile tests on Btrfs") > disabled the swapfile tests on Btrfs because it did not support > swapfiles at the time. Now that we're adding support, we want these So currently btrfs has no swapfile support yet, and tests still _notrun with current v4.17-rc5 kernel? Just want to make sure that's expected. > tests to run, but they don't. _require_scratch_swapfile always fails for > Btrfs because swapfiles on Btrfs must be set to nocow. After fixing > that, generic/356 and generic/357 fail for the same reason. After fixing > _that_, both tests still fail because we don't allow reflinking a > non-checksummed extent (which nocow implies) to a checksummed extent. > Add a helper for formatting a swap file which does the chattr, and > chattr the second file, which gets these tests running on kernels > supporting Btrfs swapfiles. > > Signed-off-by: Omar Sandoval > --- > common/rc | 15 --- > tests/generic/356 | 7 --- > tests/generic/357 | 6 +++--- > 3 files changed, 19 insertions(+), 9 deletions(-) > > diff --git a/common/rc b/common/rc > index ffe53236..814b8b5c 100644 > --- a/common/rc > +++ b/common/rc > @@ -,6 +,17 @@ _require_odirect() > rm -f $testfile 2>&1 > /dev/null > } > > +_format_swapfile() { > + local fname="$1" > + local sz="$2" > + > + touch "$fname" Better to remove $fname first, just in case it's an existing file with some blocks allocated, as chattr(1) says "Note: For btrfs, the 'C' flag should be set on new or empty files. If it is set on a file which already has data blocks, it is undefined when the blocks assigned to the file will be fully stable" > + chmod 0600 "$fname" > + $CHATTR_PROG +C "$fname" > /dev/null 2>&1 It'd be good to have some comments on this chattr +C. > + _pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full > + mkswap -U 27376b42-ff65-42ca-919f-6c9b62292a5c "$fname" >> $seqres.full Is the label really needed? Thanks, Eryu > +} > + > # Check that the filesystem supports swapfiles > _require_scratch_swapfile() > { > @@ -2231,10 +2242,8 @@ _require_scratch_swapfile() > _scratch_mount > > # Minimum size for mkswap is 10 pages > - local size=$(($(get_page_size) * 10)) > + _format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10)) > > - _pwrite_byte 0x61 0 "$size" "$SCRATCH_MNT/swap" >/dev/null 2>&1 > - mkswap "$SCRATCH_MNT/swap" >/dev/null 2>&1 > if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then > _scratch_unmount > _notrun "swapfiles are not supported" > diff --git a/tests/generic/356 b/tests/generic/356 > index 51eeb652..b4a38f84 100755 > --- a/tests/generic/356 > +++ b/tests/generic/356 > @@ -59,11 +59,12 @@ blocks=160 > blksz=65536 > > echo "Initialize file" > -echo >> $seqres.full > -_pwrite_byte 0x61 0 $((blocks * blksz)) $testdir/file1 >> $seqres.full > -mkswap -U 27376b42-ff65-42ca-919f-6c9b62292a5c $testdir/file1 >> $seqres.full > +_format_swapfile "$testdir/file1" $((blocks * blksz)) > swapon $testdir/file1 > > +touch "$testdir/file2" > +$CHATTR_PROG +C "$testdir/file2" >/dev/null 2>&1 > + > echo "Try to reflink" > _cp_reflink $testdir/file1 $testdir/file2 2>&1 | _filter_scratch > > diff --git a/tests/generic/357 b/tests/generic/357 > index 0dd0c10f..9a83a283 100755 > --- a/tests/generic/357 > +++ b/tests/generic/357 > @@ -59,9 +59,9 @@ blocks=160 > blksz=65536 > > echo "Initialize file" > -echo >> $seqres.full > -_pwrite_byte 0x61 0 $((blocks * blksz)) $testdir/file1 >> $seqres.full > -mkswap -U 27376b42-ff65-42ca-919f-6c9b62292a5c $testdir/file1 >> $seqres.full > +_format_swapfile "$testdir/file1" $((blocks * blksz)) > +touch "$testdir/file2" > +$CHATTR_PROG +C "$testdir/file2" >/dev/null 2>&1 > _cp_reflink $testdir/file1 $testdir/file2 2>&1 | _filter_scratch > > echo "Try to swapon" > -- > 2.17.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/5] generic: enable swapfile tests on Btrfs
From: Omar Sandoval Commit 8c96cfbfe530 ("generic/35[67]: disable swapfile tests on Btrfs") disabled the swapfile tests on Btrfs because it did not support swapfiles at the time. Now that we're adding support, we want these tests to run, but they don't. _require_scratch_swapfile always fails for Btrfs because swapfiles on Btrfs must be set to nocow. After fixing that, generic/356 and generic/357 fail for the same reason. After fixing _that_, both tests still fail because we don't allow reflinking a non-checksummed extent (which nocow implies) to a checksummed extent. Add a helper for formatting a swap file which does the chattr, and chattr the second file, which gets these tests running on kernels supporting Btrfs swapfiles. Signed-off-by: Omar Sandoval --- common/rc | 15 --- tests/generic/356 | 7 --- tests/generic/357 | 6 +++--- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/common/rc b/common/rc index ffe53236..814b8b5c 100644 --- a/common/rc +++ b/common/rc @@ -,6 +,17 @@ _require_odirect() rm -f $testfile 2>&1 > /dev/null } +_format_swapfile() { + local fname="$1" + local sz="$2" + + touch "$fname" + chmod 0600 "$fname" + $CHATTR_PROG +C "$fname" > /dev/null 2>&1 + _pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full + mkswap -U 27376b42-ff65-42ca-919f-6c9b62292a5c "$fname" >> $seqres.full +} + # Check that the filesystem supports swapfiles _require_scratch_swapfile() { @@ -2231,10 +2242,8 @@ _require_scratch_swapfile() _scratch_mount # Minimum size for mkswap is 10 pages - local size=$(($(get_page_size) * 10)) + _format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10)) - _pwrite_byte 0x61 0 "$size" "$SCRATCH_MNT/swap" >/dev/null 2>&1 - mkswap "$SCRATCH_MNT/swap" >/dev/null 2>&1 if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then _scratch_unmount _notrun "swapfiles are not supported" diff --git a/tests/generic/356 b/tests/generic/356 index 51eeb652..b4a38f84 100755 --- a/tests/generic/356 +++ b/tests/generic/356 @@ -59,11 +59,12 @@ blocks=160 blksz=65536 echo "Initialize file" -echo >> $seqres.full -_pwrite_byte 0x61 0 $((blocks * blksz)) $testdir/file1 >> $seqres.full -mkswap -U 27376b42-ff65-42ca-919f-6c9b62292a5c $testdir/file1 >> $seqres.full +_format_swapfile "$testdir/file1" $((blocks * blksz)) swapon $testdir/file1 +touch "$testdir/file2" +$CHATTR_PROG +C "$testdir/file2" >/dev/null 2>&1 + echo "Try to reflink" _cp_reflink $testdir/file1 $testdir/file2 2>&1 | _filter_scratch diff --git a/tests/generic/357 b/tests/generic/357 index 0dd0c10f..9a83a283 100755 --- a/tests/generic/357 +++ b/tests/generic/357 @@ -59,9 +59,9 @@ blocks=160 blksz=65536 echo "Initialize file" -echo >> $seqres.full -_pwrite_byte 0x61 0 $((blocks * blksz)) $testdir/file1 >> $seqres.full -mkswap -U 27376b42-ff65-42ca-919f-6c9b62292a5c $testdir/file1 >> $seqres.full +_format_swapfile "$testdir/file1" $((blocks * blksz)) +touch "$testdir/file2" +$CHATTR_PROG +C "$testdir/file2" >/dev/null 2>&1 _cp_reflink $testdir/file1 $testdir/file2 2>&1 | _filter_scratch echo "Try to swapon" -- 2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html