[PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts

[PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts

[Theodore Ts'o]

On Mon, Apr 12, 2021 at 06:41:20PM -0400, Theodore Ts'o wrote:
> 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.

So it would make it easier for me to manage running xfstests on ext4
if I had added something like this to ext4 and sent it to Linus before
v5.12 is released.  What do folks think?

							- Ted


commit 20619aefe69d39e76083d8f8598653c2dca9b47e
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Wed Apr 14 16:42:47 2021 -0400

    ext4: add feature file to advertise that ext4 supports idmapped mounts
    
    This makes it easier for automated test suites to know whether it know
    whether we should test the functionality of the new idmapped mounts
    feature introduced in v5.12-rc1.
    
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index a3d08276d441..101bf700c16b 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -313,6 +313,9 @@ EXT4_ATTR_FEATURE(verity);
 #endif
 EXT4_ATTR_FEATURE(metadata_csum_seed);
 EXT4_ATTR_FEATURE(fast_commit);
+#ifdef CONFIG_USER_NS
+EXT4_ATTR_FEATURE(idmapped_mount);
+#endif
 
 static struct attribute *ext4_feat_attrs[] = {
 	ATTR_LIST(lazy_itable_init),
@@ -330,6 +333,9 @@ static struct attribute *ext4_feat_attrs[] = {
 #endif
 	ATTR_LIST(metadata_csum_seed),
 	ATTR_LIST(fast_commit),
+#ifdef CONFIG_USER_NS
+	ATTR_LIST(idmapped_mount),
+#endif
 	NULL,
 };
 ATTRIBUTE_GROUPS(ext4_feat);

Re: [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts

[Christoph Hellwig]

On Wed, Apr 14, 2021 at 04:47:10PM -0400, Theodore Ts'o wrote:
> On Mon, Apr 12, 2021 at 06:41:20PM -0400, Theodore Ts'o wrote:
> > 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.
> 
> So it would make it easier for me to manage running xfstests on ext4
> if I had added something like this to ext4 and sent it to Linus before
> v5.12 is released.  What do folks think?

Completely stupid.  This is a VFS-level feature and we need to have
proper VFS-level testing (which we have through creating a mount
with the option), not fs-specific band aids.

Re: [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts

[Christian Brauner]

On Thu, Apr 15, 2021 at 07:54:08AM +0200, Christoph Hellwig wrote:
> On Wed, Apr 14, 2021 at 04:47:10PM -0400, Theodore Ts'o wrote:
> > On Mon, Apr 12, 2021 at 06:41:20PM -0400, Theodore Ts'o wrote:
> > > 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.
> > 
> > So it would make it easier for me to manage running xfstests on ext4
> > if I had added something like this to ext4 and sent it to Linus before
> > v5.12 is released.  What do folks think?
> 
> Completely stupid.  This is a VFS-level feature and we need to have
> proper VFS-level testing (which we have through creating a mount
> with the option), not fs-specific band aids.

Harsh words. :)
Christoph's right though I think for the xfstests we don't need it and
we're covered with what we have in the version I sent out last Sunday.

However, as a general way of advertising to users that ext4 supports
idmapped mounts I don't necessarily see a problem with that. It doesn't
have implications for other filesystems and ext4 already advertises
other features in a similar way.

Christian

Re: [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts

[Christoph Hellwig]

On Thu, Apr 15, 2021 at 09:49:21AM +0200, Christian Brauner wrote:
> However, as a general way of advertising to users that ext4 supports
> idmapped mounts I don't necessarily see a problem with that. It doesn't
> have implications for other filesystems and ext4 already advertises
> other features in a similar way.

I still think that is a very bad idea.  idmapped mounts are foremost
a VFS feature, with just a tiny bit of glue in every file system that
does not affect the on-disk format.  So any discovery for it needs
to be using VFS infrastructure.  IMHO just trying to mount is the
proper way to do it, but if people want some other form of discovery
it needs to be something in generic code.

Re: [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts

[Christian Brauner]

On Thu, Apr 15, 2021 at 09:55:37AM +0200, Christoph Hellwig wrote:
> On Thu, Apr 15, 2021 at 09:49:21AM +0200, Christian Brauner wrote:
> > However, as a general way of advertising to users that ext4 supports
> > idmapped mounts I don't necessarily see a problem with that. It doesn't
> > have implications for other filesystems and ext4 already advertises
> > other features in a similar way.
> 
> I still think that is a very bad idea.  idmapped mounts are foremost
> a VFS feature, with just a tiny bit of glue in every file system that
> does not affect the on-disk format.  So any discovery for it needs
> to be using VFS infrastructure.  IMHO just trying to mount is the
> proper way to do it, but if people want some other form of discovery
> it needs to be something in generic code.

I agree with you there.
I think fsinfo() (Arguably a slimmed-down version of it.) would've been
the best place for this.

Re: [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts

[Theodore Ts'o]

On Thu, Apr 15, 2021 at 09:49:21AM +0200, Christian Brauner wrote:
> Harsh words. :)
> Christoph's right though I think for the xfstests we don't need it and
> we're covered with what we have in the version I sent out last Sunday.

Sorry, I had missed your v13 patch set and that it had included tests
for the existence of idmapped.  I had sent out the RFC patch because I
was under the impression that we hadn't made forward progress on
testing for the support idmapped mounts.

If we have a way of doing this, and you're comfortable that it is
reliable (e.g., that a bug might cause the test to be skipped because
it thinks the file system doesn't support idmapped mount, when really
it was caused by a regression), and I'm happy to just rely on the
method you've used in the v13 fstests patch set.

That being said, I still think it would be helpful to have a
VFS-standard way for userspace to be able to test for the presence of
a particular kernel feature/capability without having to do a test
mount, possibly requiring setting up a test user_ns, etc., etc.  But
that isn't as urgent as making sure we can easily the feature without
needing manual customization of the test suite as file systems add
support for the feature, or as the feature gets backported to
enterprise distro kernels.

Cheers,

					- Ted