[PATCH 0/4] Add the ability to query mount options in statmount

[PATCH 0/4] Add the ability to query mount options in statmount

[Josef Bacik]

Hello,

Currently if you want to get mount options for a mount and you're using
statmount(), you still have to open /proc/mounts to parse the mount options.
statmount() does have the ability to store an arbitrary string however,
additionally the way we do that is with a seq_file, which is also how we use
->show_options for the individual file systems.

Extent statmount() to have a flag for fetching the mount options of a mount.
This allows users to not have to parse /proc mount for anything related to a
mount.  I've extended the existing statmount() test to validate this feature
works as expected.  As you can tell from the ridiculous amount of silly string
parsing, this is a huge win for users and climate change as we will no longer
have to waste several cycles parsing strings anymore.

This is based on my branch that extends listmount/statmount to walk into foreign
namespaces.  Below are links to that posting, that branch, and this branch to
make it easier to review.

https://lore.kernel.org/linux-fsdevel/cover.1719243756.git.josef@toxicpanda.com/
https://github.com/josefbacik/linux/tree/listmount.combined
https://github.com/josefbacik/linux/tree/statmount-opts

Thanks,

Josef

Josef Bacik (4):
  fs: rename show_mnt_opts -> show_vfsmnt_opts
  fs: add a helper to show all the options for a mount
  fs: export mount options via statmount()
  sefltests: extend the statmount test for mount options

 fs/internal.h                                 |   5 +
 fs/namespace.c                                |   7 +
 fs/proc_namespace.c                           |  29 ++--
 include/uapi/linux/mount.h                    |   3 +-
 .../filesystems/statmount/statmount_test.c    | 131 +++++++++++++++++-
 5 files changed, 164 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH 1/4] fs: rename show_mnt_opts -> show_vfsmnt_opts

[Josef Bacik]

This name is more consistent with what the helper does, which is to just
show the vfsmount options.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/proc_namespace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
index 0a808951b7d3..e133b507ddf3 100644
--- a/fs/proc_namespace.c
+++ b/fs/proc_namespace.c
@@ -61,7 +61,7 @@ static int show_sb_opts(struct seq_file *m, struct super_block *sb)
 	return security_sb_show_options(m, sb);
 }
 
-static void show_mnt_opts(struct seq_file *m, struct vfsmount *mnt)
+static void show_vfsmnt_opts(struct seq_file *m, struct vfsmount *mnt)
 {
 	static const struct proc_fs_opts mnt_opts[] = {
 		{ MNT_NOSUID, ",nosuid" },
@@ -124,7 +124,7 @@ static int show_vfsmnt(struct seq_file *m, struct vfsmount *mnt)
 	err = show_sb_opts(m, sb);
 	if (err)
 		goto out;
-	show_mnt_opts(m, mnt);
+	show_vfsmnt_opts(m, mnt);
 	if (sb->s_op->show_options)
 		err = sb->s_op->show_options(m, mnt_path.dentry);
 	seq_puts(m, " 0 0\n");
@@ -153,7 +153,7 @@ static int show_mountinfo(struct seq_file *m, struct vfsmount *mnt)
 		goto out;
 
 	seq_puts(m, mnt->mnt_flags & MNT_READONLY ? " ro" : " rw");
-	show_mnt_opts(m, mnt);
+	show_vfsmnt_opts(m, mnt);
 
 	/* Tagged fields ("foo:X" or "bar") */
 	if (IS_MNT_SHARED(r))
-- 
2.43.0

[PATCH 2/4] fs: add a helper to show all the options for a mount

[Josef Bacik]

In order to add the ability to export the mount options via statmount()
we need a helper to combine all the various mount option things we do
for /proc/mounts and /proc/$PID/mountinfo.  The helper for /proc/mounts
can use this helper, however mountinfo is slightly (and infuriatingly)
different, so it can only be used in one place.  This helper will be
used in a followup patch to export mount options via statmount().

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/internal.h       |  5 +++++
 fs/proc_namespace.c | 25 ++++++++++++++++++-------
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/fs/internal.h b/fs/internal.h
index 84f371193f74..dc40c9d4173f 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -321,3 +321,8 @@ struct stashed_operations {
 int path_from_stashed(struct dentry **stashed, struct vfsmount *mnt, void *data,
 		      struct path *path);
 void stashed_dentry_prune(struct dentry *dentry);
+
+/*
+ * fs/proc_namespace.c
+ */
+int show_mount_opts(struct seq_file *seq, struct vfsmount *mnt);
diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
index e133b507ddf3..19bffd9d80dc 100644
--- a/fs/proc_namespace.c
+++ b/fs/proc_namespace.c
@@ -84,6 +84,22 @@ static void show_vfsmnt_opts(struct seq_file *m, struct vfsmount *mnt)
 		seq_puts(m, ",idmapped");
 }
 
+int show_mount_opts(struct seq_file *seq, struct vfsmount *mnt)
+{
+	struct dentry *dentry = mnt->mnt_root;
+	struct super_block *sb = dentry->d_sb;
+	int ret;
+
+	seq_puts(seq, __mnt_is_readonly(mnt) ? "ro" : "rw");
+	ret = show_sb_opts(seq, sb);
+	if (ret)
+		return ret;
+	show_vfsmnt_opts(seq, mnt);
+	if (sb->s_op->show_options)
+		ret = sb->s_op->show_options(seq, dentry);
+	return ret;
+}
+
 static inline void mangle(struct seq_file *m, const char *s)
 {
 	seq_escape(m, s, " \t\n\\#");
@@ -120,13 +136,8 @@ static int show_vfsmnt(struct seq_file *m, struct vfsmount *mnt)
 		goto out;
 	seq_putc(m, ' ');
 	show_type(m, sb);
-	seq_puts(m, __mnt_is_readonly(mnt) ? " ro" : " rw");
-	err = show_sb_opts(m, sb);
-	if (err)
-		goto out;
-	show_vfsmnt_opts(m, mnt);
-	if (sb->s_op->show_options)
-		err = sb->s_op->show_options(m, mnt_path.dentry);
+	seq_putc(m, ' ');
+	err = show_mount_opts(m, mnt);
 	seq_puts(m, " 0 0\n");
 out:
 	return err;
-- 
2.43.0

Re: [PATCH 2/4] fs: add a helper to show all the options for a mount

[Christian Brauner]

On Mon, Jun 24, 2024 at 03:40:51PM GMT, Josef Bacik wrote:
> In order to add the ability to export the mount options via statmount()
> we need a helper to combine all the various mount option things we do
> for /proc/mounts and /proc/$PID/mountinfo.  The helper for /proc/mounts
> can use this helper, however mountinfo is slightly (and infuriatingly)
> different, so it can only be used in one place.  This helper will be
> used in a followup patch to export mount options via statmount().
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/internal.h       |  5 +++++
>  fs/proc_namespace.c | 25 ++++++++++++++++++-------
>  2 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/internal.h b/fs/internal.h
> index 84f371193f74..dc40c9d4173f 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -321,3 +321,8 @@ struct stashed_operations {
>  int path_from_stashed(struct dentry **stashed, struct vfsmount *mnt, void *data,
>  		      struct path *path);
>  void stashed_dentry_prune(struct dentry *dentry);
> +
> +/*
> + * fs/proc_namespace.c
> + */
> +int show_mount_opts(struct seq_file *seq, struct vfsmount *mnt);
> diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
> index e133b507ddf3..19bffd9d80dc 100644
> --- a/fs/proc_namespace.c
> +++ b/fs/proc_namespace.c
> @@ -84,6 +84,22 @@ static void show_vfsmnt_opts(struct seq_file *m, struct vfsmount *mnt)
>  		seq_puts(m, ",idmapped");
>  }
>  
> +int show_mount_opts(struct seq_file *seq, struct vfsmount *mnt)
> +{
> +	struct dentry *dentry = mnt->mnt_root;
> +	struct super_block *sb = dentry->d_sb;
> +	int ret;
> +
> +	seq_puts(seq, __mnt_is_readonly(mnt) ? "ro" : "rw");
> +	ret = show_sb_opts(seq, sb);
> +	if (ret)
> +		return ret;
> +	show_vfsmnt_opts(seq, mnt);
> +	if (sb->s_op->show_options)
> +		ret = sb->s_op->show_options(seq, dentry);
> +	return ret;
> +}
> +
>  static inline void mangle(struct seq_file *m, const char *s)
>  {
>  	seq_escape(m, s, " \t\n\\#");
> @@ -120,13 +136,8 @@ static int show_vfsmnt(struct seq_file *m, struct vfsmount *mnt)
>  		goto out;
>  	seq_putc(m, ' ');
>  	show_type(m, sb);
> -	seq_puts(m, __mnt_is_readonly(mnt) ? " ro" : " rw");
> -	err = show_sb_opts(m, sb);
> -	if (err)
> -		goto out;
> -	show_vfsmnt_opts(m, mnt);
> -	if (sb->s_op->show_options)
> -		err = sb->s_op->show_options(m, mnt_path.dentry);
> +	seq_putc(m, ' ');
> +	err = show_mount_opts(m, mnt);
>  	seq_puts(m, " 0 0\n");
>  out:
>  	return err;
> -- 
> 2.43.0
> 

So Karel just made me aware of this. You're using the old /proc/mounts
format here and you're mixing generic sb options, mount options, and fs
specific mount options.

For example, the mount options you're currently passing contain "ro" and
"rw". But these can be either per-mount setting or superblock settings
and that isn't distinguiable based on "ro" and "rw". That information is
also already contained and differentiated in statmount->sb_flags vs
statmount->mnt_attr.

Neither should or does show_vfsmnt_opts() need to be called as all that
information is present in statmount->mnt_attr.

Please only call ->show_options().

Re: [PATCH 2/4] fs: add a helper to show all the options for a mount

[Karel Zak]

On Tue, Jun 25, 2024 at 04:16:47PM GMT, Christian Brauner wrote:
> On Mon, Jun 24, 2024 at 03:40:51PM GMT, Josef Bacik wrote:
> So Karel just made me aware of this. You're using the old /proc/mounts
> format here and you're mixing generic sb options, mount options, and fs
> specific mount options.

Yes, the patch takes us back in time to the era of /etc/mtab when vfs
and fs options were merged into a single string. For example,
/proc/self/mountinfo separates the options into two fields.

> Please only call ->show_options().

 +1

 Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

[PATCH 3/4] fs: export mount options via statmount()

[Josef Bacik]

statmount() can export arbitrary strings, so utilize the __spare1 slot
for a mnt_opts string pointer, and then support asking for and setting
the mount options during statmount().  This calls into the helper for
showing mount options, which already uses a seq_file, so fits in nicely
with our existing mechanism for exporting strings via statmount().

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/namespace.c             | 7 +++++++
 include/uapi/linux/mount.h | 3 ++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 6d44537fd78c..2a6a8cbf5d0a 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5003,6 +5003,10 @@ static int statmount_string(struct kstatmount *s, u64 flag)
 		sm->mnt_point = seq->count;
 		ret = statmount_mnt_point(s, seq);
 		break;
+	case STATMOUNT_MNT_OPTS:
+		sm->mnt_opts = seq->count;
+		ret = show_mount_opts(seq, s->mnt);
+		break;
 	default:
 		WARN_ON_ONCE(true);
 		return -EINVAL;
@@ -5079,6 +5083,9 @@ static int do_statmount(struct kstatmount *s)
 	if (!err && s->mask & STATMOUNT_MNT_POINT)
 		err = statmount_string(s, STATMOUNT_MNT_POINT);
 
+	if (!err && s->mask & STATMOUNT_MNT_OPTS)
+		err = statmount_string(s, STATMOUNT_MNT_OPTS);
+
 	if (!err && s->mask & STATMOUNT_MNT_NS_ID)
 		statmount_mnt_ns_id(s, ns);
 
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index ee1559cd6764..225bc366ffcb 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -154,7 +154,7 @@ struct mount_attr {
  */
 struct statmount {
 	__u32 size;		/* Total size, including strings */
-	__u32 __spare1;
+	__u32 mnt_opts;		/* [str] Mount options of the mount */
 	__u64 mask;		/* What results were written */
 	__u32 sb_dev_major;	/* Device ID */
 	__u32 sb_dev_minor;
@@ -206,6 +206,7 @@ struct mnt_id_req {
 #define STATMOUNT_MNT_POINT		0x00000010U	/* Want/got mnt_point */
 #define STATMOUNT_FS_TYPE		0x00000020U	/* Want/got fs_type */
 #define STATMOUNT_MNT_NS_ID		0x00000040U	/* Want/got mnt_ns_id */
+#define STATMOUNT_MNT_OPTS		0x00000080U	/* Want/got mnt_opts */
 
 /*
  * Special @mnt_id values that can be passed to listmount
-- 
2.43.0

[PATCH 4/4] sefltests: extend the statmount test for mount options

[Josef Bacik]

Now that we support exporting mount options, via statmount(), add a test
to validate that it works.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 .../filesystems/statmount/statmount_test.c    | 131 +++++++++++++++++-
 1 file changed, 130 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/filesystems/statmount/statmount_test.c b/tools/testing/selftests/filesystems/statmount/statmount_test.c
index 4f7023c2de77..96696183d27b 100644
--- a/tools/testing/selftests/filesystems/statmount/statmount_test.c
+++ b/tools/testing/selftests/filesystems/statmount/statmount_test.c
@@ -64,6 +64,20 @@ static struct statmount *statmount_alloc(uint64_t mnt_id, uint64_t mask, unsigne
 	return buf;
 }
 
+static void read_file(const char *path, void *buf, size_t len)
+{
+	int fd = open(path, O_RDONLY);
+	ssize_t ret;
+
+	if (fd < 0)
+		ksft_exit_fail_msg("opening %s for read: %s\n", path, strerror(errno));
+
+	ret = read(fd, buf, len);
+	if (ret < 0)
+		ksft_exit_fail_msg("reading from %s: %s\n", path, strerror(errno));
+	close(fd);
+}
+
 static void write_file(const char *path, const char *val)
 {
 	int fd = open(path, O_WRONLY);
@@ -107,6 +121,8 @@ static char root_mntpoint[] = "/tmp/statmount_test_root.XXXXXX";
 static int orig_root;
 static uint64_t root_id, parent_id;
 static uint32_t old_root_id, old_parent_id;
+static char proc_mounts_buf[4096];
+static char proc_mountinfo_buf[4096];
 
 static void cleanup_namespace(void)
 {
@@ -134,6 +150,11 @@ static void setup_namespace(void)
 	sprintf(buf, "0 %d 1", gid);
 	write_file("/proc/self/gid_map", buf);
 
+	memset(proc_mounts_buf, 0, 4096);
+	memset(proc_mountinfo_buf, 0, 4096);
+	read_file("/proc/self/mounts", proc_mounts_buf, 4095);
+	read_file("/proc/self/mountinfo", proc_mountinfo_buf, 4095);
+
 	ret = mount("", "/", NULL, MS_REC|MS_PRIVATE, NULL);
 	if (ret == -1)
 		ksft_exit_fail_msg("making mount tree private: %s\n",
@@ -435,6 +456,113 @@ static void test_statmount_fs_type(void)
 	free(sm);
 }
 
+/* Search proc_mountinfo_buf for our mount point. */
+static char *find_mnt_point(void)
+{
+	char buf[256];
+	char *cur, *end;
+
+	snprintf(buf, 256, "%llu", (unsigned long long)old_parent_id);
+	cur = strstr(proc_mountinfo_buf, buf);
+	if (!cur)
+		ksft_exit_fail_msg("couldn't find %llu in /proc/self/mountinfo\n",
+				   (unsigned long long)old_parent_id);
+
+	/*
+	 * The format is
+	 *
+	 * <mnt id> <parent mnt id> <device> <parent mnt point> <mnt point>
+	 *
+	 * We are currently at <mnt id>, we must skip the next 4 columns.
+	 */
+	for (int i = 0; i < 4; i++) {
+		cur = strchr(cur, ' ');
+		if (!cur)
+			ksft_exit_fail_msg("/proc/self/mountinfo isn't formatted as expected\n");
+		cur++;
+	}
+
+	/*
+	 * We are now positioned right at <mnt point>, find the next space and
+	 * \0 it out so we have the mount point isolated.
+	 */
+	end = strchr(cur, ' ');
+	if (!end)
+		ksft_exit_fail_msg("/proc/self/mountinfo isn't formatted as expected\n");
+	*end = '\0';
+	return cur;
+}
+
+static void test_statmount_mnt_opts(void)
+{
+	struct statmount *sm;
+	char search_buf[256];
+	const char *opts;
+	const char *statmount_opts;
+	const char *mntpoint;
+	char *end;
+
+	sm = statmount_alloc(root_id, STATMOUNT_MNT_OPTS, 0);
+	if (!sm) {
+		ksft_test_result_fail("statmount mnt opts: %s\n",
+				      strerror(errno));
+		return;
+	}
+
+	mntpoint = find_mnt_point();
+	snprintf(search_buf, 256, " %s ", mntpoint);
+
+	/*
+	 * This fun bit of string parsing is to extract out the mount options
+	 * for our root id.  Normally it would be the first entry but we don't
+	 * want to rely on on that, so we're going to search for the path
+	 * manually.  The format is
+	 *
+	 * <device> <mnt point> <fs type> <mnt options> ...
+	 *
+	 * We start by searching for <mnt point> and then advancing along until
+	 * we get the <mnt options> field.
+	 */
+
+	/* Look for the root entry. */
+	opts = strstr(proc_mounts_buf, search_buf);
+	if (!opts)
+		ksft_exit_fail_msg("no mount entry for / in /proc/mounts\n");
+
+	/* Now advance us past that chunk, and into the fstype. */
+	opts += strlen(search_buf);
+
+	/* Now find the next space. */
+	opts = strchr(opts, ' ');
+	if (!opts)
+		ksft_exit_fail_msg("/proc/mounts isn't formatted as expected\n");
+
+	/* Now advance one to the start of where the mount options should be. */
+	opts++;
+
+	/*
+	 * Now we go all the way to the end of opts and set that value to \0 so
+	 * we can easily strcmp what we got from statmount().
+	 *
+	 * If the mount options have a space in them this will mess up the test,
+	 * but I don't know if that happens anywhere.  If this fails on that
+	 * then we need to update the test to handle that, but for now I'm going
+	 * to pretend lik that never happens.
+	 */
+	end = strchr(opts, ' ');
+	if (!end)
+		ksft_exit_fail_msg("/proc/mounts isn't formatted as expected\n");
+	*end = '\0';
+
+	statmount_opts = sm->str + sm->mnt_opts;
+	if (strcmp(statmount_opts, opts) != 0)
+		ksft_test_result_fail("unexpected mount options: '%s' != '%s'\n",
+				      statmount_opts, opts);
+	else
+		ksft_test_result_pass("statmount mount options\n");
+	free(sm);
+}
+
 static void test_statmount_string(uint64_t mask, size_t off, const char *name)
 {
 	struct statmount *sm;
@@ -561,7 +689,7 @@ int main(void)
 
 	setup_namespace();
 
-	ksft_set_plan(14);
+	ksft_set_plan(15);
 	test_listmount_empty_root();
 	test_statmount_zero_mask();
 	test_statmount_mnt_basic();
@@ -569,6 +697,7 @@ int main(void)
 	test_statmount_mnt_root();
 	test_statmount_mnt_point();
 	test_statmount_fs_type();
+	test_statmount_mnt_opts();
 	test_statmount_string(STATMOUNT_MNT_ROOT, str_off(mnt_root), "mount root");
 	test_statmount_string(STATMOUNT_MNT_POINT, str_off(mnt_point), "mount point");
 	test_statmount_string(STATMOUNT_FS_TYPE, str_off(fs_type), "fs type");
-- 
2.43.0

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Jeff Layton]

On Mon, 2024-06-24 at 15:40 -0400, Josef Bacik wrote:
> Hello,
> 
> Currently if you want to get mount options for a mount and you're
> using
> statmount(), you still have to open /proc/mounts to parse the mount
> options.
> statmount() does have the ability to store an arbitrary string
> however,
> additionally the way we do that is with a seq_file, which is also how
> we use
> ->show_options for the individual file systems.
> 
> Extent statmount() to have a flag for fetching the mount options of a
> mount.
> This allows users to not have to parse /proc mount for anything
> related to a
> mount.  I've extended the existing statmount() test to validate this
> feature
> works as expected.  As you can tell from the ridiculous amount of
> silly string
> parsing, this is a huge win for users and climate change as we will
> no longer
> have to waste several cycles parsing strings anymore.
> 
> This is based on my branch that extends listmount/statmount to walk
> into foreign
> namespaces.  Below are links to that posting, that branch, and this
> branch to
> make it easier to review.
> 
> https://lore.kernel.org/linux-fsdevel/cover.1719243756.git.josef@toxicpanda.com/
> https://github.com/josefbacik/linux/tree/listmount.combined
> https://github.com/josefbacik/linux/tree/statmount-opts
> 
> Thanks,
> 
> Josef
> 
> Josef Bacik (4):
>   fs: rename show_mnt_opts -> show_vfsmnt_opts
>   fs: add a helper to show all the options for a mount
>   fs: export mount options via statmount()
>   sefltests: extend the statmount test for mount options
> 
>  fs/internal.h                                 |   5 +
>  fs/namespace.c                                |   7 +
>  fs/proc_namespace.c                           |  29 ++--
>  include/uapi/linux/mount.h                    |   3 +-
>  .../filesystems/statmount/statmount_test.c    | 131
> +++++++++++++++++-
>  5 files changed, 164 insertions(+), 11 deletions(-)
> 

Nice work. I especially like that there is a selftest now.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Christian Brauner]

On Mon, Jun 24, 2024 at 03:40:49PM GMT, Josef Bacik wrote:
> Hello,
> 
> Currently if you want to get mount options for a mount and you're using
> statmount(), you still have to open /proc/mounts to parse the mount options.
> statmount() does have the ability to store an arbitrary string however,
> additionally the way we do that is with a seq_file, which is also how we use
> ->show_options for the individual file systems.
> 
> Extent statmount() to have a flag for fetching the mount options of a mount.
> This allows users to not have to parse /proc mount for anything related to a
> mount.  I've extended the existing statmount() test to validate this feature
> works as expected.  As you can tell from the ridiculous amount of silly string
> parsing, this is a huge win for users and climate change as we will no longer
> have to waste several cycles parsing strings anymore.
> 
> This is based on my branch that extends listmount/statmount to walk into foreign
> namespaces.  Below are links to that posting, that branch, and this branch to
> make it easier to review.

So I was very hesitant to do it this way because I feel this is pretty
ugly dumping mount options like that but Karel and others have the same
use-case that they want to retrieve it all via statmount() (or another
ID-based system call) so I guess I'll live with this. But note that this
will be a fairly ugly interface at times. For example, mounting overlayfs with
500 lower layers then what one gets is:

/mnt/test
mnt_ns_id: 2
mnt_id: 4294969217
mnt_id_old: 820
mnt_parent_id: 4294967529
mnt_opts: rw,relatime,lowerdir+=/mnt/tmp1,
lowerdir+=/mnt/tmp2,lowerdir+=/mnt/tmp3,lowerdir+=/mnt/tmp4,lowerdir+=/mnt/tmp5,
lowerdir+=/mnt/tmp6,lowerdir+=/mnt/tmp7,lowerdir+=/mnt/tmp8,lowerdir+=/mnt/tmp9,
lowerdir+=/mnt/tmp10,lowerdir+=/mnt/tmp11,lowerdir+=/mnt/tmp12,
lowerdir+=/mnt/tmp13,lowerdir+=/mnt/tmp14,lowerdir+=/mnt/tmp15,
lowerdir+=/mnt/tmp16,lowerdir+=/mnt/tmp17,lowerdir+=/mnt/tmp18,
lowerdir+=/mnt/tmp19,lowerdir+=/mnt/tmp20,lowerdir+=/mnt/tmp21,
lowerdir+=/mnt/tmp22,lowerdir+=/mnt/tmp23,lowerdir+=/mnt/tmp24,
lowerdir+=/mnt/tmp25,lowerdir+=/mnt/tmp26,lowerdir+=/mnt/tmp27,
lowerdir+=/mnt/tmp28,lowerdir+=/mnt/tmp29,lowerdir+=/mnt/tmp30,
lowerdir+=/mnt/tmp31,lowerdir+=/mnt/tmp32,lowerdir+=/mnt/tmp33,
lowerdir+=/mnt/tmp34,lowerdir+=/mnt/tmp35,lowerdir+=/mnt/tmp36,
lowerdir+=/mnt/tmp37,lowerdir+=/mnt/tmp38,lowerdir+=/mnt/tmp39,
lowerdir+=/mnt/tmp40,lowerdir+=/mnt/tmp41,lowerdir+=/mnt/tmp42,
lowerdir+=/mnt/tmp43,lowerdir+=/mnt/tmp44,lowerdir+=/mnt/tmp45,
lowerdir+=/mnt/tmp46,lowerdir+=/mnt/tmp47,lowerdir+=/mnt/tmp48,
lowerdir+=/mnt/tmp49,lowerdir+=/mnt/tmp50,lowerdir+=/mnt/tmp51,
lowerdir+=/mnt/tmp52,lowerdir+=/mnt/tmp53,lowerdir+=/mnt/tmp54,
lowerdir+=/mnt/tmp55,lowerdir+=/mnt/tmp56,lowerdir+=/mnt/tmp57,
lowerdir+=/mnt/tmp58,lowerdir+=/mnt/tmp59,lowerdir+=/mnt/tmp60,
lowerdir+=/mnt/tmp61,lowerdir+=/mnt/tmp62,lowerdir+=/mnt/tmp63,
lowerdir+=/mnt/tmp64,lowerdir+=/mnt/tmp65,lowerdir+=/mnt/tmp66,
lowerdir+=/mnt/tmp67,lowerdir+=/mnt/tmp68,lowerdir+=/mnt/tmp69,
lowerdir+=/mnt/tmp70,lowerdir+=/mnt/tmp71,lowerdir+=/mnt/tmp72,
lowerdir+=/mnt/tmp73,lowerdir+=/mnt/tmp74,lowerdir+=/mnt/tmp75,
lowerdir+=/mnt/tmp76,lowerdir+=/mnt/tmp77,lowerdir+=/mnt/tmp78,
lowerdir+=/mnt/tmp79,lowerdir+=/mnt/tmp80,lowerdir+=/mnt/tmp81,
lowerdir+=/mnt/tmp82,lowerdir+=/mnt/tmp83,lowerdir+=/mnt/tmp84,
lowerdir+=/mnt/tmp85,lowerdir+=/mnt/tmp86,lowerdir+=/mnt/tmp87,
lowerdir+=/mnt/tmp88,lowerdir+=/mnt/tmp89,lowerdir+=/mnt/tmp90,
lowerdir+=/mnt/tmp91,lowerdir+=/mnt/tmp92,lowerdir+=/mnt/tmp93,
lowerdir+=/mnt/tmp94,lowerdir+=/mnt/tmp95,lowerdir+=/mnt/tmp96,
lowerdir+=/mnt/tmp97,lowerdir+=/mnt/tmp98,lowerdir+=/mnt/tmp99,
lowerdir+=/mnt/tmp100,lowerdir+=/mnt/tmp101,lowerdir+=/mnt/tmp102,
lowerdir+=/mnt/tmp103,lowerdir+=/mnt/tmp104,lowerdir+=/mnt/tmp105,
lowerdir+=/mnt/tmp106,lowerdir+=/mnt/tmp107,lowerdir+=/mnt/tmp108,
lowerdir+=/mnt/tmp109,lowerdir+=/mnt/tmp110,lowerdir+=/mnt/tmp111,
lowerdir+=/mnt/tmp112,lowerdir+=/mnt/tmp113,lowerdir+=/mnt/tmp114,
lowerdir+=/mnt/tmp115,lowerdir+=/mnt/tmp116,lowerdir+=/mnt/tmp117,
lowerdir+=/mnt/tmp118,lowerdir+=/mnt/tmp119,lowerdir+=/mnt/tmp120,
lowerdir+=/mnt/tmp121,lowerdir+=/mnt/tmp122,lowerdir+=/mnt/tmp123,
lowerdir+=/mnt/tmp124,lowerdir+=/mnt/tmp125,lowerdir+=/mnt/tmp126,
lowerdir+=/mnt/tmp127,lowerdir+=/mnt/tmp128,lowerdir+=/mnt/tmp129,
lowerdir+=/mnt/tmp130,lowerdir+=/mnt/tmp131,lowerdir+=/mnt/tmp132,
lowerdir+=/mnt/tmp133,lowerdir+=/mnt/tmp134,lowerdir+=/mnt/tmp135,
lowerdir+=/mnt/tmp136,lowerdir+=/mnt/tmp137,lowerdir+=/mnt/tmp138,
lowerdir+=/mnt/tmp139,lowerdir+=/mnt/tmp140,lowerdir+=/mnt/tmp141,
lowerdir+=/mnt/tmp142,lowerdir+=/mnt/tmp143,lowerdir+=/mnt/tmp144,
lowerdir+=/mnt/tmp145,lowerdir+=/mnt/tmp146,lowerdir+=/mnt/tmp147,
lowerdir+=/mnt/tmp148,lowerdir+=/mnt/tmp149,lowerdir+=/mnt/tmp150,
lowerdir+=/mnt/tmp151,lowerdir+=/mnt/tmp152,lowerdir+=/mnt/tmp153,
lowerdir+=/mnt/tmp154,lowerdir+=/mnt/tmp155,lowerdir+=/mnt/tmp156,
lowerdir+=/mnt/tmp157,lowerdir+=/mnt/tmp158,lowerdir+=/mnt/tmp159,
lowerdir+=/mnt/tmp160,lowerdir+=/mnt/tmp161,lowerdir+=/mnt/tmp162,
lowerdir+=/mnt/tmp163,lowerdir+=/mnt/tmp164,lowerdir+=/mnt/tmp165,
lowerdir+=/mnt/tmp166,lowerdir+=/mnt/tmp167,lowerdir+=/mnt/tmp168,
lowerdir+=/mnt/tmp169,lowerdir+=/mnt/tmp170,lowerdir+=/mnt/tmp171,
lowerdir+=/mnt/tmp172,lowerdir+=/mnt/tmp173,lowerdir+=/mnt/tmp174,
lowerdir+=/mnt/tmp175,lowerdir+=/mnt/tmp176,lowerdir+=/mnt/tmp177,
lowerdir+=/mnt/tmp178,lowerdir+=/mnt/tmp179,lowerdir+=/mnt/tmp180,
lowerdir+=/mnt/tmp181,lowerdir+=/mnt/tmp182,lowerdir+=/mnt/tmp183,
lowerdir+=/mnt/tmp184,lowerdir+=/mnt/tmp185,lowerdir+=/mnt/tmp186,
lowerdir+=/mnt/tmp187,lowerdir+=/mnt/tmp188,lowerdir+=/mnt/tmp189,
lowerdir+=/mnt/tmp190,lowerdir+=/mnt/tmp191,lowerdir+=/mnt/tmp192,
lowerdir+=/mnt/tmp193,lowerdir+=/mnt/tmp194,lowerdir+=/mnt/tmp195,
lowerdir+=/mnt/tmp196,lowerdir+=/mnt/tmp197,lowerdir+=/mnt/tmp198,
lowerdir+=/mnt/tmp199,lowerdir+=/mnt/tmp200,lowerdir+=/mnt/tmp201,
lowerdir+=/mnt/tmp202,lowerdir+=/mnt/tmp203,lowerdir+=/mnt/tmp204,
lowerdir+=/mnt/tmp205,lowerdir+=/mnt/tmp206,lowerdir+=/mnt/tmp207,
lowerdir+=/mnt/tmp208,lowerdir+=/mnt/tmp209,lowerdir+=/mnt/tmp210,
lowerdir+=/mnt/tmp211,lowerdir+=/mnt/tmp212,lowerdir+=/mnt/tmp213,
lowerdir+=/mnt/tmp214,lowerdir+=/mnt/tmp215,lowerdir+=/mnt/tmp216,
lowerdir+=/mnt/tmp217,lowerdir+=/mnt/tmp218,lowerdir+=/mnt/tmp219,
lowerdir+=/mnt/tmp220,lowerdir+=/mnt/tmp221,lowerdir+=/mnt/tmp222,
lowerdir+=/mnt/tmp223,lowerdir+=/mnt/tmp224,lowerdir+=/mnt/tmp225,
lowerdir+=/mnt/tmp226,lowerdir+=/mnt/tmp227,lowerdir+=/mnt/tmp228,
lowerdir+=/mnt/tmp229,lowerdir+=/mnt/tmp230,lowerdir+=/mnt/tmp231,
lowerdir+=/mnt/tmp232,lowerdir+=/mnt/tmp233,lowerdir+=/mnt/tmp234,
lowerdir+=/mnt/tmp235,lowerdir+=/mnt/tmp236,lowerdir+=/mnt/tmp237,
lowerdir+=/mnt/tmp238,lowerdir+=/mnt/tmp239,lowerdir+=/mnt/tmp240,
lowerdir+=/mnt/tmp241,lowerdir+=/mnt/tmp242,lowerdir+=/mnt/tmp243,
lowerdir+=/mnt/tmp244,lowerdir+=/mnt/tmp245,lowerdir+=/mnt/tmp246,
lowerdir+=/mnt/tmp247,lowerdir+=/mnt/tmp248,lowerdir+=/mnt/tmp249,
lowerdir+=/mnt/tmp250,lowerdir+=/mnt/tmp251,lowerdir+=/mnt/tmp252,
lowerdir+=/mnt/tmp253,lowerdir+=/mnt/tmp254,lowerdir+=/mnt/tmp255,
lowerdir+=/mnt/tmp256,lowerdir+=/mnt/tmp257,lowerdir+=/mnt/tmp258,
lowerdir+=/mnt/tmp259,lowerdir+=/mnt/tmp260,lowerdir+=/mnt/tmp261,
lowerdir+=/mnt/tmp262,lowerdir+=/mnt/tmp263,lowerdir+=/mnt/tmp264,
lowerdir+=/mnt/tmp265,lowerdir+=/mnt/tmp266,lowerdir+=/mnt/tmp267,
lowerdir+=/mnt/tmp268,lowerdir+=/mnt/tmp269,lowerdir+=/mnt/tmp270,
lowerdir+=/mnt/tmp271,lowerdir+=/mnt/tmp272,lowerdir+=/mnt/tmp273,
lowerdir+=/mnt/tmp274,lowerdir+=/mnt/tmp275,lowerdir+=/mnt/tmp276,
lowerdir+=/mnt/tmp277,lowerdir+=/mnt/tmp278,lowerdir+=/mnt/tmp279,
lowerdir+=/mnt/tmp280,lowerdir+=/mnt/tmp281,lowerdir+=/mnt/tmp282,
lowerdir+=/mnt/tmp283,lowerdir+=/mnt/tmp284,lowerdir+=/mnt/tmp285,
lowerdir+=/mnt/tmp286,lowerdir+=/mnt/tmp287,lowerdir+=/mnt/tmp288,
lowerdir+=/mnt/tmp289,lowerdir+=/mnt/tmp290,lowerdir+=/mnt/tmp291,
lowerdir+=/mnt/tmp292,lowerdir+=/mnt/tmp293,lowerdir+=/mnt/tmp294,
lowerdir+=/mnt/tmp295,lowerdir+=/mnt/tmp296,lowerdir+=/mnt/tmp297,
lowerdir+=/mnt/tmp298,lowerdir+=/mnt/tmp299,lowerdir+=/mnt/tmp300,
lowerdir+=/mnt/tmp301,lowerdir+=/mnt/tmp302,lowerdir+=/mnt/tmp303,
lowerdir+=/mnt/tmp304,lowerdir+=/mnt/tmp305,lowerdir+=/mnt/tmp306,
lowerdir+=/mnt/tmp307,lowerdir+=/mnt/tmp308,lowerdir+=/mnt/tmp309,
lowerdir+=/mnt/tmp310,lowerdir+=/mnt/tmp311,lowerdir+=/mnt/tmp312,
lowerdir+=/mnt/tmp313,lowerdir+=/mnt/tmp314,lowerdir+=/mnt/tmp315,
lowerdir+=/mnt/tmp316,lowerdir+=/mnt/tmp317,lowerdir+=/mnt/tmp318,
lowerdir+=/mnt/tmp319,lowerdir+=/mnt/tmp320,lowerdir+=/mnt/tmp321,
lowerdir+=/mnt/tmp322,lowerdir+=/mnt/tmp323,lowerdir+=/mnt/tmp324,
lowerdir+=/mnt/tmp325,lowerdir+=/mnt/tmp326,lowerdir+=/mnt/tmp327,
lowerdir+=/mnt/tmp328,lowerdir+=/mnt/tmp329,lowerdir+=/mnt/tmp330,
lowerdir+=/mnt/tmp331,lowerdir+=/mnt/tmp332,lowerdir+=/mnt/tmp333,
lowerdir+=/mnt/tmp334,lowerdir+=/mnt/tmp335,lowerdir+=/mnt/tmp336,
lowerdir+=/mnt/tmp337,lowerdir+=/mnt/tmp338,lowerdir+=/mnt/tmp339,
lowerdir+=/mnt/tmp340,lowerdir+=/mnt/tmp341,lowerdir+=/mnt/tmp342,
lowerdir+=/mnt/tmp343,lowerdir+=/mnt/tmp344,lowerdir+=/mnt/tmp345,
lowerdir+=/mnt/tmp346,lowerdir+=/mnt/tmp347,lowerdir+=/mnt/tmp348,
lowerdir+=/mnt/tmp349,lowerdir+=/mnt/tmp350,lowerdir+=/mnt/tmp351,
lowerdir+=/mnt/tmp352,lowerdir+=/mnt/tmp353,lowerdir+=/mnt/tmp354,
lowerdir+=/mnt/tmp355,lowerdir+=/mnt/tmp356,lowerdir+=/mnt/tmp357,
lowerdir+=/mnt/tmp358,lowerdir+=/mnt/tmp359,lowerdir+=/mnt/tmp360,
lowerdir+=/mnt/tmp361,lowerdir+=/mnt/tmp362,lowerdir+=/mnt/tmp363,
lowerdir+=/mnt/tmp364,lowerdir+=/mnt/tmp365,lowerdir+=/mnt/tmp366,
lowerdir+=/mnt/tmp367,lowerdir+=/mnt/tmp368,lowerdir+=/mnt/tmp369,
lowerdir+=/mnt/tmp370,lowerdir+=/mnt/tmp371,lowerdir+=/mnt/tmp372,
lowerdir+=/mnt/tmp373,lowerdir+=/mnt/tmp374,lowerdir+=/mnt/tmp375,
lowerdir+=/mnt/tmp376,lowerdir+=/mnt/tmp377,lowerdir+=/mnt/tmp378,
lowerdir+=/mnt/tmp379,lowerdir+=/mnt/tmp380,lowerdir+=/mnt/tmp381,
lowerdir+=/mnt/tmp382,lowerdir+=/mnt/tmp383,lowerdir+=/mnt/tmp384,
lowerdir+=/mnt/tmp385,lowerdir+=/mnt/tmp386,lowerdir+=/mnt/tmp387,
lowerdir+=/mnt/tmp388,lowerdir+=/mnt/tmp389,lowerdir+=/mnt/tmp390,
lowerdir+=/mnt/tmp391,lowerdir+=/mnt/tmp392,lowerdir+=/mnt/tmp393,
lowerdir+=/mnt/tmp394,lowerdir+=/mnt/tmp395,lowerdir+=/mnt/tmp396,
lowerdir+=/mnt/tmp397,lowerdir+=/mnt/tmp398,lowerdir+=/mnt/tmp399,
lowerdir+=/mnt/tmp400,lowerdir+=/mnt/tmp401,lowerdir+=/mnt/tmp402,
lowerdir+=/mnt/tmp403,lowerdir+=/mnt/tmp404,lowerdir+=/mnt/tmp405,
lowerdir+=/mnt/tmp406,lowerdir+=/mnt/tmp407,lowerdir+=/mnt/tmp408,
lowerdir+=/mnt/tmp409,lowerdir+=/mnt/tmp410,lowerdir+=/mnt/tmp411,
lowerdir+=/mnt/tmp412,lowerdir+=/mnt/tmp413,lowerdir+=/mnt/tmp414,
lowerdir+=/mnt/tmp415,lowerdir+=/mnt/tmp416,lowerdir+=/mnt/tmp417,
lowerdir+=/mnt/tmp418,lowerdir+=/mnt/tmp419,lowerdir+=/mnt/tmp420,
lowerdir+=/mnt/tmp421,lowerdir+=/mnt/tmp422,lowerdir+=/mnt/tmp423,
lowerdir+=/mnt/tmp424,lowerdir+=/mnt/tmp425,lowerdir+=/mnt/tmp426,
lowerdir+=/mnt/tmp427,lowerdir+=/mnt/tmp428,lowerdir+=/mnt/tmp429,
lowerdir+=/mnt/tmp430,lowerdir+=/mnt/tmp431,lowerdir+=/mnt/tmp432,
lowerdir+=/mnt/tmp433,lowerdir+=/mnt/tmp434,lowerdir+=/mnt/tmp435,
lowerdir+=/mnt/tmp436,lowerdir+=/mnt/tmp437,lowerdir+=/mnt/tmp438,
lowerdir+=/mnt/tmp439,lowerdir+=/mnt/tmp440,lowerdir+=/mnt/tmp441,
lowerdir+=/mnt/tmp442,lowerdir+=/mnt/tmp443,lowerdir+=/mnt/tmp444,
lowerdir+=/mnt/tmp445,lowerdir+=/mnt/tmp446,lowerdir+=/mnt/tmp447,
lowerdir+=/mnt/tmp448,lowerdir+=/mnt/tmp449,lowerdir+=/mnt/tmp450,
lowerdir+=/mnt/tmp451,lowerdir+=/mnt/tmp452,lowerdir+=/mnt/tmp453,
lowerdir+=/mnt/tmp454,lowerdir+=/mnt/tmp455,lowerdir+=/mnt/tmp456,
lowerdir+=/mnt/tmp457,lowerdir+=/mnt/tmp458,lowerdir+=/mnt/tmp459,
lowerdir+=/mnt/tmp460,lowerdir+=/mnt/tmp461,lowerdir+=/mnt/tmp462,
lowerdir+=/mnt/tmp463,lowerdir+=/mnt/tmp464,lowerdir+=/mnt/tmp465,
lowerdir+=/mnt/tmp466,lowerdir+=/mnt/tmp467,lowerdir+=/mnt/tmp468,
lowerdir+=/mnt/tmp469,lowerdir+=/mnt/tmp470,lowerdir+=/mnt/tmp471,
lowerdir+=/mnt/tmp472,lowerdir+=/mnt/tmp473,lowerdir+=/mnt/tmp474,
lowerdir+=/mnt/tmp475,lowerdir+=/mnt/tmp476,lowerdir+=/mnt/tmp477,
lowerdir+=/mnt/tmp478,lowerdir+=/mnt/tmp479,lowerdir+=/mnt/tmp480,
lowerdir+=/mnt/tmp481,lowerdir+=/mnt/tmp482,lowerdir+=/mnt/tmp483,
lowerdir+=/mnt/tmp484,lowerdir+=/mnt/tmp485,lowerdir+=/mnt/tmp486,
lowerdir+=/mnt/tmp487,lowerdir+=/mnt/tmp488,lowerdir+=/mnt/tmp489,
lowerdir+=/mnt/tmp490,lowerdir+=/mnt/tmp491,lowerdir+=/mnt/tmp492,
lowerdir+=/mnt/tmp493,lowerdir+=/mnt/tmp494,lowerdir+=/mnt/tmp495,
lowerdir+=/mnt/tmp496,lowerdir+=/mnt/tmp497,lowerdir+=/mnt/tmp498,
lowerdir+=/mnt/tmp499,lowerdir+=/mnt/tmp500,upperdir=/mnt/upper,workdir=/mnt/work uuid=on)

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Josef Bacik]

On Tue, Jun 25, 2024 at 12:42:14PM +0200, Christian Brauner wrote:
> On Mon, Jun 24, 2024 at 03:40:49PM GMT, Josef Bacik wrote:
> > Hello,
> > 
> > Currently if you want to get mount options for a mount and you're using
> > statmount(), you still have to open /proc/mounts to parse the mount options.
> > statmount() does have the ability to store an arbitrary string however,
> > additionally the way we do that is with a seq_file, which is also how we use
> > ->show_options for the individual file systems.
> > 
> > Extent statmount() to have a flag for fetching the mount options of a mount.
> > This allows users to not have to parse /proc mount for anything related to a
> > mount.  I've extended the existing statmount() test to validate this feature
> > works as expected.  As you can tell from the ridiculous amount of silly string
> > parsing, this is a huge win for users and climate change as we will no longer
> > have to waste several cycles parsing strings anymore.
> > 
> > This is based on my branch that extends listmount/statmount to walk into foreign
> > namespaces.  Below are links to that posting, that branch, and this branch to
> > make it easier to review.
> 
> So I was very hesitant to do it this way because I feel this is pretty
> ugly dumping mount options like that but Karel and others have the same
> use-case that they want to retrieve it all via statmount() (or another
> ID-based system call) so I guess I'll live with this. But note that this
> will be a fairly ugly interface at times. For example, mounting overlayfs with
> 500 lower layers then what one gets is:
> 

Yeah this isn't awesome, but neither is the string parsing code I have in the
selftest to validate this feature.

I chose this approach because 1) it's simple and I'm lazy, and 2) I think
anything else becomes a lot more complicated and more work than what people
actually want.

I imagine what would be ideal would be some sort of mount option iterator
mechanism.  Either we shoe-horn it into statmount() or we add a
listmountoptions() syscall, and we then get back a list of mount options
individually laid out.  This means we now have to keep track of where we are in
our mount option traversal, and change all the ->show_options callbacks to
handle this new iter thing.

We could go this way I suppose, but again this is a lot of work, and honestly I
just want to log mount options into some database so I can go looking for people
doing weird shit on my giant fleet of machines/containers.  Would the iter thing
make the overlayfs thing better?  Yeah for sure.  Do we care?  I don't think so,
we just want all the options, and we can all strsep/strtok with a comma.
Thanks,

Josef

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Miklos Szeredi]

On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote:

> We could go this way I suppose, but again this is a lot of work, and honestly I
> just want to log mount options into some database so I can go looking for people
> doing weird shit on my giant fleet of machines/containers.  Would the iter thing
> make the overlayfs thing better?  Yeah for sure.  Do we care?  I don't think so,
> we just want all the options, and we can all strsep/strtok with a comma.

I think we can live with the monolithic option block.  However I'd
prefer the separator to be a null character, thus the options could be
sent unescaped.  That way the iterator will be a lot simpler to
implement.

Thanks,
Miklos

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Christian Brauner]

On Tue, Jun 25, 2024 at 03:04:40PM GMT, Miklos Szeredi wrote:
> On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote:
> 
> > We could go this way I suppose, but again this is a lot of work, and honestly I
> > just want to log mount options into some database so I can go looking for people
> > doing weird shit on my giant fleet of machines/containers.  Would the iter thing
> > make the overlayfs thing better?  Yeah for sure.  Do we care?  I don't think so,
> > we just want all the options, and we can all strsep/strtok with a comma.
> 
> I think we can live with the monolithic option block.  However I'd
> prefer the separator to be a null character, thus the options could be
> sent unescaped.  That way the iterator will be a lot simpler to
> implement.

For libmount it means writing a new parser and Karel prefers the ","
format so I would like to keep the current format.

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Karel Zak]

On Tue, Jun 25, 2024 at 03:35:17PM GMT, Christian Brauner wrote:
> On Tue, Jun 25, 2024 at 03:04:40PM GMT, Miklos Szeredi wrote:
> > On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote:
> > 
> > > We could go this way I suppose, but again this is a lot of work, and honestly I
> > > just want to log mount options into some database so I can go looking for people
> > > doing weird shit on my giant fleet of machines/containers.  Would the iter thing
> > > make the overlayfs thing better?  Yeah for sure.  Do we care?  I don't think so,
> > > we just want all the options, and we can all strsep/strtok with a comma.
> > 
> > I think we can live with the monolithic option block.  However I'd
> > prefer the separator to be a null character, thus the options could be
> > sent unescaped.  That way the iterator will be a lot simpler to
> > implement.
> 
> For libmount it means writing a new parser and Karel prefers the ","
> format so I would like to keep the current format.
 
Sorry for the misunderstanding. I had a chat with Christian about it
when I was out of my office (and phone chats are not ideal for this).

I thought Miklos had suggested using a space (" ") as the separator, but
after reading the entire email thread, I now understand that Miklos'
suggestion is to use \0 (zero) as the options separator.

I have no issue with using \0, as it will make things much simpler.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Christian Brauner]

On Tue, Jun 25, 2024 at 03:52:03PM GMT, Karel Zak wrote:
> On Tue, Jun 25, 2024 at 03:35:17PM GMT, Christian Brauner wrote:
> > On Tue, Jun 25, 2024 at 03:04:40PM GMT, Miklos Szeredi wrote:
> > > On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote:
> > > 
> > > > We could go this way I suppose, but again this is a lot of work, and honestly I
> > > > just want to log mount options into some database so I can go looking for people
> > > > doing weird shit on my giant fleet of machines/containers.  Would the iter thing
> > > > make the overlayfs thing better?  Yeah for sure.  Do we care?  I don't think so,
> > > > we just want all the options, and we can all strsep/strtok with a comma.
> > > 
> > > I think we can live with the monolithic option block.  However I'd
> > > prefer the separator to be a null character, thus the options could be
> > > sent unescaped.  That way the iterator will be a lot simpler to
> > > implement.
> > 
> > For libmount it means writing a new parser and Karel prefers the ","
> > format so I would like to keep the current format.
>  
> Sorry for the misunderstanding. I had a chat with Christian about it
> when I was out of my office (and phone chats are not ideal for this).
> 
> I thought Miklos had suggested using a space (" ") as the separator, but
> after reading the entire email thread, I now understand that Miklos'
> suggestion is to use \0 (zero) as the options separator.
> 
> I have no issue with using \0, as it will make things much simpler.

Ah, I misread that myself. Fine by me.

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Josef Bacik]

On Tue, Jun 25, 2024 at 03:52:03PM +0200, Karel Zak wrote:
> On Tue, Jun 25, 2024 at 03:35:17PM GMT, Christian Brauner wrote:
> > On Tue, Jun 25, 2024 at 03:04:40PM GMT, Miklos Szeredi wrote:
> > > On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote:
> > > 
> > > > We could go this way I suppose, but again this is a lot of work, and honestly I
> > > > just want to log mount options into some database so I can go looking for people
> > > > doing weird shit on my giant fleet of machines/containers.  Would the iter thing
> > > > make the overlayfs thing better?  Yeah for sure.  Do we care?  I don't think so,
> > > > we just want all the options, and we can all strsep/strtok with a comma.
> > > 
> > > I think we can live with the monolithic option block.  However I'd
> > > prefer the separator to be a null character, thus the options could be
> > > sent unescaped.  That way the iterator will be a lot simpler to
> > > implement.
> > 
> > For libmount it means writing a new parser and Karel prefers the ","
> > format so I would like to keep the current format.
>  
> Sorry for the misunderstanding. I had a chat with Christian about it
> when I was out of my office (and phone chats are not ideal for this).
> 
> I thought Miklos had suggested using a space (" ") as the separator, but
> after reading the entire email thread, I now understand that Miklos'
> suggestion is to use \0 (zero) as the options separator.
> 
> I have no issue with using \0, as it will make things much simpler.

What I mean was "we can all strsep/strtok with a comma" I meant was in
userspace.  statmount() gives you the giant block, it's up to user space to
parse it.

I can change the kernel to do this for you, and then add a mnt_opts_len field so
you know how big of a block you get.

But that means getting the buffer, and going back through it and replacing every
',' with a '\0', because I'm sure as hell not going and changing all of our
->show_options() callbacks to not put in a ','.

Is this the direction we want to go?  Thanks,

Josef

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Karel Zak]

On Tue, Jun 25, 2024 at 10:17:56AM GMT, Josef Bacik wrote:
> On Tue, Jun 25, 2024 at 03:52:03PM +0200, Karel Zak wrote:
> > On Tue, Jun 25, 2024 at 03:35:17PM GMT, Christian Brauner wrote:
> > > On Tue, Jun 25, 2024 at 03:04:40PM GMT, Miklos Szeredi wrote:
> > > > On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote:
> > > > 
> > > > > We could go this way I suppose, but again this is a lot of work, and honestly I
> > > > > just want to log mount options into some database so I can go looking for people
> > > > > doing weird shit on my giant fleet of machines/containers.  Would the iter thing
> > > > > make the overlayfs thing better?  Yeah for sure.  Do we care?  I don't think so,
> > > > > we just want all the options, and we can all strsep/strtok with a comma.
> > > > 
> > > > I think we can live with the monolithic option block.  However I'd
> > > > prefer the separator to be a null character, thus the options could be
> > > > sent unescaped.  That way the iterator will be a lot simpler to
> > > > implement.
> > > 
> > > For libmount it means writing a new parser and Karel prefers the ","
> > > format so I would like to keep the current format.
> >  
> > Sorry for the misunderstanding. I had a chat with Christian about it
> > when I was out of my office (and phone chats are not ideal for this).
> > 
> > I thought Miklos had suggested using a space (" ") as the separator, but
> > after reading the entire email thread, I now understand that Miklos'
> > suggestion is to use \0 (zero) as the options separator.
> > 
> > I have no issue with using \0, as it will make things much simpler.
> 
> What I mean was "we can all strsep/strtok with a comma" I meant was in
> userspace.  statmount() gives you the giant block, it's up to user space to
> parse it.
> 
> I can change the kernel to do this for you, and then add a mnt_opts_len field so
> you know how big of a block you get.
> 
> But that means getting the buffer, and going back through it and replacing every
> ',' with a '\0', because I'm sure as hell not going and changing all of our
> ->show_options() callbacks to not put in a ','.
> 
> Is this the direction we want to go?  Thanks,

Honestly, I don't have a strong opinion on this. Both the ','
and \0 options work for me, and the userspace is already set up for
','. 

Using \0 is a possibility for creating an ideal kernel interface as it
doesn't require escaping, but it may require significant changes to the
kernel with minimal advantage for userspace. 

By the way, starmount() is so flexible that adding support for other
formats can be done anytime later with some new STATMOUNT_* mask.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Miklos Szeredi]

On Tue, 25 Jun 2024 at 16:18, Josef Bacik <josef@toxicpanda.com> wrote:

> But that means getting the buffer, and going back through it and replacing every
> ',' with a '\0', because I'm sure as hell not going and changing all of our
> ->show_options() callbacks to not put in a ','.
>
> Is this the direction we want to go?

IMO yes.  Having a clean interface is much more important than doing
slightly less processing on the kernel side (which would likely be
done anyway on the user side).

Thanks,
Miklos

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Miklos Szeredi]

On Wed, 26 Jun 2024 at 14:23, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 25 Jun 2024 at 16:18, Josef Bacik <josef@toxicpanda.com> wrote:
>
> > But that means getting the buffer, and going back through it and replacing every
> > ',' with a '\0', because I'm sure as hell not going and changing all of our
> > ->show_options() callbacks to not put in a ','.
> >
> > Is this the direction we want to go?
>
> IMO yes.  Having a clean interface is much more important than doing
> slightly less processing on the kernel side (which would likely be
> done anyway on the user side).

So I went for an extended leave, and this interface was merged in the
meantime with much to be desired.

The options are presented just the same as in /proc/self/mountinfo
(just the standard options left out).  And that has all the same
problems:

 - options can't contain commas (this causes much headache for
overlayfs which has filenames in its options)

 - to allow the result to be consumed by fsconfig() for example
options need to be unescaped

 - mnt_opts is confusing, since these are *not* mount options, these
are super block options.

This patchset was apparently hurried through without much thought and
review, and what review I did provide was ignored.  So I'm
frustrated, but not sure what if anything can be done at this point,
since the interface went live in the last release and changing it
would probably break things...

Thanks,
Miklos

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Christian Brauner]

On Mon, Nov 11, 2024 at 02:12:16PM +0100, Miklos Szeredi wrote:
> On Wed, 26 Jun 2024 at 14:23, Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, 25 Jun 2024 at 16:18, Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > > But that means getting the buffer, and going back through it and replacing every
> > > ',' with a '\0', because I'm sure as hell not going and changing all of our
> > > ->show_options() callbacks to not put in a ','.
> > >
> > > Is this the direction we want to go?
> >
> > IMO yes.  Having a clean interface is much more important than doing
> > slightly less processing on the kernel side (which would likely be
> > done anyway on the user side).
> 
> So I went for an extended leave, and this interface was merged in the
> meantime with much to be desired.
> 
> The options are presented just the same as in /proc/self/mountinfo
> (just the standard options left out).  And that has all the same
> problems:
> 
>  - options can't contain commas (this causes much headache for
> overlayfs which has filenames in its options)
> 
>  - to allow the result to be consumed by fsconfig() for example
> options need to be unescaped
> 
>  - mnt_opts is confusing, since these are *not* mount options, these
> are super block options.
> 
> This patchset was apparently hurried through without much thought and
> review, and what review I did provide was ignored.  So I'm
> frustrated, but not sure what if anything can be done at this point,
> since the interface went live in the last release and changing it
> would probably break things...

I understand your frustation but multiple people agreed that the
interface as is is fine and Karel as main consumer agreed as well. So
ultimately I didn't see a reason to delay the patchset.

None of the issues you raised are really things that make the interface
uncomsumable and Karel succeeded to port libmount to the new interfaces
with success (minus the mnt_devname we're adding now that he requested)
and was happy.

If there's genuine behavioral problems that cause substatntial issues
for userspace then I would request that you please add a new flag that
changes escaping and parsing behavior for statmount().

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Miklos Szeredi]

On Mon, 11 Nov 2024 at 14:29, Christian Brauner <brauner@kernel.org> wrote:

> I understand your frustation but multiple people agreed that the
> interface as is is fine and Karel as main consumer agreed as well. So
> ultimately I didn't see a reason to delay the patchset.

This was actually in the first versions that I sent out, but then was
removed per your request.  Then Josef's crufty version added back.
Yeah, for libmount it's fine, but the de-crufted version would've been
alright as well.  Oh, well...

> None of the issues you raised are really things that make the interface
> uncomsumable and Karel succeeded to port libmount to the new interfaces
> with success (minus the mnt_devname we're adding now that he requested)
> and was happy.

The problem is with non-libmount users.  They won't implement
unescaping until they run into trouble.  And that will be too late
because these cases are rare.

> If there's genuine behavioral problems that cause substatntial issues
> for userspace then I would request that you please add a new flag that
> changes escaping and parsing behavior for statmount().

Need to take a look at what this now does to overlayfs filenames with
commas and other special chars in them and see if it can be salvaged.

Thanks,
Miklos

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Josef Bacik]

On Mon, Nov 11, 2024 at 02:12:16PM +0100, Miklos Szeredi wrote:
> On Wed, 26 Jun 2024 at 14:23, Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, 25 Jun 2024 at 16:18, Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > > But that means getting the buffer, and going back through it and replacing every
> > > ',' with a '\0', because I'm sure as hell not going and changing all of our
> > > ->show_options() callbacks to not put in a ','.
> > >
> > > Is this the direction we want to go?
> >
> > IMO yes.  Having a clean interface is much more important than doing
> > slightly less processing on the kernel side (which would likely be
> > done anyway on the user side).
> 
> So I went for an extended leave, and this interface was merged in the
> meantime with much to be desired.
> 
> The options are presented just the same as in /proc/self/mountinfo
> (just the standard options left out).  And that has all the same
> problems:
> 
>  - options can't contain commas (this causes much headache for
> overlayfs which has filenames in its options)
> 
>  - to allow the result to be consumed by fsconfig() for example
> options need to be unescaped
> 
>  - mnt_opts is confusing, since these are *not* mount options, these
> are super block options.
> 
> This patchset was apparently hurried through without much thought and
> review, and what review I did provide was ignored.  So I'm
> frustrated, but not sure what if anything can be done at this point,
> since the interface went live in the last release and changing it
> would probably break things...

My apologies Miklos, I value your opinion and your feedback.  Sending my mind
back to when we were discussing this I think it just got lost in the other
patchsets I was working on, and then it got merged so it was "ok that's done,
next thing."  That's my bad, I'll be more careful in the future.  Thanks,

Josef

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Miklos Szeredi]

On Mon, 11 Nov 2024 at 16:28, Josef Bacik <josef@toxicpanda.com> wrote:

> My apologies Miklos, I value your opinion and your feedback.  Sending my mind
> back to when we were discussing this I think it just got lost in the other
> patchsets I was working on, and then it got merged so it was "ok that's done,
> next thing."  That's my bad, I'll be more careful in the future.  Thanks,

Thanks, Josef.  Sorry about getting a bit emotional, it wasn't totally
fair, given that I went offline for two months...

As Christian said, we can add a new flag for the un-escaped variant, if needed.

I'll make a patch, because I think it would be better if there was a
variant that non-libmount users could use without having to bother
with unescaping the options.  Maybe having two variants isn't such a
bad thing, as the current version is still useful for getting a single
printable string.

Thanks,
Miklos

Re: [PATCH 0/4] Add the ability to query mount options in statmount

[Karel Zak]

On Mon, Nov 11, 2024 at 05:02:10PM GMT, Miklos Szeredi wrote:
> As Christian said, we can add a new flag for the un-escaped variant, if needed.
> 
> I'll make a patch, because I think it would be better if there was a
> variant that non-libmount users could use without having to bother
> with unescaping the options.  Maybe having two variants isn't such a
> bad thing, as the current version is still useful for getting a single
> printable string.

I believe the ideal solution would be to support both variants. The
comma-separated variant is not a mistake, in my opinion. It is a very
common formatting style that has been used for decades.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: (subset) [PATCH 0/4] Add the ability to query mount options in statmount

[Christian Brauner]

On Mon, 24 Jun 2024 15:40:49 -0400, Josef Bacik wrote:
> Currently if you want to get mount options for a mount and you're using
> statmount(), you still have to open /proc/mounts to parse the mount options.
> statmount() does have the ability to store an arbitrary string however,
> additionally the way we do that is with a seq_file, which is also how we use
> ->show_options for the individual file systems.
> 
> Extent statmount() to have a flag for fetching the mount options of a mount.
> This allows users to not have to parse /proc mount for anything related to a
> mount.  I've extended the existing statmount() test to validate this feature
> works as expected.  As you can tell from the ridiculous amount of silly string
> parsing, this is a huge win for users and climate change as we will no longer
> have to waste several cycles parsing strings anymore.
> 
> [...]

* Changed to only call sb->d_op->show_options() so we only show filesystem
  mount options.
* Fixed/tweaked selftests to parse /proc/self/mountinfo directly and look for
  mount options, skipping over vfs generic superblock mount options.
* Since Karel is fine with keeping "," let's keep it.

---

Applied to the vfs.mount branch of the vfs/vfs.git tree.
Patches in the vfs.mount branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.mount

[1/4] fs: rename show_mnt_opts -> show_vfsmnt_opts
      https://git.kernel.org/vfs/vfs/c/429fc05aefd3
[3/4] fs: export mount options via statmount()
      https://git.kernel.org/vfs/vfs/c/f363afa8cbe0
[4/4] sefltests: extend the statmount test for mount options
      https://git.kernel.org/vfs/vfs/c/06bedc037f74