Re: [PATCH 07/12] btrfs: test snapshotting encrypted subvol

[Josef Bacik <josef@toxicpanda.com>]

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@toxicpanda.com> wrote:
> > > 
> > > From: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> > > 
> > > 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@dorminy.me>
> > > ---
> > >   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