| Message ID | YHdUzqZ7PZtb64zf@mit.edu |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [-RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts | expand |
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.
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
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.
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.
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
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);