Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts
       [not found]                 ` <YHTMkBcVTFAGqyks@mit.edu>
@ 2021-04-14 20:47                   ` Theodore Ts'o
  2021-04-15  5:54                     ` Christoph Hellwig
  0 siblings, 1 reply; 6+ messages in thread
From: Theodore Ts'o @ 2021-04-14 20:47 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Eryu Guan, Christian Brauner, fstests, Christoph Hellwig,
	linux-ext4, Darrick J . Wong, David Howells, Amir Goldstein

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);

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts
  2021-04-14 20:47                   ` [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts Theodore Ts'o
@ 2021-04-15  5:54                     ` Christoph Hellwig
  2021-04-15  7:49                       ` Christian Brauner
  0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2021-04-15  5:54 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: Christian Brauner, Eryu Guan, Christian Brauner, fstests,
	Christoph Hellwig, linux-ext4, Darrick J . Wong, David Howells,
	Amir Goldstein

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts
  2021-04-15  5:54                     ` Christoph Hellwig
@ 2021-04-15  7:49                       ` Christian Brauner
  2021-04-15  7:55                         ` Christoph Hellwig
  2021-04-15 14:59                         ` Theodore Ts'o
  0 siblings, 2 replies; 6+ messages in thread
From: Christian Brauner @ 2021-04-15  7:49 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Theodore Ts'o, Eryu Guan, Christian Brauner, fstests,
	linux-ext4, Darrick J . Wong, David Howells, Amir Goldstein

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts
  2021-04-15  7:49                       ` Christian Brauner
@ 2021-04-15  7:55                         ` Christoph Hellwig
  2021-04-15  8:13                           ` Christian Brauner
  2021-04-15 14:59                         ` Theodore Ts'o
  1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2021-04-15  7:55 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Christoph Hellwig, Theodore Ts'o, Eryu Guan,
	Christian Brauner, fstests, linux-ext4, Darrick J . Wong,
	David Howells, Amir Goldstein

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts
  2021-04-15  7:55                         ` Christoph Hellwig
@ 2021-04-15  8:13                           ` Christian Brauner
  0 siblings, 0 replies; 6+ messages in thread
From: Christian Brauner @ 2021-04-15  8:13 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Theodore Ts'o, Eryu Guan, Christian Brauner, fstests,
	linux-ext4, Darrick J . Wong, David Howells, Amir Goldstein

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts
  2021-04-15  7:49                       ` Christian Brauner
  2021-04-15  7:55                         ` Christoph Hellwig
@ 2021-04-15 14:59                         ` Theodore Ts'o
  1 sibling, 0 replies; 6+ messages in thread
From: Theodore Ts'o @ 2021-04-15 14:59 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Christoph Hellwig, Eryu Guan, Christian Brauner, fstests,
	linux-ext4, Darrick J . Wong, David Howells, Amir Goldstein

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20210328223400.1800301-1-brauner@kernel.org>
     [not found] ` <20210328223400.1800301-3-brauner@kernel.org>
     [not found]   ` <YHMH/JRmzg3ETcED@desktop>
     [not found]     ` <20210411151249.6y34x7yatqtpcvi6@wittgenstein>
     [not found]       ` <20210411151857.wd6gd46u53vlh2xv@wittgenstein>
     [not found]         ` <YHMUAL/oD4fB3+R7@desktop>
     [not found]           ` <20210411153223.vhcegiklrwoczy55@wittgenstein>
     [not found]             ` <YHOW7DN51YuYgLPM@mit.edu>
     [not found]               ` <20210412115426.a4bzsx4cp7jhx2ou@wittgenstein>
     [not found]                 ` <YHTMkBcVTFAGqyks@mit.edu>
2021-04-14 20:47                   ` [PATCH -RFC] ext4: add feature file to advertise that ext4 supports idmapped mounts Theodore Ts'o
2021-04-15  5:54                     ` Christoph Hellwig
2021-04-15  7:49                       ` Christian Brauner
2021-04-15  7:55                         ` Christoph Hellwig
2021-04-15  8:13                           ` Christian Brauner
2021-04-15 14:59                         ` Theodore Ts'o

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git