Re: [PATCH V3 4/4] 9p: convert to the new mount API

[Remi Pommarel <repk@triplefau.lt>]

Hi Eric, Dominique,

On Mon, Oct 13, 2025 at 07:26:35PM +0900, Dominique Martinet wrote:
> Hi Eric,
> 
> Thanks for this V3!
> 
> I find it much cleaner, hopefully will be easier to debug :)
> ... Which turned out to be needed right away, trying with qemu's 9p
> export "mount -t 9p -o trans=virtio tmp /mnt" apparently calls
> p9_virtio_create() with fc->source == NULL, instead of the expected
> "tmp" string
> (FWIW I tried '-o trans=tcp 127.0.0.1' and I got the same problem in
> p9_fd_create_tcp(), might be easier to test with diod if that's what you
> used)
> 
> Looking at other filesystems (e.g. fs/nfs/fs_context.c but others are
> the same) it looks like they all define a fsparam_string "source" option
> explicitly?...
> 
> Something like this looks like it works to do (+ probably make the error
> more verbose? nothing in dmesg hints at why mount returns EINVAL...)
> -----
> diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c
> index 6c07635f5776..999d54a0c7d9 100644
> --- a/fs/9p/v9fs.c
> +++ b/fs/9p/v9fs.c
> @@ -34,6 +34,8 @@ struct kmem_cache *v9fs_inode_cache;
>   */
>  
>  enum {
> +	/* Mount-point source */
> +	Opt_source,
>  	/* Options that take integer arguments */
>  	Opt_debug, Opt_dfltuid, Opt_dfltgid, Opt_afid,
>  	/* String options */
> @@ -82,6 +84,7 @@ static const struct constant_table p9_cache_mode[] = {
>   * the client, and all the transports.
>   */
>  const struct fs_parameter_spec v9fs_param_spec[] = {
> +	fsparam_string  ("source",      Opt_source),
>  	fsparam_u32hex	("debug",	Opt_debug),
>  	fsparam_uid	("dfltuid",	Opt_dfltuid),
>  	fsparam_gid	("dfltgid",	Opt_dfltgid),
> @@ -210,6 +213,14 @@ int v9fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  	}
>  
>  	switch (opt) {
> +	case Opt_source:
> +                if (fc->source) {
> +			pr_info("p9: multiple sources not supported\n");
> +			return -EINVAL;
> +		}
> +		fc->source = param->string;
> +		param->string = NULL;
> +		break;
>  	case Opt_debug:
>  		session_opts->debug = result.uint_32;
>  #ifdef CONFIG_NET_9P_DEBUG
> -----

While testing this series to mount a QEMU's 9p directory with
trans=virtio, I encountered a few issues. The same fix as above was
necessary, but further regressions were also observed.

Previously, using msize=2048k would silently fail to parse the option,
but the mount would still proceed. With this series, the parsing error
now prevents the mount entirely. While I prefer the new behavior, I know
there is a strict rule to not break userspace, so are we not breaking
userspace here?

Another more important issue is that I was not able to successfully
mount a 9p as rootfs with the command line below:
 'root=/dev/root rw rootfstype=9p rootflags=trans=virtio,cache=loose'

The issue arises because init systems typically remount root as
read-only (mount -oremount,ro /). This process retrieves the current
mount options via v9fs_show_options(), then attempts to remount with
those options plus ro. However, v9fs_show_options() formats the cache
option as an integer but v9fs_parse_param() expect cache option to be
a string (fsparam_enum) causing remount to fail. The patch below fix the
issue for the cache option, but pretty sure all fsparam_enum options
should be fixed.

----
diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c
index a58abe69ec7a..e4e8304e5934 100644
--- a/fs/9p/v9fs.c
+++ b/fs/9p/v9fs.c
@@ -120,6 +120,21 @@ const struct fs_parameter_spec v9fs_param_spec[] = {
 	{}
 };
 
+static char const *p9_cache_to_str(enum p9_cache_shortcuts sc)
+{
+	char const *cache = "none";
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(p9_cache_mode); ++i) {
+		if (p9_cache_mode[i].value == sc) {
+			cache = p9_cache_mode[i].name;
+			break;
+		}
+	}
+
+	return cache;
+}
+
 /*
  * Display the mount options in /proc/mounts.
  */
@@ -144,7 +159,7 @@ int v9fs_show_options(struct seq_file *m, struct dentry *root)
 	if (v9ses->nodev)
 		seq_puts(m, ",nodevmap");
 	if (v9ses->cache)
-		seq_printf(m, ",cache=%x", v9ses->cache);
+		seq_printf(m, ",cache=%s", p9_cache_to_str(v9ses->cache));
 #ifdef CONFIG_9P_FSCACHE
 	if (v9ses->cachetag && (v9ses->cache & CACHE_FSCACHE))
 		seq_printf(m, ",cachetag=%s", v9ses->cachetag);
----

However same question as above arise with this patch. Previously cat
/proc/mounts would format cache as an hexadecimal value while now it is
the enum value name string. Would this be considered userspace
breakage?

Thanks,

-- 
Remi