| Message ID | 80a5002310ab7f06efd9fed76cf268673e631335.1526503000.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 16, 2018 at 01:38:46PM -0700, Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > 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 <osandov@fb.com> > --- > 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 > @@ -2222,6 +2222,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 fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <osandov@fb.com> > > > > 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 <osandov@fb.com> > > --- > > 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 > > @@ -2222,6 +2222,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 fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/common/rc b/common/rc index ffe53236..814b8b5c 100644 --- a/common/rc +++ b/common/rc @@ -2222,6 +2222,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"