On Mon, Apr 4, 2022 at 1:55 PM Christian Brauner <brauner@xxxxxxxxxx> 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.
>
> Note that the infrastructure is completely generic so every filesystem that
> supports idmapped mounts can simply run all of their tests idmapped. But
> note that not all ways to create a mount have been converted yet. That
> includes e.g. _dmthin_mount and direct calls to _mount in various tests.
>
> In addition, 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.
>
> While we could of course restrict this testmode to -overlay for which we
> know things work correctly we should not do this. It would mean that
> people won't start using it and so we won't see issues unless someone
> sits down and goes through more than 1000 tests and figures out for each
> individual one whether it needs to be skipped or not.
>
> So instead allow this mode but for all filesystems so people can start
> running and reporting failures and we can fix them up or block them as
> we detect them.
>
> Cc: Amir Goldstein <amir73il@xxxxxxxxx>
> Cc: Eryu Guan <guaneryu@xxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: <fstests@xxxxxxxxxxxxxxx>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
one nit below could be addressed later.
> ---
> /* v2 */
> unchanged
>
> /* v3 */
> - Amir Goldstein <amir73il@xxxxxxxxx>:
> - Add more detailed explanation about the current state and
> expectations of the newly added IDMAPPED_MOUNTS support.
>
> /* v4 */
> - Amir Goldstein <amir73il@xxxxxxxxx>:
> - Document new IDMAPPED_MOUNTS env variable in README and add an
> [idmapped] section to README.overlay.
> ---
> README | 5 +++++
> README.overlay | 6 +++++-
> common/config | 1 +
> common/overlay | 2 ++
> common/rc | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/README b/README
> index 9f01aa10..0f98035c 100644
> --- a/README
> +++ b/README
> @@ -78,6 +78,11 @@ Preparing system for tests:
> - setenv TEST_LOGDEV "device for test-fs external log"
> - setenv TEST_RTDEV "device for test-fs realtime data"
> - if TEST_LOGDEV and/or TEST_RTDEV, these will always be used.
> + - set IDMAPPED_MOUNTS=true to run all tests on top of idmapped
> + mounts. While this option is supported for all filesystems
> + currently only -overlay is expected to run without issues. For
> + other filesystems additional patches and fixes to the test suite
> + might be needed.
> - if SCRATCH_LOGDEV and/or SCRATCH_RTDEV, the USE_EXTERNAL
> environment variable set to "yes" will enable their use.
> - setenv DIFF_LENGTH "number of diff lines to print from a failed test",
> diff --git a/README.overlay b/README.overlay
> index 39e25ada..ec4671c3 100644
> --- a/README.overlay
> +++ b/README.overlay
> @@ -31,7 +31,8 @@ partly supported with './check -overlay'. Only multi-section files that
> do not change FSTYP and MKFS_OPTIONS can be safely used with -overlay.
>
> For example, the following multi-section config file can be used to
> -run overlay tests on the same base fs, but with different mount options:
> +run overlay tests on the same base fs, but with different mount options, and on
> +top of idmapped mounts:
>
> [xfs]
> TEST_DEV=/dev/sda5
> @@ -46,6 +47,9 @@ run overlay tests on the same base fs, but with different mount options:
> OVERLAY_MOUNT_OPTIONS="-o redirect_dir=off"
> OVERLAY_FSCK_OPTIONS="-n -o redirect_dir=off"
>
> + [idmapped]
> + IDMAPPED_MOUNTS=true
> +
> In the example above, MOUNT_OPTIONS will be used to mount the base scratch fs,
> TEST_FS_MOUNT_OPTS will be used to mount the base test fs,
> OVERLAY_MOUNT_OPTIONS will be used to mount both test and scratch overlay and
> 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 line has no purpose here.
It does not matter yet because overlayfs mount itself cannot be idmapped
so there is no need to test idmapped overlayfs mount, but if you did want
to allow that, the code would need to look like this:
export OVL_BASE_IDMAPPED_MOUNTS="$IDMAPPED_MOUNTS"
export IDMAPPED_MOUNTS="$OVERLAY_IDMAPPED_MOUNTS"
and in _overlay_config_restore:
[ -z "$OVL_BASE_IDMAPPED_MOUNTS" ] || \
export IDMAPPED_MOUNTS=$OVL_BASE_IDMAPPED_MOUNTS
Then, code dealing with idmapped layers would check the variable
OVL_BASE_IDMAPPED_MOUNTS and code dealing with idmapped
overlayfs mount would check the generic variable IDMAPPED_MOUNTS.
In the config file there would be two possible config vars:
OVERLAY_IDMAPPED_MOUNTS - use idmapped overlayfs mount
IDMAPPED_MOUNTS - use idmapped overlayfs layers
Same as the roles of OVERLAY_MOUNT_OPTIONS and
MOUNT_OPTIONS that are documented in README.overlay.
P.S. the implementation of OVERLAY_FSCK_OPTIONS seems a bit
broken (no restore), but fsck.overlay tool is not very alive, so probably
nobody is using this feature.
Bottom line, I do not know if and when you intend to add support for
idmapped overlayfs mounts, until then, there is no action item.
Unless you want to follow my instructions and use
OVL_BASE_IDMAPPED_MOUNTS semantics already to make the
code a bit more clear. I leave the decision to you or to the maintainer.
Just wanted to put this on record.
Thanks,
Amir.
