| Message ID | 20220330102409.1290850-21-brauner@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] common: allow to run all tests on idmapped mounts | expand |
On Wed, Mar 30, 2022 at 1:26 PM Christian Brauner <brauner@kernel.org> wrote: > > In addition to the generic and filesystem-specific idmapped mount > testsuites that already exist upstream today add simple infrastructure > so any test can be run on idmapped mounts simply by setting > IDMAPPED_MOUNTS=true in the config file or section. The main user for > now will be overlay to verify it works correctly on idmapped mounts. > It is not clear from this message how IDMAPPED_MOUNTS=true affects non -overlay runs and the answer is complicated: You handled the majority of cases using _test_mount and _scratch_mount but there are still many tests that open code _mount and some helpers like _dmthin_mount that are used quite often. So either document that IDMAPPED_MOUNTS=true will idmapp the fs mounts whenever it can as best effort, or restrict the use of IDMAPPED_MOUNTS=true with OVERLAY=true for now. > Cc: Eryu Guan <guaneryu@gmail.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: <fstests@vger.kernel.org> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > --- > common/config | 1 + > common/overlay | 2 ++ > common/rc | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 53 insertions(+) > > diff --git a/common/config b/common/config > index 479e50d1..1033b890 100644 > --- a/common/config > +++ b/common/config > @@ -647,6 +647,7 @@ _overlay_config_override() > # Set fsck options, use default if user not set directly. > export FSCK_OPTIONS="$OVERLAY_FSCK_OPTIONS" > [ -z "$FSCK_OPTIONS" ] && _fsck_opts > + export IDMAPPED_MOUNTS="$IDMAPPED_MOUNTS" This is odd. The purpose of _overlay_config_override() and _overlay_config_restore() is to allow different configurations for the base layers and for the overlay. This will be relevant when you implement idmapping for the overlay itself - you would want to test all combinations of idmapped layers/overlay. So this should be: export OVERLAY_IDMAPPED_LAYERS="$IDMAPPED_MOUNTS" export IDMAPPED_MOUNTS="$OVERLAY_IDMAPPED_MOUNTS" And I think it would be simpler and better understood to open code the check if the var in callers like this: [ $OVERLAY_IDMAPPED_LAYERS ] || _idmapped_mount $dev $mnt and not inside _idmapped_mount() It will may it easier for you to implement _overlay_mount() in the future. To clarify, your test setup to test idmapped overlay layers would not change, you would still set IDMAPPED_MOUNTS=true in config file as described in commit message because as README.overlay tries to explain, with ./check -overlay MOUNT_OPTIONS= refers to the layers and OVERLAY_MOUNT_OPTIONS= refers to the overlay I guess adding the IDMAPPED_MOUNTS=true to README.overlay multi section config example wouldn't hurt. Thanks, Amir.
On Wed, Mar 30, 2022 at 02:10:11PM +0300, Amir Goldstein wrote: > On Wed, Mar 30, 2022 at 1:26 PM Christian Brauner <brauner@kernel.org> wrote: > > > > In addition to the generic and filesystem-specific idmapped mount > > testsuites that already exist upstream today add simple infrastructure > > so any test can be run on idmapped mounts simply by setting > > IDMAPPED_MOUNTS=true in the config file or section. The main user for > > now will be overlay to verify it works correctly on idmapped mounts. > > > > It is not clear from this message how IDMAPPED_MOUNTS=true affects > non -overlay runs and the answer is complicated: > > You handled the majority of cases using _test_mount and _scratch_mount > but there are still many tests that open code _mount and some helpers > like _dmthin_mount that are used quite often. I verified that actually for testing overlay. _dmthin_mount is used in 8 tests (btrfs/196, generic/347, generic/405, generic/455, generic/457, generic/470, generic/482, generic/500). None of them were run in a regular testrun on the 5.17 base kernel. Direct _mount calls in the generic/ testsuite are 8 (generic/042, generic/081, generic/108, generic/361, generic/459, generic/563, generic/620, geneirc/649). Again, none of them were run in a regular testrun on the 5.17 base kernel. Let alone that for most of them conversion to be tested on idmapped mounts didn't make sense. > > So either document that IDMAPPED_MOUNTS=true will idmapp the > fs mounts whenever it can as best effort, or restrict the use of > IDMAPPED_MOUNTS=true with OVERLAY=true for now. Just to clarify, the test patch will be upstreamed independently. It is included here so people can run xfstests right away if they are interested. The infrastructure is completely generic so every filesystem that supports idmapped mounts can simply run all of their tests idmapped. But there will be corner-cases. For example, xfs doesn't allow bulkstat on idmapped mounts because it is a filesystem wide operation, i.e. you can retrieve information for any inode in the filesystem so the operation cannot be scoped reasonably under a single mount. So xfstests testing bulkstat will fail as it's blocked. Similar for some btrfs ioctl()s. We could of course restrict this testmode to -overlay for which we know things work correctly but that I don't like that as this means that people won't start using it and so we won't see issues unless someone sits down and goes through all 750+ tests and figures out for each individual one why it fails the way it fails. I'd rather not restrict the generic infrastructure so people can run and report failures and we can fix them up or block them as we detect them. So I'd rather just point that out in the commit message, I think. In any case, the tests will be upstreamed in parallel but separate from this series. :)
On Wed, Mar 30, 2022 at 2:38 PM Christian Brauner <brauner@kernel.org> wrote: > > On Wed, Mar 30, 2022 at 02:10:11PM +0300, Amir Goldstein wrote: > > On Wed, Mar 30, 2022 at 1:26 PM Christian Brauner <brauner@kernel.org> wrote: > > > > > > In addition to the generic and filesystem-specific idmapped mount > > > testsuites that already exist upstream today add simple infrastructure > > > so any test can be run on idmapped mounts simply by setting > > > IDMAPPED_MOUNTS=true in the config file or section. The main user for > > > now will be overlay to verify it works correctly on idmapped mounts. > > > > > > > It is not clear from this message how IDMAPPED_MOUNTS=true affects > > non -overlay runs and the answer is complicated: > > > > You handled the majority of cases using _test_mount and _scratch_mount > > but there are still many tests that open code _mount and some helpers > > like _dmthin_mount that are used quite often. > > I verified that actually for testing overlay. _dmthin_mount is used in 8 > tests (btrfs/196, generic/347, generic/405, generic/455, generic/457, > generic/470, generic/482, generic/500). None of them were run in a > regular testrun on the 5.17 base kernel. > Direct _mount calls in the generic/ testsuite are 8 (generic/042, > generic/081, generic/108, generic/361, generic/459, generic/563, > generic/620, geneirc/649). > Again, none of them were run in a regular testrun on the 5.17 base > kernel. Let alone that for most of them conversion to be tested on > idmapped mounts didn't make sense. > Yes of course, overlayfs tests cannot call _mount directly so your tests with check -overlay should be perfectly fine. My comment is only addressing the *generic* implementation of IDMAPPED_MOUNTS. > > > > So either document that IDMAPPED_MOUNTS=true will idmapp the > > fs mounts whenever it can as best effort, or restrict the use of > > IDMAPPED_MOUNTS=true with OVERLAY=true for now. > > Just to clarify, the test patch will be upstreamed independently. It is > included here so people can run xfstests right away if they are > interested. > > The infrastructure is completely generic so every filesystem that > supports idmapped mounts can simply run all of their tests idmapped. But > there will be corner-cases. For example, xfs doesn't allow bulkstat on > idmapped mounts because it is a filesystem wide operation, i.e. you can > retrieve information for any inode in the filesystem so the operation > cannot be scoped reasonably under a single mount. So xfstests testing > bulkstat will fail as it's blocked. Similar for some btrfs ioctl()s. > > We could of course restrict this testmode to -overlay for which we know > things work correctly but that I don't like that as this means that > people won't start using it and so we won't see issues unless someone > sits down and goes through all 750+ tests and figures out for each > individual one why it fails the way it fails. > > I'd rather not restrict the generic infrastructure so people can run and > report failures and we can fix them up or block them as we detect them. > So I'd rather just point that out in the commit message, I think. > Yes this makes perfect sense to me. The failing bulkstat tests are exactly why we would want to let people start testing their fs, it's the false negatives that I am worried about. all the tests that use _dmthin_mount() will succeed without the tester knows that idmalleped was not tested and its going to be hard to audit all the callers of _mount, so either you make _mount a wrapper to __mount or I don't know what. Maybe you'll think of something wiser to do. Thanks, Amir.
diff --git a/common/config b/common/config index 479e50d1..1033b890 100644 --- a/common/config +++ b/common/config @@ -647,6 +647,7 @@ _overlay_config_override() # Set fsck options, use default if user not set directly. export FSCK_OPTIONS="$OVERLAY_FSCK_OPTIONS" [ -z "$FSCK_OPTIONS" ] && _fsck_opts + export IDMAPPED_MOUNTS="$IDMAPPED_MOUNTS" } _overlay_config_restore() diff --git a/common/overlay b/common/overlay index 1ca37e29..fff67ba1 100644 --- a/common/overlay +++ b/common/overlay @@ -73,6 +73,7 @@ _overlay_base_mount() if [ -z "$dev" -o -z "$mnt" ] || \ _check_mounted_on $devname $dev $mntname $mnt; then + _idmapped_mount $dev $mnt # no base fs or already mounted return 0 elif [ $? -ne 1 ]; then @@ -81,6 +82,7 @@ _overlay_base_mount() fi _mount $* $dev $mnt + _idmapped_mount $dev $mnt } _overlay_base_test_mount() diff --git a/common/rc b/common/rc index faf54ef9..5090cbf8 100644 --- a/common/rc +++ b/common/rc @@ -334,6 +334,7 @@ _try_scratch_mount() return $? fi _mount -t $FSTYP `_scratch_mount_options $*` + _idmapped_mount $SCRATCH_DEV $SCRATCH_MNT } # mount scratch device with given options and _fail if mount fails @@ -444,6 +445,53 @@ _scratch_shutdown_handle() fi } +_move_mount() +{ + local mnt=$1 + local tmp=$2 + + # Replace $mnt with $tmp. Use a temporary bind-mount because + # mount --move will fail with certain mount propagation layouts. + $UMOUNT_PROG $mnt || _fail "Failed to unmount $mnt" + _mount --bind $tmp $mnt || _fail "Failed to bind-mount $tmp to $mnt" + $UMOUNT_PROG $tmp || _fail "Failed to unmount $tmp" + rmdir $tmp +} + +_idmapped_mount() +{ + [ "$IDMAPPED_MOUNTS" = "true" ] || return 0 + + local dev=$1 + local mnt=$2 + local status=0 + local tmp=`mktemp -d` + + local mount_rec=`findmnt -rncv -S $dev -o OPTIONS` + if [[ "$mount_rec" == *"idmapped"* ]]; then + return 0 + fi + + # We create an idmapped mount where {g,u}id 0 writes to disk as + # {g,u}id 10000000 and $(id -u fsgqa) + 10000000. We change ownership + # of $mnt so {g,u} id 0 can actually create objects in there. + chown 10000000:10000000 $mnt || return 1 + $here/src/idmapped-mounts/mount-idmapped \ + --map-mount b:10000000:0:100000000000 \ + $mnt $tmp + if [ $? -ne 0 ]; then + rmdir $tmp + return 1 + fi + + # The next call ensures we don't end up stacking an idmapped mount on + # top of the original mount. Instead we fully replace the original + # mount with the idmapped mount. This will not just allow a clean mount + # layout it also makes unmount and remounting way simpler. + _move_mount $mnt $tmp + return $? +} + _test_mount() { if [ "$FSTYP" == "overlay" ]; then @@ -452,6 +500,7 @@ _test_mount() fi _test_options mount _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR + _idmapped_mount $TEST_DEV $TEST_DIR } _test_unmount() @@ -3007,6 +3056,7 @@ _mount_or_remount_rw() if [ $USE_REMOUNT -eq 0 ]; then if [ "$FSTYP" != "overlay" ]; then _mount -t $FSTYP $mount_opts $device $mountpoint + _idmapped_mount $device $mountpoint else _overlay_mount $device $mountpoint fi
In addition to the generic and filesystem-specific idmapped mount testsuites that already exist upstream today add simple infrastructure so any test can be run on idmapped mounts simply by setting IDMAPPED_MOUNTS=true in the config file or section. The main user for now will be overlay to verify it works correctly on idmapped mounts. Cc: Eryu Guan <guaneryu@gmail.com> Cc: Christoph Hellwig <hch@lst.de> Cc: <fstests@vger.kernel.org> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> --- common/config | 1 + common/overlay | 2 ++ common/rc | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+)