Message-ID: <b7b203c4-6e4b-4eeb-a23e-e6314342f288@redhat.com>
Date: Mon, 1 Dec 2025 16:36:58 -0600
From: Eric Sandeen <sandeen@...hat.com>
To: Dominique Martinet <asmadeus@...ewreck.org>,
 Remi Pommarel <repk@...plefau.lt>
Cc: v9fs@...ts.linux.dev, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, ericvh@...nel.org, lucho@...kov.net,
 linux_oss@...debyte.com, eadavis@...com
Subject: Re: [PATCH V3 4/4] 9p: convert to the new mount API

On 11/26/25 4:43 PM, Dominique Martinet wrote:
> Hi Remi,
> 
> Remi Pommarel wrote on Wed, Nov 26, 2025 at 09:16:14PM +0100:
>> 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.
> 
> Thanks for testing!
> (FWIW that patch has been rolled into my 9p-next branch, so you shouldn't
> have needed to fiddle with it if using linux-next)
> 
>> 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?
> 
> That's a good question, we had the same discussion about unknown options
> which were causing errors in the previous version of this patch.
> 
> My personal opinion is that given it's easy enough to notice/fix and it
> points at something that's obviously wrong, I think such breakage is a
> necessary evil and are occasionally ok -- but it should be intentional,
> so let's add some fallback for this version and we can make this break
> at the same time as we make unknown options break
> 
>> 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.

Sorry, I was out for the US holiday and just getting to this.

So previously, for cache mode we expected a string for the mount option,
converted that string to the numeric value via get_cache_mode(), and
v9fs_show_options displayed that cache mode value  as hexadecimal, right?

        if (v9ses->cache)
                seq_printf(m, ",cache=%x", v9ses->cache);

Oh, I see - the last "if" in get_cache_mode() accepted the bare numeric value.

>> The patch below fix the
>> issue for the cache option, but pretty sure all fsparam_enum options
>> should be fixed.
> 
> Oww. That's a bit more annoying, yes...
> 
>> 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?
> 
> Now these are most likely ok, it already changed when Eric (VH) made it
> display caches as hex a while ago, I wouldn't fuss too much about it.
> 
> OTOH if the old code worked I assume it parsed the hex values too, so
> that might be what we ought to do? Or was it just ignored?

Looks like it accepted either the string or the hex value, so that's my
mistake.

I suppose it would be a terrible hack to just extend the enum to include
hexadecimal "strings" like this, right.... ;)

+static const struct constant_table p9_cache_mode[] = {
+	{ "loose",	CACHE_SC_LOOSE },
+	{ "0b00000000",	CACHE_SC_LOOSE },
+	{ "fscache",	CACHE_SC_FSCACHE },
+	{ "0b10001111",	CACHE_SC_FSCACHE },
...
+	{}

I think the right approach would be to just reinstate get_cache_mode() to
do open-coded parsing as before, and get rid of the enum for the cache
option.

Would you like me to send a patch 5/4, or an updated 4/4 to implement this,
or would you rather do it yourself if you think you have a better chance
of getting it right than I do?

As for the other enum, I think we're still ok (though maybe you can confirm)
because p9_show_client_options() still does a switch on clnt->proto_version,
and outputs the appropriate mount option string.

-Eric 

> I'll try to find some time to play with this and let's send a patch
> before the merge window coming in fast... This was due for next
> week-ish!

Powered by blists - more mailing lists