[PATCH v3 0/2] fs: allow statmount to fetch the subtype and devname

[PATCH v3 0/2] fs: allow statmount to fetch the subtype and devname

[Jeff Layton]

Meta has some internal logging that scrapes /proc/self/mountinfo today.
I'd like to convert it to use listmount()/statmount(), so we can do a
better job of monitoring with containers. We're missing some fields
though. This patchset adds them.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v3:
- Unescape the result of ->show_devname
- Move handling of nothing being emitted out of the switch statement
- Link to v2: https://lore.kernel.org/r/20241106-statmount-v2-0-93ba2aad38d1@kernel.org

Changes in v2:
- make statmount_fs_subtype
- return fast if no subtype is emitted
- new patch to allow statmount() to return devicename
- Link to v1: https://lore.kernel.org/r/20241106-statmount-v1-1-b93bafd97621@kernel.org

---
Jeff Layton (2):
      fs: add the ability for statmount() to report the fs_subtype
      fs: add the ability for statmount() to report the mnt_devname

 fs/namespace.c             | 68 ++++++++++++++++++++++++++++++++++++++++++----
 include/uapi/linux/mount.h |  6 +++-
 2 files changed, 67 insertions(+), 7 deletions(-)
---
base-commit: 26213e1a6caa5a7f508b919059b0122b451f4dfe
change-id: 20241106-statmount-3f91a7ed75fa

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v3 1/2] fs: add the ability for statmount() to report the fs_subtype

[Jeff Layton]

/proc/self/mountinfo prints out the sb->s_subtype after the type. This
is particularly useful for disambiguating FUSE mounts (at least when the
userland driver bothers to set it).

Add STATMOUNT_FS_SUBTYPE and claim one of the __spare2 fields to point
to the offset into the str[] array.

Handle the case where there is no subtype by not setting
STATMOUNT_FS_SUBTYPE in the returned mask. Check whether the function
emitted anything and just return immediately if not.

Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Ian Kent <raven@themaw.net>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/namespace.c             | 34 ++++++++++++++++++++++++++++------
 include/uapi/linux/mount.h |  5 ++++-
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index ba77ce1c6788dfe461814b5826fcbb3aab68fad4..fc4f81891d544305caf863904c0a6e16562fab49 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5006,6 +5006,14 @@ static int statmount_fs_type(struct kstatmount *s, struct seq_file *seq)
 	return 0;
 }
 
+static void statmount_fs_subtype(struct kstatmount *s, struct seq_file *seq)
+{
+	struct super_block *sb = s->mnt->mnt_sb;
+
+	if (sb->s_subtype)
+		seq_puts(seq, sb->s_subtype);
+}
+
 static void statmount_mnt_ns_id(struct kstatmount *s, struct mnt_namespace *ns)
 {
 	s->sm.mask |= STATMOUNT_MNT_NS_ID;
@@ -5042,33 +5050,44 @@ static int statmount_mnt_opts(struct kstatmount *s, struct seq_file *seq)
 
 static int statmount_string(struct kstatmount *s, u64 flag)
 {
-	int ret;
+	int ret = 0;
 	size_t kbufsize;
 	struct seq_file *seq = &s->seq;
 	struct statmount *sm = &s->sm;
+	u32 start = seq->count;
 
 	switch (flag) {
 	case STATMOUNT_FS_TYPE:
-		sm->fs_type = seq->count;
+		sm->fs_type = start;
 		ret = statmount_fs_type(s, seq);
 		break;
 	case STATMOUNT_MNT_ROOT:
-		sm->mnt_root = seq->count;
+		sm->mnt_root = start;
 		ret = statmount_mnt_root(s, seq);
 		break;
 	case STATMOUNT_MNT_POINT:
-		sm->mnt_point = seq->count;
+		sm->mnt_point = start;
 		ret = statmount_mnt_point(s, seq);
 		break;
 	case STATMOUNT_MNT_OPTS:
-		sm->mnt_opts = seq->count;
+		sm->mnt_opts = start;
 		ret = statmount_mnt_opts(s, seq);
 		break;
+	case STATMOUNT_FS_SUBTYPE:
+		sm->fs_subtype = start;
+		statmount_fs_subtype(s, seq);
+		break;
 	default:
 		WARN_ON_ONCE(true);
 		return -EINVAL;
 	}
 
+	/*
+	 * If nothing was emitted, return to avoid setting the flag
+	 * and terminating the buffer.
+	 */
+	if (seq->count == start)
+		return ret;
 	if (unlikely(check_add_overflow(sizeof(*sm), seq->count, &kbufsize)))
 		return -EOVERFLOW;
 	if (kbufsize >= s->bufsize)
@@ -5203,6 +5222,9 @@ static int do_statmount(struct kstatmount *s, u64 mnt_id, u64 mnt_ns_id,
 	if (!err && s->mask & STATMOUNT_MNT_OPTS)
 		err = statmount_string(s, STATMOUNT_MNT_OPTS);
 
+	if (!err && s->mask & STATMOUNT_FS_SUBTYPE)
+		err = statmount_string(s, STATMOUNT_FS_SUBTYPE);
+
 	if (!err && s->mask & STATMOUNT_MNT_NS_ID)
 		statmount_mnt_ns_id(s, ns);
 
@@ -5224,7 +5246,7 @@ static inline bool retry_statmount(const long ret, size_t *seq_size)
 }
 
 #define STATMOUNT_STRING_REQ (STATMOUNT_MNT_ROOT | STATMOUNT_MNT_POINT | \
-			      STATMOUNT_FS_TYPE | STATMOUNT_MNT_OPTS)
+			      STATMOUNT_FS_TYPE | STATMOUNT_MNT_OPTS | STATMOUNT_FS_SUBTYPE)
 
 static int prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
 			      struct statmount __user *buf, size_t bufsize,
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index 225bc366ffcbf0319929e2f55f1fbea88e4d7b81..2e939dddf9cbabe574dafdb6cff9ad4cf9298a74 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -173,7 +173,9 @@ struct statmount {
 	__u32 mnt_root;		/* [str] Root of mount relative to root of fs */
 	__u32 mnt_point;	/* [str] Mountpoint relative to current root */
 	__u64 mnt_ns_id;	/* ID of the mount namespace */
-	__u64 __spare2[49];
+	__u32 fs_subtype;	/* [str] Subtype of fs_type (if any) */
+	__u32 __spare1[1];
+	__u64 __spare2[48];
 	char str[];		/* Variable size part containing strings */
 };
 
@@ -207,6 +209,7 @@ struct mnt_id_req {
 #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 */
+#define STATMOUNT_FS_SUBTYPE		0x00000100U	/* Want/got fs_subtype */
 
 /*
  * Special @mnt_id values that can be passed to listmount

-- 
2.47.0

[PATCH v3 2/2] fs: add the ability for statmount() to report the mnt_devname

[Jeff Layton]

/proc/self/mountinfo displays the devicename for the mount, but
statmount() doesn't yet have a way to return it. Add a new
STATMOUNT_MNT_DEVNAME flag, claim the 32-bit __spare1 field to hold the
offset into the str[] array. STATMOUNT_MNT_DEVNAME will only be set in
the return mask if there is a device string.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/namespace.c             | 36 +++++++++++++++++++++++++++++++++++-
 include/uapi/linux/mount.h |  3 ++-
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index fc4f81891d544305caf863904c0a6e16562fab49..56750fcc890271e22b3b722dc0b4af445686bb86 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5014,6 +5014,32 @@ static void statmount_fs_subtype(struct kstatmount *s, struct seq_file *seq)
 		seq_puts(seq, sb->s_subtype);
 }
 
+static int statmount_mnt_devname(struct kstatmount *s, struct seq_file *seq)
+{
+	struct super_block *sb = s->mnt->mnt_sb;
+	struct mount *r = real_mount(s->mnt);
+
+	if (sb->s_op->show_devname) {
+		size_t start = seq->count;
+		int ret;
+
+		ret = sb->s_op->show_devname(seq, s->mnt->mnt_root);
+		if (ret)
+			return ret;
+
+		if (unlikely(seq_has_overflowed(seq)))
+			return -EAGAIN;
+
+		/* Unescape the result */
+		seq->buf[seq->count] = '\0';
+		seq->count = start;
+		seq_commit(seq, string_unescape_inplace(seq->buf + start, UNESCAPE_OCTAL));
+	} else if (r->mnt_devname) {
+		seq_puts(seq, r->mnt_devname);
+	}
+	return 0;
+}
+
 static void statmount_mnt_ns_id(struct kstatmount *s, struct mnt_namespace *ns)
 {
 	s->sm.mask |= STATMOUNT_MNT_NS_ID;
@@ -5077,6 +5103,10 @@ static int statmount_string(struct kstatmount *s, u64 flag)
 		sm->fs_subtype = start;
 		statmount_fs_subtype(s, seq);
 		break;
+	case STATMOUNT_MNT_DEVNAME:
+		sm->mnt_devname = seq->count;
+		ret = statmount_mnt_devname(s, seq);
+		break;
 	default:
 		WARN_ON_ONCE(true);
 		return -EINVAL;
@@ -5225,6 +5255,9 @@ static int do_statmount(struct kstatmount *s, u64 mnt_id, u64 mnt_ns_id,
 	if (!err && s->mask & STATMOUNT_FS_SUBTYPE)
 		err = statmount_string(s, STATMOUNT_FS_SUBTYPE);
 
+	if (!err && s->mask & STATMOUNT_MNT_DEVNAME)
+		err = statmount_string(s, STATMOUNT_MNT_DEVNAME);
+
 	if (!err && s->mask & STATMOUNT_MNT_NS_ID)
 		statmount_mnt_ns_id(s, ns);
 
@@ -5246,7 +5279,8 @@ static inline bool retry_statmount(const long ret, size_t *seq_size)
 }
 
 #define STATMOUNT_STRING_REQ (STATMOUNT_MNT_ROOT | STATMOUNT_MNT_POINT | \
-			      STATMOUNT_FS_TYPE | STATMOUNT_MNT_OPTS | STATMOUNT_FS_SUBTYPE)
+			      STATMOUNT_FS_TYPE | STATMOUNT_MNT_OPTS | \
+			      STATMOUNT_FS_SUBTYPE | STATMOUNT_MNT_DEVNAME)
 
 static int prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
 			      struct statmount __user *buf, size_t bufsize,
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index 2e939dddf9cbabe574dafdb6cff9ad4cf9298a74..3de1b0231b639fb8ed739d65b5b5406021f74196 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -174,7 +174,7 @@ struct statmount {
 	__u32 mnt_point;	/* [str] Mountpoint relative to current root */
 	__u64 mnt_ns_id;	/* ID of the mount namespace */
 	__u32 fs_subtype;	/* [str] Subtype of fs_type (if any) */
-	__u32 __spare1[1];
+	__u32 mnt_devname;	/* [str] Device string for the mount */
 	__u64 __spare2[48];
 	char str[];		/* Variable size part containing strings */
 };
@@ -210,6 +210,7 @@ struct mnt_id_req {
 #define STATMOUNT_MNT_NS_ID		0x00000040U	/* Want/got mnt_ns_id */
 #define STATMOUNT_MNT_OPTS		0x00000080U	/* Want/got mnt_opts */
 #define STATMOUNT_FS_SUBTYPE		0x00000100U	/* Want/got fs_subtype */
+#define STATMOUNT_MNT_DEVNAME		0x00000200U	/* Want/got mnt_devname */
 
 /*
  * Special @mnt_id values that can be passed to listmount

-- 
2.47.0

Re: [PATCH v3 2/2] fs: add the ability for statmount() to report the mnt_devname

[Jan Kara]

On Thu 07-11-24 16:00:07, Jeff Layton wrote:
> /proc/self/mountinfo displays the devicename for the mount, but
> statmount() doesn't yet have a way to return it. Add a new
> STATMOUNT_MNT_DEVNAME flag, claim the 32-bit __spare1 field to hold the
> offset into the str[] array. STATMOUNT_MNT_DEVNAME will only be set in
> the return mask if there is a device string.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/namespace.c             | 36 +++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/mount.h |  3 ++-
>  2 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index fc4f81891d544305caf863904c0a6e16562fab49..56750fcc890271e22b3b722dc0b4af445686bb86 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -5014,6 +5014,32 @@ static void statmount_fs_subtype(struct kstatmount *s, struct seq_file *seq)
>  		seq_puts(seq, sb->s_subtype);
>  }
>  
> +static int statmount_mnt_devname(struct kstatmount *s, struct seq_file *seq)
> +{
> +	struct super_block *sb = s->mnt->mnt_sb;
> +	struct mount *r = real_mount(s->mnt);
> +
> +	if (sb->s_op->show_devname) {
> +		size_t start = seq->count;
> +		int ret;
> +
> +		ret = sb->s_op->show_devname(seq, s->mnt->mnt_root);
> +		if (ret)
> +			return ret;
> +
> +		if (unlikely(seq_has_overflowed(seq)))
> +			return -EAGAIN;
> +
> +		/* Unescape the result */
> +		seq->buf[seq->count] = '\0';
> +		seq->count = start;
> +		seq_commit(seq, string_unescape_inplace(seq->buf + start, UNESCAPE_OCTAL));
> +	} else if (r->mnt_devname) {
> +		seq_puts(seq, r->mnt_devname);
> +	}
> +	return 0;
> +}
> +
>  static void statmount_mnt_ns_id(struct kstatmount *s, struct mnt_namespace *ns)
>  {
>  	s->sm.mask |= STATMOUNT_MNT_NS_ID;
> @@ -5077,6 +5103,10 @@ static int statmount_string(struct kstatmount *s, u64 flag)
>  		sm->fs_subtype = start;
>  		statmount_fs_subtype(s, seq);
>  		break;
> +	case STATMOUNT_MNT_DEVNAME:
> +		sm->mnt_devname = seq->count;
> +		ret = statmount_mnt_devname(s, seq);
> +		break;
>  	default:
>  		WARN_ON_ONCE(true);
>  		return -EINVAL;
> @@ -5225,6 +5255,9 @@ static int do_statmount(struct kstatmount *s, u64 mnt_id, u64 mnt_ns_id,
>  	if (!err && s->mask & STATMOUNT_FS_SUBTYPE)
>  		err = statmount_string(s, STATMOUNT_FS_SUBTYPE);
>  
> +	if (!err && s->mask & STATMOUNT_MNT_DEVNAME)
> +		err = statmount_string(s, STATMOUNT_MNT_DEVNAME);
> +
>  	if (!err && s->mask & STATMOUNT_MNT_NS_ID)
>  		statmount_mnt_ns_id(s, ns);
>  
> @@ -5246,7 +5279,8 @@ static inline bool retry_statmount(const long ret, size_t *seq_size)
>  }
>  
>  #define STATMOUNT_STRING_REQ (STATMOUNT_MNT_ROOT | STATMOUNT_MNT_POINT | \
> -			      STATMOUNT_FS_TYPE | STATMOUNT_MNT_OPTS | STATMOUNT_FS_SUBTYPE)
> +			      STATMOUNT_FS_TYPE | STATMOUNT_MNT_OPTS | \
> +			      STATMOUNT_FS_SUBTYPE | STATMOUNT_MNT_DEVNAME)
>  
>  static int prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
>  			      struct statmount __user *buf, size_t bufsize,
> diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
> index 2e939dddf9cbabe574dafdb6cff9ad4cf9298a74..3de1b0231b639fb8ed739d65b5b5406021f74196 100644
> --- a/include/uapi/linux/mount.h
> +++ b/include/uapi/linux/mount.h
> @@ -174,7 +174,7 @@ struct statmount {
>  	__u32 mnt_point;	/* [str] Mountpoint relative to current root */
>  	__u64 mnt_ns_id;	/* ID of the mount namespace */
>  	__u32 fs_subtype;	/* [str] Subtype of fs_type (if any) */
> -	__u32 __spare1[1];
> +	__u32 mnt_devname;	/* [str] Device string for the mount */
>  	__u64 __spare2[48];
>  	char str[];		/* Variable size part containing strings */
>  };
> @@ -210,6 +210,7 @@ struct mnt_id_req {
>  #define STATMOUNT_MNT_NS_ID		0x00000040U	/* Want/got mnt_ns_id */
>  #define STATMOUNT_MNT_OPTS		0x00000080U	/* Want/got mnt_opts */
>  #define STATMOUNT_FS_SUBTYPE		0x00000100U	/* Want/got fs_subtype */
> +#define STATMOUNT_MNT_DEVNAME		0x00000200U	/* Want/got mnt_devname */
>  
>  /*
>   * Special @mnt_id values that can be passed to listmount
> 
> -- 
> 2.47.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 0/2] fs: allow statmount to fetch the subtype and devname

[Christian Brauner]

On Thu, 07 Nov 2024 16:00:05 -0500, Jeff Layton wrote:
> Meta has some internal logging that scrapes /proc/self/mountinfo today.
> I'd like to convert it to use listmount()/statmount(), so we can do a
> better job of monitoring with containers. We're missing some fields
> though. This patchset adds them.
> 
> 

I know Karel has been wanting this for libmount as well. Thanks for
doing this! It would be nice if you could also add some selftests!

---

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc 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.misc

[1/2] fs: add the ability for statmount() to report the fs_subtype
      https://git.kernel.org/vfs/vfs/c/ddfdeccd46bd
[2/2] fs: add the ability for statmount() to report the mnt_devname
      https://git.kernel.org/vfs/vfs/c/6fb42b3c00cd

Re: [PATCH v3 1/2] fs: add the ability for statmount() to report the fs_subtype

[Miklos Szeredi]

On Thu, 7 Nov 2024 at 22:00, Jeff Layton <jlayton@kernel.org> wrote:

> +       /*
> +        * If nothing was emitted, return to avoid setting the flag
> +        * and terminating the buffer.
> +        */
> +       if (seq->count == start)
> +               return ret;

First of all, I don't think it's okay to subtly change behavior of
other string attributes in this patch.   If that is what we want, it
should be separated into a separate prep or followup patch.

Clearing the returned mask if there's no subtype does sound like the
right thing to do.  But it makes it impossible to detect whether the
running kernel supports returning subtype or not.  Missing the
STATMOUNT_FS_SUBTYPE in statmount.mask may mean two different things:

 - kernel supports returning subtype and filesystem does not have a subtype

 - kernel does not support returning a subtype and the filesystem may
or may not have a subtype

I think we can live with  that, but it would be really good if there
was a universal way to detect whether a particular feature is
supported on the running kernel or not, and not have to rely on
syscall specific ways.

Thanks,
Miklos

Re: [PATCH v3 1/2] fs: add the ability for statmount() to report the fs_subtype

[Jeff Layton]

On Mon, 2024-11-11 at 11:49 +0100, Miklos Szeredi wrote:
> On Thu, 7 Nov 2024 at 22:00, Jeff Layton <jlayton@kernel.org> wrote:
> 
> > +       /*
> > +        * If nothing was emitted, return to avoid setting the flag
> > +        * and terminating the buffer.
> > +        */
> > +       if (seq->count == start)
> > +               return ret;
> 
> First of all, I don't think it's okay to subtly change behavior of
> other string attributes in this patch.   If that is what we want, it
> should be separated into a separate prep or followup patch.
> 

As far as I can tell, the existing cases in statmount_string() either
always emit a string or an error code. If a string isn't emitted, then
the two EOVERFLOW cases and the EAGAIN case can't happen, so I don't
think this will result in any change in behavior for the existing code.


> Clearing the returned mask if there's no subtype does sound like the
> right thing to do.  But it makes it impossible to detect whether the
> running kernel supports returning subtype or not.  Missing the
> STATMOUNT_FS_SUBTYPE in statmount.mask may mean two different things:
> 
>  - kernel supports returning subtype and filesystem does not have a subtype
> 
>  - kernel does not support returning a subtype and the filesystem may
> or may not have a subtype
> 
> I think we can live with  that, but it would be really good if there
> was a universal way to detect whether a particular feature is
> supported on the running kernel or not, and not have to rely on
> syscall specific ways.

The idea for statmount() was to add a statx() like interface. This is
exactly how statx() works, so I don't think it ought to be any sort of
surprise to anyone.

That said, if we did want to add a way to detect what flags are
actually supported, we could just add a new STATMOUNT_SUPPORTED_FLAGS
flag and add a field that holds a returned mask with all of the bits
set that the kernel supports.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 1/2] fs: add the ability for statmount() to report the fs_subtype

[Miklos Szeredi]

On Mon, 11 Nov 2024 at 12:28, Jeff Layton <jlayton@kernel.org> wrote:

> As far as I can tell, the existing cases in statmount_string() either
> always emit a string or an error code. If a string isn't emitted, then
> the two EOVERFLOW cases and the EAGAIN case can't happen, so I don't
> think this will result in any change in behavior for the existing code.

Both mnt_point and mnt_opts can be empty.

> The idea for statmount() was to add a statx() like interface. This is
> exactly how statx() works, so I don't think it ought to be any sort of
> surprise to anyone.

Maybe, but silently changing the interface is not okay.  At least make
it a separate patch.

> That said, if we did want to add a way to detect what flags are
> actually supported, we could just add a new STATMOUNT_SUPPORTED_FLAGS
> flag and add a field that holds a returned mask with all of the bits
> set that the kernel supports.

Yeah, that makes sense.

Thanks,
Miklos

Re: [PATCH v3 1/2] fs: add the ability for statmount() to report the fs_subtype

[Jeff Layton]

On Mon, 2024-11-11 at 14:01 +0100, Miklos Szeredi wrote:
> On Mon, 11 Nov 2024 at 12:28, Jeff Layton <jlayton@kernel.org> wrote:
> 
> > As far as I can tell, the existing cases in statmount_string() either
> > always emit a string or an error code. If a string isn't emitted, then
> > the two EOVERFLOW cases and the EAGAIN case can't happen, so I don't
> > think this will result in any change in behavior for the existing code.
> 
> Both mnt_point and mnt_opts can be empty.
> 

Ok, so currently if they are, the flag gets set and there is no
string?  If so, then you're correct and this is a behavior change. The
question is -- is it a desirable one? The interface is new enough that
I think we have the luxury of changing this now (and establishing a
future standard).

Personally, I think that's how it ought to work. When there is no
string present, we ought not set the flag in the return mask. Does
anyone prefer it the other way?

> > The idea for statmount() was to add a statx() like interface. This is
> > exactly how statx() works, so I don't think it ought to be any sort of
> > surprise to anyone.
> 
> Maybe, but silently changing the interface is not okay.  At least make
> it a separate patch.
> 

Ok, Christian has already taken this in, but I can respin it and 
split that bit out into a separate patch. Or, I could just revise the
changelog to note the behavior change.

Either way, we'll need to decide about how empty fields should be
handled first.

> > That said, if we did want to add a way to detect what flags are
> > actually supported, we could just add a new STATMOUNT_SUPPORTED_FLAGS
> > flag and add a field that holds a returned mask with all of the bits
> > set that the kernel supports.
> 
> Yeah, that makes sense.
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 1/2] fs: add the ability for statmount() to report the fs_subtype

[Christian Brauner]

On Mon, Nov 11, 2024 at 08:28:07AM -0500, Jeff Layton wrote:
> On Mon, 2024-11-11 at 14:01 +0100, Miklos Szeredi wrote:
> > On Mon, 11 Nov 2024 at 12:28, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > > As far as I can tell, the existing cases in statmount_string() either
> > > always emit a string or an error code. If a string isn't emitted, then
> > > the two EOVERFLOW cases and the EAGAIN case can't happen, so I don't
> > > think this will result in any change in behavior for the existing code.
> > 
> > Both mnt_point and mnt_opts can be empty.
> > 
> 
> Ok, so currently if they are, the flag gets set and there is no
> string?  If so, then you're correct and this is a behavior change. The
> question is -- is it a desirable one? The interface is new enough that
> I think we have the luxury of changing this now (and establishing a
> future standard).
> 
> Personally, I think that's how it ought to work. When there is no
> string present, we ought not set the flag in the return mask. Does
> anyone prefer it the other way?

It's a change we can certainly do. I see no reason not to try it.

Re: [PATCH v3 0/2] fs: allow statmount to fetch the subtype and devname

[Jeff Layton]

On Mon, 2024-11-11 at 10:17 +0100, Christian Brauner wrote:
> On Thu, 07 Nov 2024 16:00:05 -0500, Jeff Layton wrote:
> > Meta has some internal logging that scrapes /proc/self/mountinfo today.
> > I'd like to convert it to use listmount()/statmount(), so we can do a
> > better job of monitoring with containers. We're missing some fields
> > though. This patchset adds them.
> > 
> > 
> 
> I know Karel has been wanting this for libmount as well. Thanks for
> doing this! It would be nice if you could also add some selftests!
> 

(cc'ing Karel)

Thanks. We may need to tweak this a bit, based on Miklos' comments
about how empty strings are handled now, but it shouldn't be too big a
change.

I actually have a related question about libmount: glibc doesn't
currently provide syscall wrappers for statmount() and listmount().
Would it make sense to have libmount provide those? We could copy the
wrappers in tools/testing/selftests/filesystems/statmount/statmount.h
to libmount.h.

It's error-prone and a pain to roll these yourself, and that would make
things simpler until someone is ready to do something for glibc.

Another idea might be to start a new userland header file that is just
a collection of static inline wrappers for syscalls that aren't
packaged in glibc.e.g.  pidfd_open also doesn't have glibc bindings, so
we could add that there too.

> ---
> 
> Applied to the vfs.misc branch of the vfs/vfs.git tree.
> Patches in the vfs.misc 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.misc
> 
> [1/2] fs: add the ability for statmount() to report the fs_subtype
>       https://git.kernel.org/vfs/vfs/c/ddfdeccd46bd
> [2/2] fs: add the ability for statmount() to report the mnt_devname
>       https://git.kernel.org/vfs/vfs/c/6fb42b3c00cd

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 2/2] fs: add the ability for statmount() to report the mnt_devname

[Miklos Szeredi]

On Thu, 7 Nov 2024 at 22:00, Jeff Layton <jlayton@kernel.org> wrote:

> diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
> index 2e939dddf9cbabe574dafdb6cff9ad4cf9298a74..3de1b0231b639fb8ed739d65b5b5406021f74196 100644
> --- a/include/uapi/linux/mount.h
> +++ b/include/uapi/linux/mount.h
> @@ -174,7 +174,7 @@ struct statmount {
>         __u32 mnt_point;        /* [str] Mountpoint relative to current root */
>         __u64 mnt_ns_id;        /* ID of the mount namespace */
>         __u32 fs_subtype;       /* [str] Subtype of fs_type (if any) */
> -       __u32 __spare1[1];
> +       __u32 mnt_devname;      /* [str] Device string for the mount */

One more point:  this is called source in both the old mount(2) API
and in new the fsconfig(2) API, where it's handled just like a plain
option (i.e. "-osource=/dev/foo").

Also this is a sb property, not a mount property, so the naming is confusing.

So I'd call this "sb_source" for consistency.

Thanks,
Miklos

Re: [PATCH v3 0/2] fs: allow statmount to fetch the subtype and devname

[Karel Zak]

On Mon, Nov 11, 2024 at 08:42:26AM GMT, Jeff Layton wrote:
> On Mon, 2024-11-11 at 10:17 +0100, Christian Brauner wrote:
> > On Thu, 07 Nov 2024 16:00:05 -0500, Jeff Layton wrote:
> > > Meta has some internal logging that scrapes /proc/self/mountinfo today.
> > > I'd like to convert it to use listmount()/statmount(), so we can do a
> > > better job of monitoring with containers. We're missing some fields
> > > though. This patchset adds them.
> > > 
> > > 
> > 
> > I know Karel has been wanting this for libmount as well. Thanks for
> > doing this! It would be nice if you could also add some selftests!
> > 
> 
> (cc'ing Karel)
> 
> Thanks. We may need to tweak this a bit, based on Miklos' comments
> about how empty strings are handled now, but it shouldn't be too big a
> change.
> 
> I actually have a related question about libmount: glibc doesn't
> currently provide syscall wrappers for statmount() and listmount().
> Would it make sense to have libmount provide those? We could copy the
> wrappers in tools/testing/selftests/filesystems/statmount/statmount.h
> to libmount.h.

I'm not sure if libmount is the right place. The library is complex
and large, and installing it just to get some simple wrappers seems
like overkill.

I believe the issue of "syscall wrappers" should be addressed in a
more generic manner, as the situation of wanting to try or use new
kernel features is quite common. This approach would also simplify
kernel testing.

> Another idea might be to start a new userland header file that is just
> a collection of static inline wrappers for syscalls that aren't
> packaged in glibc.e.g.  pidfd_open also doesn't have glibc bindings, so
> we could add that there too.

Yes, this seems like proper and portable solution. It would be great
if kernel itself will introduce any convention for this.

It's more syscalls without wrappers, sched_setattr, cachestat, kcmp, 
all pidfd_*, etc.

I'm not sure whether to create one collection of all syscalls or add
the wrappers to the appropriate locations (e.g. uapi/linux/mount.h
or introduce uapi/linux/mount_calls.h). The syscalls have dependencies
on specific structs such as 'struct statmount', etc.

    Karel

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

Re: [PATCH v3 0/2] fs: allow statmount to fetch the subtype and devname

[Christian Brauner]

On Mon, Nov 11, 2024 at 08:42:26AM -0500, Jeff Layton wrote:
> On Mon, 2024-11-11 at 10:17 +0100, Christian Brauner wrote:
> > On Thu, 07 Nov 2024 16:00:05 -0500, Jeff Layton wrote:
> > > Meta has some internal logging that scrapes /proc/self/mountinfo today.
> > > I'd like to convert it to use listmount()/statmount(), so we can do a
> > > better job of monitoring with containers. We're missing some fields
> > > though. This patchset adds them.
> > > 
> > > 
> > 
> > I know Karel has been wanting this for libmount as well. Thanks for
> > doing this! It would be nice if you could also add some selftests!
> > 
> 
> (cc'ing Karel)
> 
> Thanks. We may need to tweak this a bit, based on Miklos' comments
> about how empty strings are handled now, but it shouldn't be too big a
> change.
> 
> I actually have a related question about libmount: glibc doesn't
> currently provide syscall wrappers for statmount() and listmount().

I think it'll be a bit until glibc exposes those system calls because I
think they are special-purpose in a lot of ways. But also because glibc
usually takes a while to add new system call wrappers.

> Would it make sense to have libmount provide those? We could copy the

I think libmount may not necessarily provide direct syscall wrappers but
will expose new api functionality. This is at least what I gather from
all the discussions on util-linux.

> wrappers in tools/testing/selftests/filesystems/statmount/statmount.h
> to libmount.h.
> 
> It's error-prone and a pain to roll these yourself, and that would make

As with most system calls.

> things simpler until someone is ready to do something for glibc.
> 
> Another idea might be to start a new userland header file that is just
> a collection of static inline wrappers for syscalls that aren't
> packaged in glibc.e.g.  pidfd_open also doesn't have glibc bindings, so
> we could add that there too.

Oh? What glibc version are you on? pidfd_open() et al should all have
glibc wrappers afaik. It just always takes a while:

        > cat /usr/include/x86_64-linux-gnu/sys/pidfd.h | grep pidfd
        extern int pidfd_open (__pid_t __pid, unsigned int __flags) __THROW;
        extern int pidfd_getfd (int __pidfd, int __targetfd,
        extern int pidfd_send_signal (int __pidfd, int __sig, siginfo_t *__info,
        extern pid_t pidfd_getpid (int __fd) __THROW;

Re: [PATCH v3 0/2] fs: allow statmount to fetch the subtype and devname

[Miklos Szeredi]

On Tue, 12 Nov 2024 at 10:42, Christian Brauner <brauner@kernel.org> wrote:
>
> On Mon, Nov 11, 2024 at 08:42:26AM -0500, Jeff Layton wrote:

> > It's error-prone and a pain to roll these yourself, and that would make
>
> As with most system calls.

Also couldn't the kernel tree have a man2 directory, where all the
syscall man pages could be maintained?   I think it would very much
make sense to update the man page together with the kernel API change.

Thanks,
Miklos

Re: [PATCH v3 0/2] fs: allow statmount to fetch the subtype and devname

[Jeff Layton]

On Tue, 2024-11-12 at 10:42 +0100, Christian Brauner wrote:
> On Mon, Nov 11, 2024 at 08:42:26AM -0500, Jeff Layton wrote:
> > On Mon, 2024-11-11 at 10:17 +0100, Christian Brauner wrote:
> > > On Thu, 07 Nov 2024 16:00:05 -0500, Jeff Layton wrote:
> > > > Meta has some internal logging that scrapes /proc/self/mountinfo today.
> > > > I'd like to convert it to use listmount()/statmount(), so we can do a
> > > > better job of monitoring with containers. We're missing some fields
> > > > though. This patchset adds them.
> > > > 
> > > > 
> > > 
> > > I know Karel has been wanting this for libmount as well. Thanks for
> > > doing this! It would be nice if you could also add some selftests!
> > > 
> > 
> > (cc'ing Karel)
> > 
> > Thanks. We may need to tweak this a bit, based on Miklos' comments
> > about how empty strings are handled now, but it shouldn't be too big a
> > change.
> > 
> > I actually have a related question about libmount: glibc doesn't
> > currently provide syscall wrappers for statmount() and listmount().
> 
> I think it'll be a bit until glibc exposes those system calls because I
> think they are special-purpose in a lot of ways. But also because glibc
> usually takes a while to add new system call wrappers.
> 
> > Would it make sense to have libmount provide those? We could copy the
> 
> I think libmount may not necessarily provide direct syscall wrappers but
> will expose new api functionality. This is at least what I gather from
> all the discussions on util-linux.
> 
> > wrappers in tools/testing/selftests/filesystems/statmount/statmount.h
> > to libmount.h.
> > 
> > It's error-prone and a pain to roll these yourself, and that would make
> 
> As with most system calls.
> 
> > things simpler until someone is ready to do something for glibc.
> > 
> > Another idea might be to start a new userland header file that is just
> > a collection of static inline wrappers for syscalls that aren't
> > packaged in glibc.e.g.  pidfd_open also doesn't have glibc bindings, so
> > we could add that there too.
> 
> Oh? What glibc version are you on? pidfd_open() et al should all have
> glibc wrappers afaik. It just always takes a while:
> 
>         > cat /usr/include/x86_64-linux-gnu/sys/pidfd.h | grep pidfd
>         extern int pidfd_open (__pid_t __pid, unsigned int __flags) __THROW;
>         extern int pidfd_getfd (int __pidfd, int __targetfd,
>         extern int pidfd_send_signal (int __pidfd, int __sig, siginfo_t *__info,
>         extern pid_t pidfd_getpid (int __fd) __THROW;

Ahh, I was trusting the manpage, which says:

       Note: glibc provides no wrapper for pidfd_open(), necessitating
the use of syscall(2).

It looks like recent glibc does have wrappers for this.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 0/2] fs: allow statmount to fetch the subtype and devname

[Christian Brauner]

On Tue, Nov 12, 2024 at 11:24:45AM +0100, Miklos Szeredi wrote:
> On Tue, 12 Nov 2024 at 10:42, Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Mon, Nov 11, 2024 at 08:42:26AM -0500, Jeff Layton wrote:
> 
> > > It's error-prone and a pain to roll these yourself, and that would make
> >
> > As with most system calls.
> 
> Also couldn't the kernel tree have a man2 directory, where all the
> syscall man pages could be maintained?   I think it would very much
> make sense to update the man page together with the kernel API change.

I keep saying that over and over as well. IMHO, we should integrate the
system call manpages into the kernel tree. So fully agreed.