mbox series

[v12,0/6] fstests: add idmapped mounts tests

Message ID 20210328223400.1800301-1-brauner@kernel.org (mailing list archive)
Headers show
Series fstests: add idmapped mounts tests | expand

Message

Christian Brauner March 28, 2021, 10:33 p.m. UTC
From: Christian Brauner <christian.brauner@ubuntu.com>

Hey everyone,

This series is available from:
https://git.kernel.org/brauner/xfstests-dev/h/idmapped_mounts
https://gitlab.com/brauner/xfstests/-/tree/idmapped_mounts
https://github.com/brauner/xfstests/tree/idmapped_mounts

/* v12 */
- Main change is a fix to the io_uring tests. io_uring_wait_cqe()
  doesn't set errno, it returns a negative errno.
- Make sure we also run without error when libcap isn't available (That
  was always the goal but the logic missed a few new tests.)

/* v11 */
Reworked according to Amir's comments.

/* v10 */
Reworked according to Eryu's comments.

/* v9 */
Rebased onto current master.

ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check -g idmapped
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-syz #18 SMP PREEMPT Fri Mar 26 13:27:16 UTC 2021
MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop1
MOUNT_OPTIONS -- /dev/loop1 /mnt/scratch

generic/632 files ...  27s
xfs/529 files ...  67s
xfs/530 files ...  43s
Ran: generic/632 xfs/529 xfs/530
Passed all 3 tests

Thanks!
Christian

Christian Brauner (6):
  generic/631: add test for detached mount propagation
  generic/632: add fstests for idmapped mounts
  common/rc: add _scratch_{u}mount_idmapped() helpers
  common/quota: move _qsetup() helper to common code
  xfs/529: quotas and idmapped mounts
  xfs/530: quotas on idmapped mounts

 .gitignore                            |    3 +
 README                                |    5 +-
 common/quota                          |   20 +
 common/rc                             |   60 +
 configure.ac                          |    2 +
 include/builddefs.in                  |    1 +
 m4/Makefile                           |    1 +
 m4/package_libcap.m4                  |    4 +
 src/Makefile                          |    8 +-
 src/detached_mounts_propagation.c     |  189 +
 src/feature.c                         |   40 +-
 src/idmapped-mounts/Makefile          |   40 +
 src/idmapped-mounts/idmapped-mounts.c | 8873 +++++++++++++++++++++++++
 src/idmapped-mounts/missing.h         |  151 +
 src/idmapped-mounts/mount-idmapped.c  |  431 ++
 src/idmapped-mounts/utils.c           |  134 +
 src/idmapped-mounts/utils.h           |   30 +
 tests/generic/631                     |   43 +
 tests/generic/631.out                 |    2 +
 tests/generic/632                     |   42 +
 tests/generic/632.out                 |    2 +
 tests/generic/group                   |    2 +
 tests/xfs/050                         |   19 -
 tests/xfs/299                         |   19 -
 tests/xfs/529                         |  377 ++
 tests/xfs/529.out                     |  657 ++
 tests/xfs/530                         |  212 +
 tests/xfs/530.out                     |  129 +
 tests/xfs/group                       |    2 +
 29 files changed, 11453 insertions(+), 45 deletions(-)
 create mode 100644 m4/package_libcap.m4
 create mode 100644 src/detached_mounts_propagation.c
 create mode 100644 src/idmapped-mounts/Makefile
 create mode 100644 src/idmapped-mounts/idmapped-mounts.c
 create mode 100644 src/idmapped-mounts/missing.h
 create mode 100644 src/idmapped-mounts/mount-idmapped.c
 create mode 100644 src/idmapped-mounts/utils.c
 create mode 100644 src/idmapped-mounts/utils.h
 create mode 100644 tests/generic/631
 create mode 100644 tests/generic/631.out
 create mode 100644 tests/generic/632
 create mode 100644 tests/generic/632.out
 create mode 100644 tests/xfs/529
 create mode 100644 tests/xfs/529.out
 create mode 100644 tests/xfs/530
 create mode 100644 tests/xfs/530.out


base-commit: f6ddaf130d5b0817278afe441fdde52f464f321b

Comments

Eryu Guan April 11, 2021, 2:30 p.m. UTC | #1
On Mon, Mar 29, 2021 at 12:33:56AM +0200, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> Add a test suite to verify the behavior of idmapped mounts. The test
> suite also includes a range of vfs tests to verify that no regressions
> are introduced by idmapped mounts. The following tests are currently
> available with more to come in the future:
> 
> 01. posix acls on regular and idmapped mounts

I'm getting failures like below when testing on btrfs and overlayfs

[root@fedoravm xfstests]# diff -u
/root/workspace/xfstests/tests/generic/633.out /root/workspace/xfstests/results//btrfs/generic/633.out.bad
--- /root/workspace/xfstests/tests/generic/633.out      2021-04-11 22:18: 24.458518716 +0800
+++ /root/workspace/xfstests/results//btrfs/generic/633.out.bad 2021-04-11 22:19:58.887980770 +0800
@@ -1,2 +1,4 @@
 QA output created by 633
  Silence is golden
  +idmapped-mounts.c: 6622: acls - Invalid argument - failure: sys_mount_setattr
  +failure: posix acls on regular mounts

Is this a known issue or test needs some fix?

[snip]

>  create mode 100644 tests/generic/632

Please change mode of new test file to 755 if you end up with sending
new version, otherwise I can fix it on commit.

Thanks,
Eryu
Eryu Guan April 11, 2021, 2:37 p.m. UTC | #2
On Mon, Mar 29, 2021 at 12:33:54AM +0200, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> Hey everyone,
> 
> This series is available from:
> https://git.kernel.org/brauner/xfstests-dev/h/idmapped_mounts
> https://gitlab.com/brauner/xfstests/-/tree/idmapped_mounts
> https://github.com/brauner/xfstests/tree/idmapped_mounts
> 
> /* v12 */
> - Main change is a fix to the io_uring tests. io_uring_wait_cqe()
>   doesn't set errno, it returns a negative errno.
> - Make sure we also run without error when libcap isn't available (That
>   was always the goal but the logic missed a few new tests.)

Thanks for the revision! The whole patchset looks fine to me, except
that I'm not sure if the generic/632 failure on btrfs & overlayfs is
expected.

With that resolved (either it's expected failure or test needs fix), I
think the patchset is ready to be merged.

Thanks,
Eryu

> 
> /* v11 */
> Reworked according to Amir's comments.
> 
> /* v10 */
> Reworked according to Eryu's comments.
> 
> /* v9 */
> Rebased onto current master.
> 
> ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check -g idmapped
> FSTYP         -- xfs (debug)
> PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-syz #18 SMP PREEMPT Fri Mar 26 13:27:16 UTC 2021
> MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop1
> MOUNT_OPTIONS -- /dev/loop1 /mnt/scratch
> 
> generic/632 files ...  27s
> xfs/529 files ...  67s
> xfs/530 files ...  43s
> Ran: generic/632 xfs/529 xfs/530
> Passed all 3 tests
> 
> Thanks!
> Christian
> 
> Christian Brauner (6):
>   generic/631: add test for detached mount propagation
>   generic/632: add fstests for idmapped mounts
>   common/rc: add _scratch_{u}mount_idmapped() helpers
>   common/quota: move _qsetup() helper to common code
>   xfs/529: quotas and idmapped mounts
>   xfs/530: quotas on idmapped mounts
> 
>  .gitignore                            |    3 +
>  README                                |    5 +-
>  common/quota                          |   20 +
>  common/rc                             |   60 +
>  configure.ac                          |    2 +
>  include/builddefs.in                  |    1 +
>  m4/Makefile                           |    1 +
>  m4/package_libcap.m4                  |    4 +
>  src/Makefile                          |    8 +-
>  src/detached_mounts_propagation.c     |  189 +
>  src/feature.c                         |   40 +-
>  src/idmapped-mounts/Makefile          |   40 +
>  src/idmapped-mounts/idmapped-mounts.c | 8873 +++++++++++++++++++++++++
>  src/idmapped-mounts/missing.h         |  151 +
>  src/idmapped-mounts/mount-idmapped.c  |  431 ++
>  src/idmapped-mounts/utils.c           |  134 +
>  src/idmapped-mounts/utils.h           |   30 +
>  tests/generic/631                     |   43 +
>  tests/generic/631.out                 |    2 +
>  tests/generic/632                     |   42 +
>  tests/generic/632.out                 |    2 +
>  tests/generic/group                   |    2 +
>  tests/xfs/050                         |   19 -
>  tests/xfs/299                         |   19 -
>  tests/xfs/529                         |  377 ++
>  tests/xfs/529.out                     |  657 ++
>  tests/xfs/530                         |  212 +
>  tests/xfs/530.out                     |  129 +
>  tests/xfs/group                       |    2 +
>  29 files changed, 11453 insertions(+), 45 deletions(-)
>  create mode 100644 m4/package_libcap.m4
>  create mode 100644 src/detached_mounts_propagation.c
>  create mode 100644 src/idmapped-mounts/Makefile
>  create mode 100644 src/idmapped-mounts/idmapped-mounts.c
>  create mode 100644 src/idmapped-mounts/missing.h
>  create mode 100644 src/idmapped-mounts/mount-idmapped.c
>  create mode 100644 src/idmapped-mounts/utils.c
>  create mode 100644 src/idmapped-mounts/utils.h
>  create mode 100644 tests/generic/631
>  create mode 100644 tests/generic/631.out
>  create mode 100644 tests/generic/632
>  create mode 100644 tests/generic/632.out
>  create mode 100644 tests/xfs/529
>  create mode 100644 tests/xfs/529.out
>  create mode 100644 tests/xfs/530
>  create mode 100644 tests/xfs/530.out
> 
> 
> base-commit: f6ddaf130d5b0817278afe441fdde52f464f321b
> -- 
> 2.27.0
Christian Brauner April 11, 2021, 3:12 p.m. UTC | #3
On Sun, Apr 11, 2021 at 10:30:20PM +0800, Eryu Guan wrote:
> On Mon, Mar 29, 2021 at 12:33:56AM +0200, Christian Brauner wrote:
> > From: Christian Brauner <christian.brauner@ubuntu.com>
> > 
> > Add a test suite to verify the behavior of idmapped mounts. The test
> > suite also includes a range of vfs tests to verify that no regressions
> > are introduced by idmapped mounts. The following tests are currently
> > available with more to come in the future:
> > 
> > 01. posix acls on regular and idmapped mounts
> 
> I'm getting failures like below when testing on btrfs and overlayfs
> 
> [root@fedoravm xfstests]# diff -u
> /root/workspace/xfstests/tests/generic/633.out /root/workspace/xfstests/results//btrfs/generic/633.out.bad
> --- /root/workspace/xfstests/tests/generic/633.out      2021-04-11 22:18: 24.458518716 +0800
> +++ /root/workspace/xfstests/results//btrfs/generic/633.out.bad 2021-04-11 22:19:58.887980770 +0800
> @@ -1,2 +1,4 @@
>  QA output created by 633
>   Silence is golden
>   +idmapped-mounts.c: 6622: acls - Invalid argument - failure: sys_mount_setattr
>   +failure: posix acls on regular mounts
> 
> Is this a known issue or test needs some fix?

Ah, this is very likely just the getfacl command missing which is part
of the "acl" package, at least on Ubuntu.

Christian
Christian Brauner April 11, 2021, 3:18 p.m. UTC | #4
On Sun, Apr 11, 2021 at 05:12:49PM +0200, Christian Brauner wrote:
> On Sun, Apr 11, 2021 at 10:30:20PM +0800, Eryu Guan wrote:
> > On Mon, Mar 29, 2021 at 12:33:56AM +0200, Christian Brauner wrote:
> > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > 
> > > Add a test suite to verify the behavior of idmapped mounts. The test
> > > suite also includes a range of vfs tests to verify that no regressions
> > > are introduced by idmapped mounts. The following tests are currently
> > > available with more to come in the future:
> > > 
> > > 01. posix acls on regular and idmapped mounts
> > 
> > I'm getting failures like below when testing on btrfs and overlayfs
> > 
> > [root@fedoravm xfstests]# diff -u
> > /root/workspace/xfstests/tests/generic/633.out /root/workspace/xfstests/results//btrfs/generic/633.out.bad
> > --- /root/workspace/xfstests/tests/generic/633.out      2021-04-11 22:18: 24.458518716 +0800
> > +++ /root/workspace/xfstests/results//btrfs/generic/633.out.bad 2021-04-11 22:19:58.887980770 +0800
> > @@ -1,2 +1,4 @@
> >  QA output created by 633
> >   Silence is golden
> >   +idmapped-mounts.c: 6622: acls - Invalid argument - failure: sys_mount_setattr
> >   +failure: posix acls on regular mounts
> > 
> > Is this a known issue or test needs some fix?
> 
> Ah, this is very likely just the getfacl command missing which is part
> of the "acl" package, at least on Ubuntu.

And it should actually skip the tests on all filesystems that don't
support idmapped mounts which is every fs apart from xfs and ext4.

Christian
Eryu Guan April 11, 2021, 3:19 p.m. UTC | #5
On Sun, Apr 11, 2021 at 05:12:49PM +0200, Christian Brauner wrote:
> On Sun, Apr 11, 2021 at 10:30:20PM +0800, Eryu Guan wrote:
> > On Mon, Mar 29, 2021 at 12:33:56AM +0200, Christian Brauner wrote:
> > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > 
> > > Add a test suite to verify the behavior of idmapped mounts. The test
> > > suite also includes a range of vfs tests to verify that no regressions
> > > are introduced by idmapped mounts. The following tests are currently
> > > available with more to come in the future:
> > > 
> > > 01. posix acls on regular and idmapped mounts
> > 
> > I'm getting failures like below when testing on btrfs and overlayfs
> > 
> > [root@fedoravm xfstests]# diff -u
> > /root/workspace/xfstests/tests/generic/633.out /root/workspace/xfstests/results//btrfs/generic/633.out.bad
> > --- /root/workspace/xfstests/tests/generic/633.out      2021-04-11 22:18: 24.458518716 +0800
> > +++ /root/workspace/xfstests/results//btrfs/generic/633.out.bad 2021-04-11 22:19:58.887980770 +0800
> > @@ -1,2 +1,4 @@
> >  QA output created by 633
> >   Silence is golden
> >   +idmapped-mounts.c: 6622: acls - Invalid argument - failure: sys_mount_setattr
> >   +failure: posix acls on regular mounts
> > 
> > Is this a known issue or test needs some fix?
> 
> Ah, this is very likely just the getfacl command missing which is part
> of the "acl" package, at least on Ubuntu.

I do have getfacl command installed, and test passed if I tested on ext4
or xfs. So I don't think it's related to getfacl command.

Thanks,
Eryu
Eryu Guan April 11, 2021, 3:21 p.m. UTC | #6
On Sun, Apr 11, 2021 at 05:18:57PM +0200, Christian Brauner wrote:
> On Sun, Apr 11, 2021 at 05:12:49PM +0200, Christian Brauner wrote:
> > On Sun, Apr 11, 2021 at 10:30:20PM +0800, Eryu Guan wrote:
> > > On Mon, Mar 29, 2021 at 12:33:56AM +0200, Christian Brauner wrote:
> > > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > > 
> > > > Add a test suite to verify the behavior of idmapped mounts. The test
> > > > suite also includes a range of vfs tests to verify that no regressions
> > > > are introduced by idmapped mounts. The following tests are currently
> > > > available with more to come in the future:
> > > > 
> > > > 01. posix acls on regular and idmapped mounts
> > > 
> > > I'm getting failures like below when testing on btrfs and overlayfs
> > > 
> > > [root@fedoravm xfstests]# diff -u
> > > /root/workspace/xfstests/tests/generic/633.out /root/workspace/xfstests/results//btrfs/generic/633.out.bad
> > > --- /root/workspace/xfstests/tests/generic/633.out      2021-04-11 22:18: 24.458518716 +0800
> > > +++ /root/workspace/xfstests/results//btrfs/generic/633.out.bad 2021-04-11 22:19:58.887980770 +0800
> > > @@ -1,2 +1,4 @@
> > >  QA output created by 633
> > >   Silence is golden
> > >   +idmapped-mounts.c: 6622: acls - Invalid argument - failure: sys_mount_setattr
> > >   +failure: posix acls on regular mounts
> > > 
> > > Is this a known issue or test needs some fix?
> > 
> > Ah, this is very likely just the getfacl command missing which is part
> > of the "acl" package, at least on Ubuntu.
> 
> And it should actually skip the tests on all filesystems that don't
> support idmapped mounts which is every fs apart from xfs and ext4.

Yeah, looks like that's the case. So it seems the _require idmapped
mount helper doesn't do the job properly.

Eryu
Christian Brauner April 11, 2021, 3:32 p.m. UTC | #7
On Sun, Apr 11, 2021 at 11:21:36PM +0800, Eryu Guan wrote:
> On Sun, Apr 11, 2021 at 05:18:57PM +0200, Christian Brauner wrote:
> > On Sun, Apr 11, 2021 at 05:12:49PM +0200, Christian Brauner wrote:
> > > On Sun, Apr 11, 2021 at 10:30:20PM +0800, Eryu Guan wrote:
> > > > On Mon, Mar 29, 2021 at 12:33:56AM +0200, Christian Brauner wrote:
> > > > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > 
> > > > > Add a test suite to verify the behavior of idmapped mounts. The test
> > > > > suite also includes a range of vfs tests to verify that no regressions
> > > > > are introduced by idmapped mounts. The following tests are currently
> > > > > available with more to come in the future:
> > > > > 
> > > > > 01. posix acls on regular and idmapped mounts
> > > > 
> > > > I'm getting failures like below when testing on btrfs and overlayfs
> > > > 
> > > > [root@fedoravm xfstests]# diff -u
> > > > /root/workspace/xfstests/tests/generic/633.out /root/workspace/xfstests/results//btrfs/generic/633.out.bad
> > > > --- /root/workspace/xfstests/tests/generic/633.out      2021-04-11 22:18: 24.458518716 +0800
> > > > +++ /root/workspace/xfstests/results//btrfs/generic/633.out.bad 2021-04-11 22:19:58.887980770 +0800
> > > > @@ -1,2 +1,4 @@
> > > >  QA output created by 633
> > > >   Silence is golden
> > > >   +idmapped-mounts.c: 6622: acls - Invalid argument - failure: sys_mount_setattr
> > > >   +failure: posix acls on regular mounts
> > > > 
> > > > Is this a known issue or test needs some fix?
> > > 
> > > Ah, this is very likely just the getfacl command missing which is part
> > > of the "acl" package, at least on Ubuntu.
> > 
> > And it should actually skip the tests on all filesystems that don't
> > support idmapped mounts which is every fs apart from xfs and ext4.
> 
> Yeah, looks like that's the case. So it seems the _require idmapped

Ah, ok I didn't know this needed to go in there. I thinke the following
might be enough. Are you able to simply apply it on top?

diff --git a/common/rc b/common/rc
index 351996fc..bd913d13 100644
--- a/common/rc
+++ b/common/rc
@@ -2047,6 +2047,16 @@ _require_mount_setattr()
 # test whether idmapped mounts are supported
 _require_idmapped_mounts()
 {
+       case "$FSTYP" in
+       xfs)
+               ;;
+       ext4)
+               ;;
+       *)
+               _notrun "Filesystem $FSTYP does not support idmapped mounts yet"
+               ;;
+       esac
+
         IDMAPPED_MOUNTS_TEST=$here/src/idmapped-mounts/idmapped-mounts
         [ -x $IDMAPPED_MOUNTS_TEST ] || _notrun "idmapped-mounts utilities required"

Christian
Theodore Ts'o April 12, 2021, 12:40 a.m. UTC | #8
On Sun, Apr 11, 2021 at 05:32:23PM +0200, Christian Brauner wrote:
> Ah, ok I didn't know this needed to go in there. I thinke the following
> might be enough. Are you able to simply apply it on top?
> 
> diff --git a/common/rc b/common/rc
> index 351996fc..bd913d13 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2047,6 +2047,16 @@ _require_mount_setattr()
>  # test whether idmapped mounts are supported
>  _require_idmapped_mounts()
>  {
> +       case "$FSTYP" in
> +       xfs)
> +               ;;
> +       ext4)
> +               ;;
> +       *)
> +               _notrun "Filesystem $FSTYP does not support idmapped mounts yet"
> +               ;;
> +       esac
> +

Is there any way we can ask the kernel which file systems support
idmapped mounts?  That way, the tests will do the right thing when run
on older LTS kernels, and if a distribution backports idmapped support
for some file system into their kernel, again, the right thing can
happen automatically.

If you can't do this by checking to see if the file system will
support a particular mount option, or some other run-time test, for
ext4 we can signal this by checking for the existence of a file in
/sys/fs/ext4/features, such as /sys/fs/ext4/features/fast_commit.
(Grep for EXT4_ATTR_FEATURE and ATTR_LIST in fs/ext4/sysfs.c; it
requires adding two lines to advertise a new ext4 feature.)

	 	    	     	       - Ted
Christoph Hellwig April 12, 2021, 7:22 a.m. UTC | #9
On Sun, Apr 11, 2021 at 05:32:23PM +0200, Christian Brauner wrote:
> > > And it should actually skip the tests on all filesystems that don't
> > > support idmapped mounts which is every fs apart from xfs and ext4.
> > 
> > Yeah, looks like that's the case. So it seems the _require idmapped
> 
> Ah, ok I didn't know this needed to go in there. I thinke the following
> might be enough. Are you able to simply apply it on top?

No, this is broken.  It needs to do feature detection, not some arbitrary
whitelists.  And I'm pretty sure we got that right at some point by
doing the _notrun when mounting with the idmap option was not supported.
Christian Brauner April 12, 2021, 7:30 a.m. UTC | #10
On Mon, Apr 12, 2021 at 09:22:41AM +0200, Christoph Hellwig wrote:
> On Sun, Apr 11, 2021 at 05:32:23PM +0200, Christian Brauner wrote:
> > > > And it should actually skip the tests on all filesystems that don't
> > > > support idmapped mounts which is every fs apart from xfs and ext4.
> > > 
> > > Yeah, looks like that's the case. So it seems the _require idmapped
> > 
> > Ah, ok I didn't know this needed to go in there. I thinke the following
> > might be enough. Are you able to simply apply it on top?
> 
> No, this is broken.  It needs to do feature detection, not some arbitrary
> whitelists.  And I'm pretty sure we got that right at some point by

I thought so too.

> doing the _notrun when mounting with the idmap option was not supported.

The thing is we need the fileystem type we're going to be tested on. I
can place this into src/feature.c if this can get access to the fstype
the test is going to be run on. It should be but if not we can do this
in src/idmapped-mounts/idmapped-mounts.c
directly to test whether the underlying fs we're being tested on
supports idmapped mounts and then return an error value from the binary
indicating that it doesn't so we can use _notrun.
I'll post an updated version of this today with the detection logic.

Christian
Christian Brauner April 12, 2021, 11:54 a.m. UTC | #11
On Sun, Apr 11, 2021 at 08:40:12PM -0400, Theodore Ts'o wrote:
> On Sun, Apr 11, 2021 at 05:32:23PM +0200, Christian Brauner wrote:
> > Ah, ok I didn't know this needed to go in there. I thinke the following
> > might be enough. Are you able to simply apply it on top?
> > 
> > diff --git a/common/rc b/common/rc
> > index 351996fc..bd913d13 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -2047,6 +2047,16 @@ _require_mount_setattr()
> >  # test whether idmapped mounts are supported
> >  _require_idmapped_mounts()
> >  {
> > +       case "$FSTYP" in
> > +       xfs)
> > +               ;;
> > +       ext4)
> > +               ;;
> > +       *)
> > +               _notrun "Filesystem $FSTYP does not support idmapped mounts yet"
> > +               ;;
> > +       esac
> > +
> 
> Is there any way we can ask the kernel which file systems support
> idmapped mounts?  That way, the tests will do the right thing when run
> on older LTS kernels, and if a distribution backports idmapped support
> for some file system into their kernel, again, the right thing can
> happen automatically.

So we can detect it pretty reliably at runtime by trying whether we can
create an idmapped mount on the given filesystem. That is enough for the
idmapped mount tests here but of course has at least two drawbacks:
1. there might be scenarios where we get false negatives
   (e.g. open_tree() could fail for a lack of permissions or sm else,
   kernel might be compiled without userns support etc. pp)
2. it's heavy in so far as we have to do the whole exercise of creating
   a detached mount
So having a reliable way to detect whether or not the underlying fs
supports it could be worth it (My hope was for the fsinfo() API to grow
this "feature check" ability but oh well.).

One possibility might be to extend fstatfs() and steal one u32 from the
padding that is currently in there?

> 
> If you can't do this by checking to see if the file system will
> support a particular mount option, or some other run-time test, for
> ext4 we can signal this by checking for the existence of a file in
> /sys/fs/ext4/features, such as /sys/fs/ext4/features/fast_commit.
> (Grep for EXT4_ATTR_FEATURE and ATTR_LIST in fs/ext4/sysfs.c; it
> requires adding two lines to advertise a new ext4 feature.)

I wonder if this wouldn't be nice to have independent of whether or not
there is another way to detect it?
I'm would think that people like to see all new ext4 features listed in
there. Even if this is technically a generic vfs feature.

Christian
Theodore Ts'o April 12, 2021, 10:41 p.m. UTC | #12
On Mon, Apr 12, 2021 at 01:54:26PM +0200, Christian Brauner wrote:
>So we can detect it pretty reliably at runtime by trying whether we can
>create an idmapped mount on the given filesystem. That is enough for the
>idmapped mount tests here but of course has at least two drawbacks:
>1. there might be scenarios where we get false negatives
>   (e.g. open_tree() could fail for a lack of permissions or sm else,
>   kernel might be compiled without userns support etc. pp)

In the ideal world, if the kernel wasn't compiled with the necessary
CONFIG options enabled, it's desirable of the test can detect that
fact and skip running the test instead failing and forcing the person
running the test to try to figure out whether this is a legitmate file
system bug or a just a test setup bug.

This is one of the other functions of /sys/fs/ext4/features/...  for
example:

#ifdef CONFIG_UNICODE
EXT4_ATTR_FEATURE(casefold);
#endif

That way, if a particular kernel CONFIG is not compiled in, we can
prevent the feature from being advertised, without having to rely on
/proc/config.gz be present.

>2. it's heavy in so far as we have to do the whole exercise of creating
>   a detached mount

Yeah, there are times when I've added a new feature flag file in
/sys/fs/ext4/features because it was simpler than being able to doing
some kind of heavyweight test.

> So having a reliable way to detect whether or not the underlying fs
> supports it could be worth it (My hope was for the fsinfo() API to grow
> this "feature check" ability but oh well.).
> 
> One possibility might be to extend fstatfs() and steal one u32 from the
> padding that is currently in there?

That would require mounting a file system in order to do the check,
which is not necesarily always convenient, but that would certainly
work.  (See below for an example of where it was much easier for
mke2fs to be able to test for a file system "capability" without
needing to mount a test file system.)

> > If you can't do this by checking to see if the file system will
> > support a particular mount option, or some other run-time test, for
> > ext4 we can signal this by checking for the existence of a file in
> > /sys/fs/ext4/features, such as /sys/fs/ext4/features/fast_commit.
> > (Grep for EXT4_ATTR_FEATURE and ATTR_LIST in fs/ext4/sysfs.c; it
> > requires adding two lines to advertise a new ext4 feature.)
> 
> I wonder if this wouldn't be nice to have independent of whether or not
> there is another way to detect it?
> I'm would think that people like to see all new ext4 features listed in
> there. Even if this is technically a generic vfs feature.

My apologies for the confusion.  There's a bit of overloading in terms
of the word "features" as used above.  In some cases, there is a
direct correspondance between bits that show up in the compat,
ro_compat, and incompat fields in the ext4 superblock.  In other cases
it might describe a file system behaviour (e.g., lazy_itable_init) and
this acts as a hint to e2fsprogs about what kind of defaults it should
use when creating a file system --- or to warn the user about whether
a particular file system can be mounted using the current kernel.

For example, currently we give the following warning if you create a
file system with a 64k block size:

% mke2fs -t ext4 -b 65536 /tmp/foo.img 8M
Warning: blocksize 65536 not usable on most systems.
mke2fs 1.46.2 (28-Feb-2021)
Creating regular file /tmp/foo.img
mke2fs: 65536-byte blocks too big for system (max 4096)
Proceed anyway? (y,N)

However, suppose at some point, thanks to advances in the MM layer, it
becomes possible to support file systems where the block size is
greater than the page size.  We might then advertise that via the
existing of a file such as /sys/fs/ext4/feature/big_blocks (for
example), and then newer versions of mke2fs would check for that file
and suppress the warning on a kernel which would actually allow file
systems with 64k blocks to be mounted on an x86_64 system.  This is
also why I designed it via using a file in /sys/fs/ext4/features, so
that I don't have to do a test mount and use a system call fstatfs(2)
to check for the existenace of an ext4 feature/capability (all of the
obvious terms are already overloaded, sigh).

I agree that having some kind of fs-independent way for querying
whether some feature/capability is present for a particular kernel
would be convenient.  This wouldn't necessarily need to be implemented
in the VFS, though, especially if the convention that gets adopted is
via a file in /sys/fs/<fstype>/features/<capability>.

						- Ted