On Mon, Nov 27, 2023 at 10:16:28PM +0800, Anand Jain wrote: > > > On 31/10/2023 23:39, Filipe Manana wrote: > > On Tue, Oct 10, 2023 at 9:26 PM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote: > > > > > > From: Sweet Tea Dorminy <sweettea-kernel@xxxxxxxxxx> > > > > > > Make sure that snapshots of encrypted data are readable and writeable. > > > > > > Test deliberately high-numbered to not conflict. > > > > > > Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@xxxxxxxxxx> > > > --- > > > tests/btrfs/614 | 76 ++++++++++++++++++++++++++++++ > > > tests/btrfs/614.out | 111 ++++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 187 insertions(+) > > > create mode 100755 tests/btrfs/614 > > > create mode 100644 tests/btrfs/614.out > > > > > > diff --git a/tests/btrfs/614 b/tests/btrfs/614 > > > new file mode 100755 > > > index 00000000..87dd27f9 > > > --- /dev/null > > > +++ b/tests/btrfs/614 > > > @@ -0,0 +1,76 @@ > > > +#! /bin/bash > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# Copyright (c) 2023 Meta Platforms, Inc. All Rights Reserved. > > > +# > > > +# FS QA Test 614 > > > +# > > > +# Try taking a snapshot of an encrypted subvolume. Make sure the snapshot is > > > +# still readable. Rewrite part of the subvol with the same data; make sure it's > > > +# still readable. > > > +# > > > +. ./common/preamble > > > +_begin_fstest auto encrypt > > > > Should be in the 'snapshot' and 'subvol' groups too, as it creates a > > snapshot and a subvolume. > > Also maybe in the 'quick' group too, see the comments further below. > > > > > + > > > +# Import common functions. > > > +. ./common/encrypt > > > +. ./common/filter > > > + > > > +# real QA test starts here > > > +_supported_fs btrfs > > > + > > > +_require_test > > > > The test device is not used, so this can go away. > > > > > +_require_scratch > > > +_require_scratch_encryption -v 2 > > > +_require_command "$KEYCTL_PROG" keyctl > > > + > > > +_scratch_mkfs_encrypted &>> $seqres.full > > > +_scratch_mount > > > + > > > +udir=$SCRATCH_MNT/reference > > > +dir=$SCRATCH_MNT/subvol > > > +dir2=$SCRATCH_MNT/subvol2 > > > +$BTRFS_UTIL_PROG subvolume create $dir >> $seqres.full > > > +mkdir $udir > > > + > > > +_set_encpolicy $dir $TEST_KEY_IDENTIFIER > > > +_add_enckey $SCRATCH_MNT "$TEST_RAW_KEY" > > > + > > > +# get files with lots of extents by using backwards writes. > > > +for j in `seq 0 50`; do > > > + for i in `seq 20 -1 1`; do > > > + $XFS_IO_PROG -f -d -c "pwrite $(($i * 4096)) 4096" \ > > > + $dir/foo-$j >> $seqres.full | _filter_xfs_io > > > + $XFS_IO_PROG -f -d -c "pwrite $(($i * 4096)) 4096" \ > > > + $udir/foo-$j >> $seqres.full | _filter_xfs_io > > > + done > > > +done > > > + > > > +$BTRFS_UTIL_PROG subvolume snapshot $dir $dir2 | _filter_scratch > > > + > > > +_scratch_remount > > > +_add_enckey $SCRATCH_MNT "$TEST_RAW_KEY" > > > +sleep 30 > > > > What's the sleep for? > > Is the 30 seconds to wait for a transaction commit? > > If it is then I'd rather mount the fs with -o commit=3 (or some other > > low value) and then "sleep 3" to make the test run much faster. > > A comment explaining why the sleep is there, what is its purpose, > > should also be in place. > > > > > +echo "Diffing $dir and $dir2" > > > +diff $dir $dir2 > > > + > > > +echo "Rewriting $dir2 partly" > > > +# rewrite half of each file in the snapshot > > > +for j in `seq 0 50`; do > > > + for i in `seq 10 -1 1`; do > > > + $XFS_IO_PROG -f -d -c "pwrite $(($i * 4096)) 4096" \ > > > + $dir2/foo-$j >> $seqres.full | _filter_xfs_io > > > + done > > > +done > > > + > > > +echo "Diffing $dir and $dir2" > > > +diff $dir $dir2 > > > + > > > +echo "Dropping key and diffing" > > > +_rm_enckey $SCRATCH_MNT $TEST_KEY_IDENTIFIER > > > +diff $dir $dir2 |& _filter_scratch | _filter_nokey_filenames > > > + > > > +$BTRFS_UTIL_PROG subvolume delete $dir > /dev/null 2>&1 > > > > What's the purpose of this subvolume delete? > > It's ignoring stdout and stderr, so it doesn't care whether it > > succeeds or fails, and we > > don't do any tests/checks after it. > > > > Thanks. > > > Josef, I'm planning to get this patchset ready for the PR. Are you planning > to address the review comments as mentioned above? These > aren't bugs, but they definitely add more clarity and adds to the > missing groups. > Can you hold off Anand? I haven't responded because I've been working on this series and making appropriate changes to my local branch, I'll send a refreshed version of the patches when I send the next set of the fscrypt enablement patches. I've got all the comments addressed locally, it'll save you some work. Thanks, Josef
