* [PATCH V3 0/4] 9p: Convert to the new mount API
@ 2025-10-10 21:36 Eric Sandeen
2025-10-10 21:36 ` [PATCH V3 1/4] fs/fs_parse: add back fsparam_u32hex Eric Sandeen
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: Eric Sandeen @ 2025-10-10 21:36 UTC (permalink / raw)
To: v9fs
Cc: linux-fsdevel, linux-kernel, ericvh, lucho, asmadeus, linux_oss,
eadavis
This is another updated attempt to convert 9p to the new mount API. 9p
is one of the last conversions needed, possibly because it is one of the
trickier ones!
This has had only light testing. I confirmed that with the diod server,
both of these mount successfully:
# mount -t 9p -o aname=/tmp/9,version=9p2000.L,uname=fsgqa,access=user,trans=tcp 127.0.0.1 /mnt
and
# mount.diod localhost:/tmp/9 /mnt
(The latter passes trans=fd under the covers)
I have not been able to test other transports, or exhaustively test
functionality of all mount options.
Changes from V1 to V2:
Address "make W=1" warnings from kernel test robot, comments from
dhowells, and some kernel-doc comments for changed arguments.
Changes from V2 to V3:
Patch 1:
None
Patch 2:
None
Patch 3:
Change to not re-use v9fs_session_info and p9_client in the
v9fs_context structure. Instead, new structures p9_client_opts and
p9_session_opts are introduced. This avoids confusion about what is
used only for option parsing, and what is used for the actual
mounted instance.
Patch 4:
Allows unknown mount options as prior code did. I noticed that
mount.diod passes a "rwdepth" option by default, which was ignored
previously but breaks mount with strict unknown option rejection.
Adjust variable names in v9fs_parse_param, v9fs_apply_options,
and v9fs_init_fs_context to reflect new context structure and
to clarify difference.
Limit the msize option to INT_MAX as the old code did.
Remove redundant v9fs_set_super function, set_anon_super is enough.
Properly extract v9fs_seesion_info from sb->s_fs_info, not
the fc context, in v9fs_fill_super. (thanks eadavis@qq.com)
Remove unneeded fc arg from v9fs_fill_super.
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH V3 1/4] fs/fs_parse: add back fsparam_u32hex 2025-10-10 21:36 [PATCH V3 0/4] 9p: Convert to the new mount API Eric Sandeen @ 2025-10-10 21:36 ` Eric Sandeen 2025-10-10 21:36 ` [PATCH V3 2/4] net/9p: move structures and macros to header files Eric Sandeen ` (2 subsequent siblings) 3 siblings, 0 replies; 23+ messages in thread From: Eric Sandeen @ 2025-10-10 21:36 UTC (permalink / raw) To: v9fs Cc: linux-fsdevel, linux-kernel, ericvh, lucho, asmadeus, linux_oss, eadavis, Eric Sandeen 296b67059 removed fsparam_u32hex because there were no callers (yet) and it didn't build due to using the nonexistent symbol fs_param_is_u32_hex. fs/9p will need this parser, so add it back with the appropriate fix (use fs_param_is_u32). Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- include/linux/fs_parser.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h index 5a0e897cae80..5e8a3b546033 100644 --- a/include/linux/fs_parser.h +++ b/include/linux/fs_parser.h @@ -120,6 +120,8 @@ static inline bool fs_validate_description(const char *name, #define fsparam_u32(NAME, OPT) __fsparam(fs_param_is_u32, NAME, OPT, 0, NULL) #define fsparam_u32oct(NAME, OPT) \ __fsparam(fs_param_is_u32, NAME, OPT, 0, (void *)8) +#define fsparam_u32hex(NAME, OPT) \ + __fsparam(fs_param_is_u32, NAME, OPT, 0, (void *)16) #define fsparam_s32(NAME, OPT) __fsparam(fs_param_is_s32, NAME, OPT, 0, NULL) #define fsparam_u64(NAME, OPT) __fsparam(fs_param_is_u64, NAME, OPT, 0, NULL) #define fsparam_enum(NAME, OPT, array) __fsparam(fs_param_is_enum, NAME, OPT, 0, array) -- 2.51.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH V3 2/4] net/9p: move structures and macros to header files 2025-10-10 21:36 [PATCH V3 0/4] 9p: Convert to the new mount API Eric Sandeen 2025-10-10 21:36 ` [PATCH V3 1/4] fs/fs_parse: add back fsparam_u32hex Eric Sandeen @ 2025-10-10 21:36 ` Eric Sandeen 2025-10-10 21:36 ` [PATCH V3 3/4] 9p: create a v9fs_context structure to hold parsed options Eric Sandeen 2025-10-10 21:36 ` [PATCH V3 4/4] 9p: convert to the new mount API Eric Sandeen 3 siblings, 0 replies; 23+ messages in thread From: Eric Sandeen @ 2025-10-10 21:36 UTC (permalink / raw) To: v9fs Cc: linux-fsdevel, linux-kernel, ericvh, lucho, asmadeus, linux_oss, eadavis, Eric Sandeen With the new mount API all option parsing will need to happen in fs/v9fs.c, so move some existing data structures and macros to header files to facilitate this. Rename some to reflect the transport they are used for (rdma, fd, etc), for clarity. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- include/net/9p/client.h | 6 ++++++ include/net/9p/transport.h | 39 ++++++++++++++++++++++++++++++++++++++ net/9p/client.c | 6 ------ net/9p/trans_fd.c | 20 ++----------------- net/9p/trans_rdma.c | 25 ++---------------------- 5 files changed, 49 insertions(+), 47 deletions(-) diff --git a/include/net/9p/client.h b/include/net/9p/client.h index 4f785098c67a..2d46f8017bd5 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -16,6 +16,12 @@ /* Number of requests per row */ #define P9_ROW_MAXTAG 255 +/* DEFAULT MSIZE = 32 pages worth of payload + P9_HDRSZ + + * room for write (16 extra) or read (11 extra) operands. + */ + +#define DEFAULT_MSIZE ((128 * 1024) + P9_IOHDRSZ) + /** enum p9_proto_versions - 9P protocol versions * @p9_proto_legacy: 9P Legacy mode, pre-9P2000.u * @p9_proto_2000u: 9P2000.u extension diff --git a/include/net/9p/transport.h b/include/net/9p/transport.h index 766ec07c9599..88702953b1ef 100644 --- a/include/net/9p/transport.h +++ b/include/net/9p/transport.h @@ -14,6 +14,45 @@ #define P9_DEF_MIN_RESVPORT (665U) #define P9_DEF_MAX_RESVPORT (1023U) +#define P9_FD_PORT 564 + +#define P9_RDMA_PORT 5640 +#define P9_RDMA_SQ_DEPTH 32 +#define P9_RDMA_RQ_DEPTH 32 +#define P9_RDMA_TIMEOUT 30000 /* 30 seconds */ + +/** + * struct p9_fd_opts - per-transport options for fd transport + * @rfd: file descriptor for reading (trans=fd) + * @wfd: file descriptor for writing (trans=fd) + * @port: port to connect to (trans=tcp) + * @privport: port is privileged + */ + +struct p9_fd_opts { + int rfd; + int wfd; + u16 port; + bool privport; +}; + +/** + * struct p9_rdma_opts - Collection of mount options for rdma transport + * @port: port of connection + * @privport: Whether a privileged port may be used + * @sq_depth: The requested depth of the SQ. This really doesn't need + * to be any deeper than the number of threads used in the client + * @rq_depth: The depth of the RQ. Should be greater than or equal to SQ depth + * @timeout: Time to wait in msecs for CM events + */ +struct p9_rdma_opts { + short port; + bool privport; + int sq_depth; + int rq_depth; + long timeout; +}; + /** * struct p9_trans_module - transport module interface * @list: used to maintain a list of currently available transports diff --git a/net/9p/client.c b/net/9p/client.c index 5c1ca57ccd28..5e3230b1bfab 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -29,12 +29,6 @@ #define CREATE_TRACE_POINTS #include <trace/events/9p.h> -/* DEFAULT MSIZE = 32 pages worth of payload + P9_HDRSZ + - * room for write (16 extra) or read (11 extra) operands. - */ - -#define DEFAULT_MSIZE ((128 * 1024) + P9_IOHDRSZ) - /* Client Option Parsing (code inspired by NFS code) * - a little lazy - parse all client options */ diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 339ec4e54778..9ef4f2e0db3c 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -31,28 +31,12 @@ #include <linux/syscalls.h> /* killme */ -#define P9_PORT 564 #define MAX_SOCK_BUF (1024*1024) #define MAXPOLLWADDR 2 static struct p9_trans_module p9_tcp_trans; static struct p9_trans_module p9_fd_trans; -/** - * struct p9_fd_opts - per-transport options - * @rfd: file descriptor for reading (trans=fd) - * @wfd: file descriptor for writing (trans=fd) - * @port: port to connect to (trans=tcp) - * @privport: port is privileged - */ - -struct p9_fd_opts { - int rfd; - int wfd; - u16 port; - bool privport; -}; - /* * Option Parsing (code inspired by NFS code) * - a little lazy - parse all fd-transport options @@ -749,7 +733,7 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req) static int p9_fd_show_options(struct seq_file *m, struct p9_client *clnt) { if (clnt->trans_mod == &p9_tcp_trans) { - if (clnt->trans_opts.tcp.port != P9_PORT) + if (clnt->trans_opts.tcp.port != P9_FD_PORT) seq_printf(m, ",port=%u", clnt->trans_opts.tcp.port); } else if (clnt->trans_mod == &p9_fd_trans) { if (clnt->trans_opts.fd.rfd != ~0) @@ -775,7 +759,7 @@ static int parse_opts(char *params, struct p9_fd_opts *opts) int option; char *options, *tmp_options; - opts->port = P9_PORT; + opts->port = P9_FD_PORT; opts->rfd = ~0; opts->wfd = ~0; opts->privport = false; diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c index b84748baf9cb..46ee37061faf 100644 --- a/net/9p/trans_rdma.c +++ b/net/9p/trans_rdma.c @@ -32,14 +32,10 @@ #include <rdma/ib_verbs.h> #include <rdma/rdma_cm.h> -#define P9_PORT 5640 -#define P9_RDMA_SQ_DEPTH 32 -#define P9_RDMA_RQ_DEPTH 32 #define P9_RDMA_SEND_SGE 4 #define P9_RDMA_RECV_SGE 4 #define P9_RDMA_IRD 0 #define P9_RDMA_ORD 0 -#define P9_RDMA_TIMEOUT 30000 /* 30 seconds */ #define P9_RDMA_MAXSIZE (1024*1024) /* 1MB */ /** @@ -110,23 +106,6 @@ struct p9_rdma_context { }; }; -/** - * struct p9_rdma_opts - Collection of mount options - * @port: port of connection - * @privport: Whether a privileged port may be used - * @sq_depth: The requested depth of the SQ. This really doesn't need - * to be any deeper than the number of threads used in the client - * @rq_depth: The depth of the RQ. Should be greater than or equal to SQ depth - * @timeout: Time to wait in msecs for CM events - */ -struct p9_rdma_opts { - short port; - bool privport; - int sq_depth; - int rq_depth; - long timeout; -}; - /* * Option Parsing (code inspired by NFS code) */ @@ -151,7 +130,7 @@ static int p9_rdma_show_options(struct seq_file *m, struct p9_client *clnt) { struct p9_trans_rdma *rdma = clnt->trans; - if (rdma->port != P9_PORT) + if (rdma->port != P9_RDMA_PORT) seq_printf(m, ",port=%u", rdma->port); if (rdma->sq_depth != P9_RDMA_SQ_DEPTH) seq_printf(m, ",sq=%u", rdma->sq_depth); @@ -178,7 +157,7 @@ static int parse_opts(char *params, struct p9_rdma_opts *opts) int option; char *options, *tmp_options; - opts->port = P9_PORT; + opts->port = P9_RDMA_PORT; opts->sq_depth = P9_RDMA_SQ_DEPTH; opts->rq_depth = P9_RDMA_RQ_DEPTH; opts->timeout = P9_RDMA_TIMEOUT; -- 2.51.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH V3 3/4] 9p: create a v9fs_context structure to hold parsed options 2025-10-10 21:36 [PATCH V3 0/4] 9p: Convert to the new mount API Eric Sandeen 2025-10-10 21:36 ` [PATCH V3 1/4] fs/fs_parse: add back fsparam_u32hex Eric Sandeen 2025-10-10 21:36 ` [PATCH V3 2/4] net/9p: move structures and macros to header files Eric Sandeen @ 2025-10-10 21:36 ` Eric Sandeen 2025-10-10 21:36 ` [PATCH V3 4/4] 9p: convert to the new mount API Eric Sandeen 3 siblings, 0 replies; 23+ messages in thread From: Eric Sandeen @ 2025-10-10 21:36 UTC (permalink / raw) To: v9fs Cc: linux-fsdevel, linux-kernel, ericvh, lucho, asmadeus, linux_oss, eadavis, Eric Sandeen This patch creates a new v9fs_context structure which includes new p9_session_opts and p9_client_opts structures, as well as re-using the existing p9_fd_opts and p9_rdma_opts to store options during parsing. The new structure will be used in the next commit to pass all parsed options to the appropriate transports. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- include/net/9p/client.h | 90 ++++++++++++++++++++++++++++++++++++++ include/net/9p/transport.h | 32 -------------- 2 files changed, 90 insertions(+), 32 deletions(-) diff --git a/include/net/9p/client.h b/include/net/9p/client.h index 2d46f8017bd5..cc18443f7d51 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -132,6 +132,96 @@ struct p9_client { char name[__NEW_UTS_LEN + 1]; }; +/** + * struct p9_fd_opts - holds client options during parsing + * @msize: maximum data size negotiated by protocol + * @prot-Oversion: 9P protocol version to use + * @trans_mod: module API instantiated with this client + * + * These parsed options get transferred into client in + * apply_client_options() + */ +struct p9_client_opts { + unsigned int msize; + unsigned char proto_version; + struct p9_trans_module *trans_mod; +}; + +/** + * struct p9_fd_opts - per-transport options for fd transport + * @rfd: file descriptor for reading (trans=fd) + * @wfd: file descriptor for writing (trans=fd) + * @port: port to connect to (trans=tcp) + * @privport: port is privileged + */ +struct p9_fd_opts { + int rfd; + int wfd; + u16 port; + bool privport; +}; + +/** + * struct p9_rdma_opts - Collection of mount options for rdma transport + * @port: port of connection + * @privport: Whether a privileged port may be used + * @sq_depth: The requested depth of the SQ. This really doesn't need + * to be any deeper than the number of threads used in the client + * @rq_depth: The depth of the RQ. Should be greater than or equal to SQ depth + * @timeout: Time to wait in msecs for CM events + */ +struct p9_rdma_opts { + short port; + bool privport; + int sq_depth; + int rq_depth; + long timeout; +}; + +/** + * struct p9_session_opts - holds parsed options for v9fs_session_info + * @flags: session options of type &p9_session_flags + * @nodev: set to 1 to disable device mapping + * @debug: debug level + * @afid: authentication handle + * @cache: cache mode of type &p9_cache_bits + * @cachetag: the tag of the cache associated with this session + * @uname: string user name to mount hierarchy as + * @aname: mount specifier for remote hierarchy + * @dfltuid: default numeric userid to mount hierarchy as + * @dfltgid: default numeric groupid to mount hierarchy as + * @uid: if %V9FS_ACCESS_SINGLE, the numeric uid which mounted the hierarchy + * @session_lock_timeout: retry interval for blocking locks + * + * This strucure holds options which are parsed and will be transferred + * to the v9fs_session_info structure when mounted, and therefore largely + * duplicates struct v9fs_session_info. + */ +struct p9_session_opts { + unsigned int flags; + unsigned char nodev; + unsigned short debug; + unsigned int afid; + unsigned int cache; +#ifdef CONFIG_9P_FSCACHE + char *cachetag; +#endif + char *uname; + char *aname; + kuid_t dfltuid; + kgid_t dfltgid; + kuid_t uid; + long session_lock_timeout; +}; + +/* Used by mount API to store parsed mount options */ +struct v9fs_context { + struct p9_client_opts client_opts; + struct p9_fd_opts fd_opts; + struct p9_rdma_opts rdma_opts; + struct p9_session_opts session_opts; +}; + /** * struct p9_fid - file system entity handle * @clnt: back pointer to instantiating &p9_client diff --git a/include/net/9p/transport.h b/include/net/9p/transport.h index 88702953b1ef..ebbb4b50ee20 100644 --- a/include/net/9p/transport.h +++ b/include/net/9p/transport.h @@ -21,38 +21,6 @@ #define P9_RDMA_RQ_DEPTH 32 #define P9_RDMA_TIMEOUT 30000 /* 30 seconds */ -/** - * struct p9_fd_opts - per-transport options for fd transport - * @rfd: file descriptor for reading (trans=fd) - * @wfd: file descriptor for writing (trans=fd) - * @port: port to connect to (trans=tcp) - * @privport: port is privileged - */ - -struct p9_fd_opts { - int rfd; - int wfd; - u16 port; - bool privport; -}; - -/** - * struct p9_rdma_opts - Collection of mount options for rdma transport - * @port: port of connection - * @privport: Whether a privileged port may be used - * @sq_depth: The requested depth of the SQ. This really doesn't need - * to be any deeper than the number of threads used in the client - * @rq_depth: The depth of the RQ. Should be greater than or equal to SQ depth - * @timeout: Time to wait in msecs for CM events - */ -struct p9_rdma_opts { - short port; - bool privport; - int sq_depth; - int rq_depth; - long timeout; -}; - /** * struct p9_trans_module - transport module interface * @list: used to maintain a list of currently available transports -- 2.51.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH V3 4/4] 9p: convert to the new mount API 2025-10-10 21:36 [PATCH V3 0/4] 9p: Convert to the new mount API Eric Sandeen ` (2 preceding siblings ...) 2025-10-10 21:36 ` [PATCH V3 3/4] 9p: create a v9fs_context structure to hold parsed options Eric Sandeen @ 2025-10-10 21:36 ` Eric Sandeen 2025-10-13 10:26 ` Dominique Martinet ` (2 more replies) 3 siblings, 3 replies; 23+ messages in thread From: Eric Sandeen @ 2025-10-10 21:36 UTC (permalink / raw) To: v9fs Cc: linux-fsdevel, linux-kernel, ericvh, lucho, asmadeus, linux_oss, eadavis, Eric Sandeen Convert 9p to the new mount API. This patch consolidates all parsing into fs/9p/v9fs.c, which stores all results into a filesystem context which can be passed to the various transports as needed. Some of the parsing helper functions such as get_cache_mode() have been eliminated in favor of using the new mount API's enum param type, for simplicity. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- fs/9p/v9fs.c | 539 +++++++++++++++++++------------------ fs/9p/v9fs.h | 7 +- fs/9p/vfs_super.c | 130 ++++++--- include/net/9p/client.h | 2 +- include/net/9p/transport.h | 2 +- net/9p/client.c | 148 +--------- net/9p/mod.c | 2 +- net/9p/trans_fd.c | 109 +------- net/9p/trans_rdma.c | 108 +------- net/9p/trans_usbg.c | 4 +- net/9p/trans_virtio.c | 8 +- net/9p/trans_xen.c | 4 +- 12 files changed, 411 insertions(+), 652 deletions(-) diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c index 77e9c4387c1d..07be64e73711 100644 --- a/fs/9p/v9fs.c +++ b/fs/9p/v9fs.c @@ -13,7 +13,8 @@ #include <linux/fs.h> #include <linux/sched.h> #include <linux/cred.h> -#include <linux/parser.h> +#include <linux/fs_parser.h> +#include <linux/fs_context.h> #include <linux/slab.h> #include <linux/seq_file.h> #include <net/9p/9p.h> @@ -43,55 +44,80 @@ enum { Opt_access, Opt_posixacl, /* Lock timeout option */ Opt_locktimeout, - /* Error token */ - Opt_err + + /* Client options */ + Opt_msize, Opt_trans, Opt_legacy, Opt_version, + + /* fd transport options */ + /* Options that take integer arguments */ + Opt_rfdno, Opt_wfdno, + /* Options that take no arguments */ + + /* rdma transport options */ + /* Options that take integer arguments */ + Opt_rq_depth, Opt_sq_depth, Opt_timeout, + + /* Options for both fd and rdma transports */ + Opt_port, Opt_privport, }; -static const match_table_t tokens = { - {Opt_debug, "debug=%x"}, - {Opt_dfltuid, "dfltuid=%u"}, - {Opt_dfltgid, "dfltgid=%u"}, - {Opt_afid, "afid=%u"}, - {Opt_uname, "uname=%s"}, - {Opt_remotename, "aname=%s"}, - {Opt_nodevmap, "nodevmap"}, - {Opt_noxattr, "noxattr"}, - {Opt_directio, "directio"}, - {Opt_ignoreqv, "ignoreqv"}, - {Opt_cache, "cache=%s"}, - {Opt_cachetag, "cachetag=%s"}, - {Opt_access, "access=%s"}, - {Opt_posixacl, "posixacl"}, - {Opt_locktimeout, "locktimeout=%u"}, - {Opt_err, NULL} +static const struct constant_table p9_versions[] = { + { "9p2000", p9_proto_legacy }, + { "9p2000.u", p9_proto_2000u }, + { "9p2000.L", p9_proto_2000L }, + {} }; -/* Interpret mount options for cache mode */ -static int get_cache_mode(char *s) -{ - int version = -EINVAL; - - if (!strcmp(s, "loose")) { - version = CACHE_SC_LOOSE; - p9_debug(P9_DEBUG_9P, "Cache mode: loose\n"); - } else if (!strcmp(s, "fscache")) { - version = CACHE_SC_FSCACHE; - p9_debug(P9_DEBUG_9P, "Cache mode: fscache\n"); - } else if (!strcmp(s, "mmap")) { - version = CACHE_SC_MMAP; - p9_debug(P9_DEBUG_9P, "Cache mode: mmap\n"); - } else if (!strcmp(s, "readahead")) { - version = CACHE_SC_READAHEAD; - p9_debug(P9_DEBUG_9P, "Cache mode: readahead\n"); - } else if (!strcmp(s, "none")) { - version = CACHE_SC_NONE; - p9_debug(P9_DEBUG_9P, "Cache mode: none\n"); - } else if (kstrtoint(s, 0, &version) != 0) { - version = -EINVAL; - pr_info("Unknown Cache mode or invalid value %s\n", s); - } - return version; -} +static const struct constant_table p9_cache_mode[] = { + { "loose", CACHE_SC_LOOSE }, + { "fscache", CACHE_SC_FSCACHE }, + { "mmap", CACHE_SC_MMAP }, + { "readahead", CACHE_SC_READAHEAD }, + { "none", CACHE_SC_NONE }, + {} +}; + +/* + * This structure contains all parameters used for the core code, + * the client, and all the transports. + */ +const struct fs_parameter_spec v9fs_param_spec[] = { + fsparam_u32hex ("debug", Opt_debug), + fsparam_uid ("dfltuid", Opt_dfltuid), + fsparam_gid ("dfltgid", Opt_dfltgid), + fsparam_u32 ("afid", Opt_afid), + fsparam_string ("uname", Opt_uname), + fsparam_string ("aname", Opt_remotename), + fsparam_flag ("nodevmap", Opt_nodevmap), + fsparam_flag ("noxattr", Opt_noxattr), + fsparam_flag ("directio", Opt_directio), + fsparam_flag ("ignoreqv", Opt_ignoreqv), + fsparam_enum ("cache", Opt_cache, p9_cache_mode), + fsparam_string ("cachetag", Opt_cachetag), + fsparam_string ("access", Opt_access), + fsparam_flag ("posixacl", Opt_posixacl), + fsparam_u32 ("locktimeout", Opt_locktimeout), + + /* client options */ + fsparam_u32 ("msize", Opt_msize), + fsparam_flag ("noextend", Opt_legacy), + fsparam_string ("trans", Opt_trans), + fsparam_enum ("version", Opt_version, p9_versions), + + /* fd transport options */ + fsparam_u32 ("rfdno", Opt_rfdno), + fsparam_u32 ("wfdno", Opt_wfdno), + + /* rdma transport options */ + fsparam_u32 ("sq", Opt_sq_depth), + fsparam_u32 ("rq", Opt_rq_depth), + fsparam_u32 ("timeout", Opt_timeout), + + /* fd and rdma transprt options */ + fsparam_u32 ("port", Opt_port), + fsparam_flag ("privport", Opt_privport), + {} +}; /* * Display the mount options in /proc/mounts. @@ -153,267 +179,244 @@ int v9fs_show_options(struct seq_file *m, struct dentry *root) } /** - * v9fs_parse_options - parse mount options into session structure - * @v9ses: existing v9fs session information - * @opts: The mount option string + * v9fs_parse_param - parse a mount option into the filesystem context + * @fc: the filesystem context + * @param: the parameter to parse * * Return 0 upon success, -ERRNO upon failure. */ - -static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts) +int v9fs_parse_param(struct fs_context *fc, struct fs_parameter *param) { - char *options, *tmp_options; - substring_t args[MAX_OPT_ARGS]; - char *p; - int option = 0; + struct v9fs_context *ctx = fc->fs_private; + struct fs_parse_result result; char *s; - int ret = 0; - - /* setup defaults */ - v9ses->afid = ~0; - v9ses->debug = 0; - v9ses->cache = CACHE_NONE; -#ifdef CONFIG_9P_FSCACHE - v9ses->cachetag = NULL; -#endif - v9ses->session_lock_timeout = P9_LOCK_TIMEOUT; - - if (!opts) - return 0; + int r; + int opt; + struct p9_client_opts *clnt = &ctx->client_opts; + struct p9_fd_opts *fd_opts = &ctx->fd_opts; + struct p9_rdma_opts *rdma_opts = &ctx->rdma_opts; + struct p9_session_opts *session_opts = &ctx->session_opts; + + opt = fs_parse(fc, v9fs_param_spec, param, &result); + if (opt < 0) { + /* + * We might like to report bad mount options here, but + * traditionally 9p has ignored unknown mount options + */ + if (opt == -ENOPARAM) + return 0; - tmp_options = kstrdup(opts, GFP_KERNEL); - if (!tmp_options) { - ret = -ENOMEM; - goto fail_option_alloc; + return opt; } - options = tmp_options; - - while ((p = strsep(&options, ",")) != NULL) { - int token, r; - - if (!*p) - continue; - - token = match_token(p, tokens, args); - switch (token) { - case Opt_debug: - r = match_int(&args[0], &option); - if (r < 0) { - p9_debug(P9_DEBUG_ERROR, - "integer field, but no integer?\n"); - ret = r; - } else { - v9ses->debug = option; + + switch (opt) { + case Opt_debug: + session_opts->debug = result.uint_32; #ifdef CONFIG_NET_9P_DEBUG - p9_debug_level = option; + p9_debug_level = result.uint_32; #endif - } - break; - - case Opt_dfltuid: - r = match_int(&args[0], &option); - if (r < 0) { - p9_debug(P9_DEBUG_ERROR, - "integer field, but no integer?\n"); - ret = r; - continue; - } - v9ses->dfltuid = make_kuid(current_user_ns(), option); - if (!uid_valid(v9ses->dfltuid)) { - p9_debug(P9_DEBUG_ERROR, - "uid field, but not a uid?\n"); - ret = -EINVAL; - } - break; - case Opt_dfltgid: - r = match_int(&args[0], &option); - if (r < 0) { - p9_debug(P9_DEBUG_ERROR, - "integer field, but no integer?\n"); - ret = r; - continue; - } - v9ses->dfltgid = make_kgid(current_user_ns(), option); - if (!gid_valid(v9ses->dfltgid)) { - p9_debug(P9_DEBUG_ERROR, - "gid field, but not a gid?\n"); - ret = -EINVAL; - } - break; - case Opt_afid: - r = match_int(&args[0], &option); - if (r < 0) { - p9_debug(P9_DEBUG_ERROR, - "integer field, but no integer?\n"); - ret = r; - } else { - v9ses->afid = option; - } - break; - case Opt_uname: - kfree(v9ses->uname); - v9ses->uname = match_strdup(&args[0]); - if (!v9ses->uname) { - ret = -ENOMEM; - goto free_and_return; - } - break; - case Opt_remotename: - kfree(v9ses->aname); - v9ses->aname = match_strdup(&args[0]); - if (!v9ses->aname) { - ret = -ENOMEM; - goto free_and_return; - } - break; - case Opt_nodevmap: - v9ses->nodev = 1; - break; - case Opt_noxattr: - v9ses->flags |= V9FS_NO_XATTR; - break; - case Opt_directio: - v9ses->flags |= V9FS_DIRECT_IO; - break; - case Opt_ignoreqv: - v9ses->flags |= V9FS_IGNORE_QV; - break; - case Opt_cachetag: + break; + + case Opt_dfltuid: + session_opts->dfltuid = result.uid; + break; + case Opt_dfltgid: + session_opts->dfltgid = result.gid; + break; + case Opt_afid: + session_opts->afid = result.uint_32; + break; + case Opt_uname: + kfree(session_opts->uname); + session_opts->uname = param->string; + param->string = NULL; + break; + case Opt_remotename: + kfree(session_opts->aname); + session_opts->aname = param->string; + param->string = NULL; + break; + case Opt_nodevmap: + session_opts->nodev = 1; + break; + case Opt_noxattr: + session_opts->flags |= V9FS_NO_XATTR; + break; + case Opt_directio: + session_opts->flags |= V9FS_DIRECT_IO; + break; + case Opt_ignoreqv: + session_opts->flags |= V9FS_IGNORE_QV; + break; + case Opt_cachetag: #ifdef CONFIG_9P_FSCACHE - kfree(v9ses->cachetag); - v9ses->cachetag = match_strdup(&args[0]); - if (!v9ses->cachetag) { - ret = -ENOMEM; - goto free_and_return; - } + kfree(session_opts->cachetag); + session_opts->cachetag = param->string; + param->string = NULL; #endif - break; - case Opt_cache: - s = match_strdup(&args[0]); - if (!s) { - ret = -ENOMEM; - p9_debug(P9_DEBUG_ERROR, - "problem allocating copy of cache arg\n"); - goto free_and_return; - } - r = get_cache_mode(s); - if (r < 0) - ret = r; - else - v9ses->cache = r; - - kfree(s); - break; - - case Opt_access: - s = match_strdup(&args[0]); - if (!s) { - ret = -ENOMEM; - p9_debug(P9_DEBUG_ERROR, - "problem allocating copy of access arg\n"); - goto free_and_return; + break; + case Opt_cache: + session_opts->cache = result.uint_32; + p9_debug(P9_DEBUG_9P, "Cache mode: %s\n", param->string); + break; + case Opt_access: + s = param->string; + session_opts->flags &= ~V9FS_ACCESS_MASK; + if (strcmp(s, "user") == 0) { + session_opts->flags |= V9FS_ACCESS_USER; + } else if (strcmp(s, "any") == 0) { + session_opts->flags |= V9FS_ACCESS_ANY; + } else if (strcmp(s, "client") == 0) { + session_opts->flags |= V9FS_ACCESS_CLIENT; + } else { + uid_t uid; + + session_opts->flags |= V9FS_ACCESS_SINGLE; + r = kstrtouint(s, 10, &uid); + if (r) { + pr_info("Unknown access argument %s: %d\n", + param->string, r); + return r; } - - v9ses->flags &= ~V9FS_ACCESS_MASK; - if (strcmp(s, "user") == 0) - v9ses->flags |= V9FS_ACCESS_USER; - else if (strcmp(s, "any") == 0) - v9ses->flags |= V9FS_ACCESS_ANY; - else if (strcmp(s, "client") == 0) { - v9ses->flags |= V9FS_ACCESS_CLIENT; - } else { - uid_t uid; - - v9ses->flags |= V9FS_ACCESS_SINGLE; - r = kstrtouint(s, 10, &uid); - if (r) { - ret = r; - pr_info("Unknown access argument %s: %d\n", - s, r); - kfree(s); - continue; - } - v9ses->uid = make_kuid(current_user_ns(), uid); - if (!uid_valid(v9ses->uid)) { - ret = -EINVAL; - pr_info("Unknown uid %s\n", s); - } + session_opts->uid = make_kuid(current_user_ns(), uid); + if (!uid_valid(session_opts->uid)) { + pr_info("Unknown uid %s\n", s); + return -EINVAL; } + } + break; - kfree(s); - break; - - case Opt_posixacl: + case Opt_posixacl: #ifdef CONFIG_9P_FS_POSIX_ACL - v9ses->flags |= V9FS_POSIX_ACL; + session_opts->flags |= V9FS_POSIX_ACL; #else - p9_debug(P9_DEBUG_ERROR, - "Not defined CONFIG_9P_FS_POSIX_ACL. Ignoring posixacl option\n"); + p9_debug(P9_DEBUG_ERROR, + "Not defined CONFIG_9P_FS_POSIX_ACL. Ignoring posixacl option\n"); #endif - break; - - case Opt_locktimeout: - r = match_int(&args[0], &option); - if (r < 0) { - p9_debug(P9_DEBUG_ERROR, - "integer field, but no integer?\n"); - ret = r; - continue; - } - if (option < 1) { - p9_debug(P9_DEBUG_ERROR, - "locktimeout must be a greater than zero integer.\n"); - ret = -EINVAL; - continue; - } - v9ses->session_lock_timeout = (long)option * HZ; - break; + break; - default: - continue; + case Opt_locktimeout: + if (result.uint_32 < 1) { + p9_debug(P9_DEBUG_ERROR, + "locktimeout must be a greater than zero integer.\n"); + return -EINVAL; + } + session_opts->session_lock_timeout = (long)result.uint_32 * HZ; + break; + + /* Options for client */ + case Opt_msize: + if (result.uint_32 < 4096) { + p9_debug(P9_DEBUG_ERROR, "msize should be at least 4k\n"); + return -EINVAL; + } + if (result.uint_32 > INT_MAX) { + p9_debug(P9_DEBUG_ERROR, "msize too big\n"); + return -EINVAL; } + clnt->msize = result.uint_32; + break; + case Opt_trans: + v9fs_put_trans(clnt->trans_mod); + clnt->trans_mod = v9fs_get_trans_by_name(param->string); + if (!clnt->trans_mod) { + pr_info("Could not find request transport: %s\n", + param->string); + return -EINVAL; + } + break; + case Opt_legacy: + clnt->proto_version = p9_proto_legacy; + break; + case Opt_version: + clnt->proto_version = result.uint_32; + p9_debug(P9_DEBUG_9P, "Protocol version: %s\n", param->string); + break; + /* Options for fd transport */ + case Opt_rfdno: + fd_opts->rfd = result.uint_32; + break; + case Opt_wfdno: + fd_opts->wfd = result.uint_32; + break; + /* Options for rdma transport */ + case Opt_sq_depth: + rdma_opts->sq_depth = result.uint_32; + break; + case Opt_rq_depth: + rdma_opts->rq_depth = result.uint_32; + break; + case Opt_timeout: + rdma_opts->timeout = result.uint_32; + break; + /* Options for both fd and rdma transports */ + case Opt_port: + fd_opts->port = result.uint_32; + rdma_opts->port = result.uint_32; + break; + case Opt_privport: + fd_opts->privport = true; + rdma_opts->port = true; + break; } -free_and_return: - kfree(tmp_options); -fail_option_alloc: - return ret; + return 0; +} + +static void v9fs_apply_options(struct v9fs_session_info *v9ses, + struct fs_context *fc) +{ + struct v9fs_context *ctx = fc->fs_private; + + v9ses->debug = ctx->session_opts.debug; + v9ses->dfltuid = ctx->session_opts.dfltuid; + v9ses->dfltgid = ctx->session_opts.dfltgid; + v9ses->afid = ctx->session_opts.afid; + v9ses->uname = ctx->session_opts.uname; + ctx->session_opts.uname = NULL; + v9ses->aname = ctx->session_opts.aname; + ctx->session_opts.aname = NULL; + v9ses->nodev = ctx->session_opts.nodev; + /* + * Note that we must |= flags here as session_init already + * set basic flags. This adds in flags from parsed options. + */ + v9ses->flags |= ctx->session_opts.flags; +#ifdef CONFIG_9P_FSCACHE + v9ses->cachetag = ctx->session_opts.cachetag; + ctx->session_opts.cachetag = NULL; +#endif + v9ses->cache = ctx->session_opts.cache; + v9ses->uid = ctx->session_opts.uid; + v9ses->session_lock_timeout = ctx->session_opts.session_lock_timeout; } /** * v9fs_session_init - initialize session * @v9ses: session information structure - * @dev_name: device being mounted - * @data: options + * @fc: the filesystem mount context * */ struct p9_fid *v9fs_session_init(struct v9fs_session_info *v9ses, - const char *dev_name, char *data) + struct fs_context *fc) { struct p9_fid *fid; int rc = -ENOMEM; - v9ses->uname = kstrdup(V9FS_DEFUSER, GFP_KERNEL); - if (!v9ses->uname) - goto err_names; - - v9ses->aname = kstrdup(V9FS_DEFANAME, GFP_KERNEL); - if (!v9ses->aname) - goto err_names; init_rwsem(&v9ses->rename_sem); - v9ses->uid = INVALID_UID; - v9ses->dfltuid = V9FS_DEFUID; - v9ses->dfltgid = V9FS_DEFGID; - - v9ses->clnt = p9_client_create(dev_name, data); + v9ses->clnt = p9_client_create(fc); if (IS_ERR(v9ses->clnt)) { rc = PTR_ERR(v9ses->clnt); p9_debug(P9_DEBUG_ERROR, "problem initializing 9p client\n"); goto err_names; } + /* + * Initialize flags on the real v9ses. v9fs_apply_options below + * will |= the additional flags from parsed options. + */ v9ses->flags = V9FS_ACCESS_USER; if (p9_is_proto_dotl(v9ses->clnt)) { @@ -423,9 +426,7 @@ struct p9_fid *v9fs_session_init(struct v9fs_session_info *v9ses, v9ses->flags |= V9FS_PROTO_2000U; } - rc = v9fs_parse_options(v9ses, data); - if (rc < 0) - goto err_clnt; + v9fs_apply_options(v9ses, fc); v9ses->maxdata = v9ses->clnt->msize - P9_IOHDRSZ; @@ -472,7 +473,7 @@ struct p9_fid *v9fs_session_init(struct v9fs_session_info *v9ses, #ifdef CONFIG_9P_FSCACHE /* register the session for caching */ if (v9ses->cache & CACHE_FSCACHE) { - rc = v9fs_cache_session_get_cookie(v9ses, dev_name); + rc = v9fs_cache_session_get_cookie(v9ses, fc->source); if (rc < 0) goto err_clnt; } diff --git a/fs/9p/v9fs.h b/fs/9p/v9fs.h index f28bc763847a..6a12445d3858 100644 --- a/fs/9p/v9fs.h +++ b/fs/9p/v9fs.h @@ -10,6 +10,9 @@ #include <linux/backing-dev.h> #include <linux/netfs.h> +#include <linux/fs_parser.h> +#include <net/9p/client.h> +#include <net/9p/transport.h> /** * enum p9_session_flags - option flags for each 9P session @@ -163,11 +166,13 @@ static inline struct fscache_volume *v9fs_session_cache(struct v9fs_session_info #endif } +extern const struct fs_parameter_spec v9fs_param_spec[]; +extern int v9fs_parse_param(struct fs_context *fc, struct fs_parameter *param); extern int v9fs_show_options(struct seq_file *m, struct dentry *root); struct p9_fid *v9fs_session_init(struct v9fs_session_info *v9ses, - const char *dev_name, char *data); + struct fs_context *fc); extern void v9fs_session_close(struct v9fs_session_info *v9ses); extern void v9fs_session_cancel(struct v9fs_session_info *v9ses); extern void v9fs_session_begin_cancel(struct v9fs_session_info *v9ses); diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c index 1581ebac5bb4..315336de6f02 100644 --- a/fs/9p/vfs_super.c +++ b/fs/9p/vfs_super.c @@ -19,6 +19,7 @@ #include <linux/statfs.h> #include <linux/magic.h> #include <linux/fscache.h> +#include <linux/fs_context.h> #include <net/9p/9p.h> #include <net/9p/client.h> @@ -30,32 +31,10 @@ static const struct super_operations v9fs_super_ops, v9fs_super_ops_dotl; -/** - * v9fs_set_super - set the superblock - * @s: super block - * @data: file system specific data - * - */ - -static int v9fs_set_super(struct super_block *s, void *data) -{ - s->s_fs_info = data; - return set_anon_super(s, data); -} - -/** - * v9fs_fill_super - populate superblock with info - * @sb: superblock - * @v9ses: session information - * @flags: flags propagated from v9fs_mount() - * - */ - -static int -v9fs_fill_super(struct super_block *sb, struct v9fs_session_info *v9ses, - int flags) +static int v9fs_fill_super(struct super_block *sb) { int ret; + struct v9fs_session_info *v9ses = v9ses = sb->s_fs_info; sb->s_maxbytes = MAX_LFS_FILESIZE; sb->s_blocksize_bits = fls(v9ses->maxdata - 1); @@ -95,16 +74,12 @@ v9fs_fill_super(struct super_block *sb, struct v9fs_session_info *v9ses, } /** - * v9fs_mount - mount a superblock - * @fs_type: file system type - * @flags: mount flags - * @dev_name: device name that was mounted - * @data: mount options + * v9fs_get_tree - create the mountable root and superblock + * @fc: the filesystem context * */ -static struct dentry *v9fs_mount(struct file_system_type *fs_type, int flags, - const char *dev_name, void *data) +static int v9fs_get_tree(struct fs_context *fc) { struct super_block *sb = NULL; struct inode *inode = NULL; @@ -117,20 +92,21 @@ static struct dentry *v9fs_mount(struct file_system_type *fs_type, int flags, v9ses = kzalloc(sizeof(struct v9fs_session_info), GFP_KERNEL); if (!v9ses) - return ERR_PTR(-ENOMEM); + return -ENOMEM; - fid = v9fs_session_init(v9ses, dev_name, data); + fid = v9fs_session_init(v9ses, fc); if (IS_ERR(fid)) { retval = PTR_ERR(fid); goto free_session; } - sb = sget(fs_type, NULL, v9fs_set_super, flags, v9ses); + fc->s_fs_info = v9ses; + sb = sget_fc(fc, NULL, set_anon_super_fc); if (IS_ERR(sb)) { retval = PTR_ERR(sb); goto clunk_fid; } - retval = v9fs_fill_super(sb, v9ses, flags); + retval = v9fs_fill_super(sb); if (retval) goto release_sb; @@ -159,14 +135,15 @@ static struct dentry *v9fs_mount(struct file_system_type *fs_type, int flags, v9fs_fid_add(root, &fid); p9_debug(P9_DEBUG_VFS, " simple set mount, return 0\n"); - return dget(sb->s_root); + fc->root = dget(sb->s_root); + return 0; clunk_fid: p9_fid_put(fid); v9fs_session_close(v9ses); free_session: kfree(v9ses); - return ERR_PTR(retval); + return retval; release_sb: /* @@ -177,7 +154,7 @@ static struct dentry *v9fs_mount(struct file_system_type *fs_type, int flags, */ p9_fid_put(fid); deactivate_locked_super(sb); - return ERR_PTR(retval); + return retval; } /** @@ -303,11 +280,86 @@ static const struct super_operations v9fs_super_ops_dotl = { .write_inode = v9fs_write_inode_dotl, }; +static void v9fs_free_fc(struct fs_context *fc) +{ + struct v9fs_context *ctx = fc->fs_private; + + if (!ctx) + return; + + /* These should be NULL by now but guard against leaks */ + kfree(ctx->session_opts.uname); + kfree(ctx->session_opts.aname); +#ifdef CONFIG_9P_FSCACHE + kfree(ctx->session_opts.cachetag); +#endif + if (ctx->client_opts.trans_mod) + v9fs_put_trans(ctx->client_opts.trans_mod); + kfree(ctx); +} + +static const struct fs_context_operations v9fs_context_ops = { + .parse_param = v9fs_parse_param, + .get_tree = v9fs_get_tree, + .free = v9fs_free_fc, +}; + +static int v9fs_init_fs_context(struct fs_context *fc) +{ + struct v9fs_context *ctx; + + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + + /* initialize core options */ + ctx->session_opts.afid = ~0; + ctx->session_opts.cache = CACHE_NONE; + ctx->session_opts.session_lock_timeout = P9_LOCK_TIMEOUT; + ctx->session_opts.uname = kstrdup(V9FS_DEFUSER, GFP_KERNEL); + if (!ctx->session_opts.uname) + goto error; + + ctx->session_opts.aname = kstrdup(V9FS_DEFANAME, GFP_KERNEL); + if (!ctx->session_opts.aname) + goto error; + + ctx->session_opts.uid = INVALID_UID; + ctx->session_opts.dfltuid = V9FS_DEFUID; + ctx->session_opts.dfltgid = V9FS_DEFGID; + + /* initialize client options */ + ctx->client_opts.proto_version = p9_proto_2000L; + ctx->client_opts.msize = DEFAULT_MSIZE; + + /* initialize fd transport options */ + ctx->fd_opts.port = P9_FD_PORT; + ctx->fd_opts.rfd = ~0; + ctx->fd_opts.wfd = ~0; + ctx->fd_opts.privport = false; + + /* initialize rdma transport options */ + ctx->rdma_opts.port = P9_RDMA_PORT; + ctx->rdma_opts.sq_depth = P9_RDMA_SQ_DEPTH; + ctx->rdma_opts.rq_depth = P9_RDMA_RQ_DEPTH; + ctx->rdma_opts.timeout = P9_RDMA_TIMEOUT; + ctx->rdma_opts.privport = false; + + fc->ops = &v9fs_context_ops; + fc->fs_private = ctx; + + return 0; +error: + fc->need_free = 1; + return -ENOMEM; +} + struct file_system_type v9fs_fs_type = { .name = "9p", - .mount = v9fs_mount, .kill_sb = v9fs_kill_super, .owner = THIS_MODULE, .fs_flags = FS_RENAME_DOES_D_MOVE, + .init_fs_context = v9fs_init_fs_context, + .parameters = v9fs_param_spec, }; MODULE_ALIAS_FS("9p"); diff --git a/include/net/9p/client.h b/include/net/9p/client.h index cc18443f7d51..838a94218b59 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -279,7 +279,7 @@ int p9_client_rename(struct p9_fid *fid, struct p9_fid *newdirfid, const char *name); int p9_client_renameat(struct p9_fid *olddirfid, const char *old_name, struct p9_fid *newdirfid, const char *new_name); -struct p9_client *p9_client_create(const char *dev_name, char *options); +struct p9_client *p9_client_create(struct fs_context *fc); void p9_client_destroy(struct p9_client *clnt); void p9_client_disconnect(struct p9_client *clnt); void p9_client_begin_disconnect(struct p9_client *clnt); diff --git a/include/net/9p/transport.h b/include/net/9p/transport.h index ebbb4b50ee20..e53f312191b6 100644 --- a/include/net/9p/transport.h +++ b/include/net/9p/transport.h @@ -53,7 +53,7 @@ struct p9_trans_module { int def; /* this transport should be default */ struct module *owner; int (*create)(struct p9_client *client, - const char *devname, char *args); + struct fs_context *fc); void (*close)(struct p9_client *client); int (*request)(struct p9_client *client, struct p9_req_t *req); int (*cancel)(struct p9_client *client, struct p9_req_t *req); diff --git a/net/9p/client.c b/net/9p/client.c index 5e3230b1bfab..22b88596e2fe 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -20,8 +20,8 @@ #include <linux/uio.h> #include <linux/netfs.h> #include <net/9p/9p.h> -#include <linux/parser.h> #include <linux/seq_file.h> +#include <linux/fs_context.h> #include <net/9p/client.h> #include <net/9p/transport.h> #include "protocol.h" @@ -33,22 +33,6 @@ * - a little lazy - parse all client options */ -enum { - Opt_msize, - Opt_trans, - Opt_legacy, - Opt_version, - Opt_err, -}; - -static const match_table_t tokens = { - {Opt_msize, "msize=%u"}, - {Opt_legacy, "noextend"}, - {Opt_trans, "trans=%s"}, - {Opt_version, "version=%s"}, - {Opt_err, NULL}, -}; - inline int p9_is_proto_dotl(struct p9_client *clnt) { return clnt->proto_version == p9_proto_2000L; @@ -97,124 +81,16 @@ static int safe_errno(int err) return err; } -/* Interpret mount option for protocol version */ -static int get_protocol_version(char *s) +static int apply_client_options(struct p9_client *clnt, struct fs_context *fc) { - int version = -EINVAL; - - if (!strcmp(s, "9p2000")) { - version = p9_proto_legacy; - p9_debug(P9_DEBUG_9P, "Protocol version: Legacy\n"); - } else if (!strcmp(s, "9p2000.u")) { - version = p9_proto_2000u; - p9_debug(P9_DEBUG_9P, "Protocol version: 9P2000.u\n"); - } else if (!strcmp(s, "9p2000.L")) { - version = p9_proto_2000L; - p9_debug(P9_DEBUG_9P, "Protocol version: 9P2000.L\n"); - } else { - pr_info("Unknown protocol version %s\n", s); - } + struct v9fs_context *ctx = fc->fs_private; - return version; -} - -/** - * parse_opts - parse mount options into client structure - * @opts: options string passed from mount - * @clnt: existing v9fs client information - * - * Return 0 upon success, -ERRNO upon failure - */ - -static int parse_opts(char *opts, struct p9_client *clnt) -{ - char *options, *tmp_options; - char *p; - substring_t args[MAX_OPT_ARGS]; - int option; - char *s; - int ret = 0; - - clnt->proto_version = p9_proto_2000L; - clnt->msize = DEFAULT_MSIZE; - - if (!opts) - return 0; + clnt->msize = ctx->client_opts.msize; + clnt->trans_mod = ctx->client_opts.trans_mod; + ctx->client_opts.trans_mod = NULL; + clnt->proto_version = ctx->client_opts.proto_version; - tmp_options = kstrdup(opts, GFP_KERNEL); - if (!tmp_options) - return -ENOMEM; - options = tmp_options; - - while ((p = strsep(&options, ",")) != NULL) { - int token, r; - - if (!*p) - continue; - token = match_token(p, tokens, args); - switch (token) { - case Opt_msize: - r = match_int(&args[0], &option); - if (r < 0) { - p9_debug(P9_DEBUG_ERROR, - "integer field, but no integer?\n"); - ret = r; - continue; - } - if (option < 4096) { - p9_debug(P9_DEBUG_ERROR, - "msize should be at least 4k\n"); - ret = -EINVAL; - continue; - } - clnt->msize = option; - break; - case Opt_trans: - s = match_strdup(&args[0]); - if (!s) { - ret = -ENOMEM; - p9_debug(P9_DEBUG_ERROR, - "problem allocating copy of trans arg\n"); - goto free_and_return; - } - - v9fs_put_trans(clnt->trans_mod); - clnt->trans_mod = v9fs_get_trans_by_name(s); - if (!clnt->trans_mod) { - pr_info("Could not find request transport: %s\n", - s); - ret = -EINVAL; - } - kfree(s); - break; - case Opt_legacy: - clnt->proto_version = p9_proto_legacy; - break; - case Opt_version: - s = match_strdup(&args[0]); - if (!s) { - ret = -ENOMEM; - p9_debug(P9_DEBUG_ERROR, - "problem allocating copy of version arg\n"); - goto free_and_return; - } - r = get_protocol_version(s); - if (r < 0) - ret = r; - else - clnt->proto_version = r; - kfree(s); - break; - default: - continue; - } - } - -free_and_return: - if (ret) - v9fs_put_trans(clnt->trans_mod); - kfree(tmp_options); - return ret; + return 0; } static int p9_fcall_init(struct p9_client *c, struct p9_fcall *fc, @@ -968,7 +844,7 @@ static int p9_client_version(struct p9_client *c) return err; } -struct p9_client *p9_client_create(const char *dev_name, char *options) +struct p9_client *p9_client_create(struct fs_context *fc) { int err; static atomic_t seqno = ATOMIC_INIT(0); @@ -991,8 +867,8 @@ struct p9_client *p9_client_create(const char *dev_name, char *options) idr_init(&clnt->fids); idr_init(&clnt->reqs); - err = parse_opts(options, clnt); - if (err < 0) + err = apply_client_options(clnt, fc); + if (err) goto free_client; if (!clnt->trans_mod) @@ -1008,7 +884,7 @@ struct p9_client *p9_client_create(const char *dev_name, char *options) p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n", clnt, clnt->trans_mod, clnt->msize, clnt->proto_version); - err = clnt->trans_mod->create(clnt, dev_name, options); + err = clnt->trans_mod->create(clnt, fc); if (err) goto put_trans; diff --git a/net/9p/mod.c b/net/9p/mod.c index 55576c1866fa..85160b52da55 100644 --- a/net/9p/mod.c +++ b/net/9p/mod.c @@ -16,7 +16,6 @@ #include <linux/moduleparam.h> #include <net/9p/9p.h> #include <linux/fs.h> -#include <linux/parser.h> #include <net/9p/client.h> #include <net/9p/transport.h> #include <linux/list.h> @@ -171,6 +170,7 @@ void v9fs_put_trans(struct p9_trans_module *m) if (m) module_put(m->owner); } +EXPORT_SYMBOL(v9fs_put_trans); /** * init_p9 - Initialize module diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 9ef4f2e0db3c..cb4398d79b0a 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -22,7 +22,7 @@ #include <linux/uaccess.h> #include <linux/inet.h> #include <linux/file.h> -#include <linux/parser.h> +#include <linux/fs_context.h> #include <linux/slab.h> #include <linux/seq_file.h> #include <net/9p/9p.h> @@ -37,26 +37,6 @@ static struct p9_trans_module p9_tcp_trans; static struct p9_trans_module p9_fd_trans; -/* - * Option Parsing (code inspired by NFS code) - * - a little lazy - parse all fd-transport options - */ - -enum { - /* Options that take integer arguments */ - Opt_port, Opt_rfdno, Opt_wfdno, Opt_err, - /* Options that take no arguments */ - Opt_privport, -}; - -static const match_table_t tokens = { - {Opt_port, "port=%u"}, - {Opt_rfdno, "rfdno=%u"}, - {Opt_wfdno, "wfdno=%u"}, - {Opt_privport, "privport"}, - {Opt_err, NULL}, -}; - enum { Rworksched = 1, /* read work scheduled or running */ Rpending = 2, /* can read */ @@ -744,73 +724,6 @@ static int p9_fd_show_options(struct seq_file *m, struct p9_client *clnt) return 0; } -/** - * parse_opts - parse mount options into p9_fd_opts structure - * @params: options string passed from mount - * @opts: fd transport-specific structure to parse options into - * - * Returns 0 upon success, -ERRNO upon failure - */ - -static int parse_opts(char *params, struct p9_fd_opts *opts) -{ - char *p; - substring_t args[MAX_OPT_ARGS]; - int option; - char *options, *tmp_options; - - opts->port = P9_FD_PORT; - opts->rfd = ~0; - opts->wfd = ~0; - opts->privport = false; - - if (!params) - return 0; - - tmp_options = kstrdup(params, GFP_KERNEL); - if (!tmp_options) { - p9_debug(P9_DEBUG_ERROR, - "failed to allocate copy of option string\n"); - return -ENOMEM; - } - options = tmp_options; - - while ((p = strsep(&options, ",")) != NULL) { - int token; - int r; - if (!*p) - continue; - token = match_token(p, tokens, args); - if ((token != Opt_err) && (token != Opt_privport)) { - r = match_int(&args[0], &option); - if (r < 0) { - p9_debug(P9_DEBUG_ERROR, - "integer field, but no integer?\n"); - continue; - } - } - switch (token) { - case Opt_port: - opts->port = option; - break; - case Opt_rfdno: - opts->rfd = option; - break; - case Opt_wfdno: - opts->wfd = option; - break; - case Opt_privport: - opts->privport = true; - break; - default: - continue; - } - } - - kfree(tmp_options); - return 0; -} - static int p9_fd_open(struct p9_client *client, int rfd, int wfd) { struct p9_trans_fd *ts = kzalloc(sizeof(struct p9_trans_fd), @@ -965,17 +878,18 @@ static int p9_bind_privport(struct socket *sock) } static int -p9_fd_create_tcp(struct p9_client *client, const char *addr, char *args) +p9_fd_create_tcp(struct p9_client *client, struct fs_context *fc) { + const char *addr = fc->source; + struct v9fs_context *ctx = fc->fs_private; int err; char port_str[6]; struct socket *csocket; struct sockaddr_storage stor = { 0 }; struct p9_fd_opts opts; - err = parse_opts(args, &opts); - if (err < 0) - return err; + /* opts are already parsed in context */ + opts = ctx->fd_opts; if (!addr) return -EINVAL; @@ -1022,8 +936,9 @@ p9_fd_create_tcp(struct p9_client *client, const char *addr, char *args) } static int -p9_fd_create_unix(struct p9_client *client, const char *addr, char *args) +p9_fd_create_unix(struct p9_client *client, struct fs_context *fc) { + const char *addr = fc->source; int err; struct socket *csocket; struct sockaddr_un sun_server; @@ -1062,14 +977,12 @@ p9_fd_create_unix(struct p9_client *client, const char *addr, char *args) } static int -p9_fd_create(struct p9_client *client, const char *addr, char *args) +p9_fd_create(struct p9_client *client, struct fs_context *fc) { + struct v9fs_context *ctx = fc->fs_private; + struct p9_fd_opts opts = ctx->fd_opts; int err; - struct p9_fd_opts opts; - err = parse_opts(args, &opts); - if (err < 0) - return err; client->trans_opts.fd.rfd = opts.rfd; client->trans_opts.fd.wfd = opts.wfd; diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c index 46ee37061faf..fa3365990fdf 100644 --- a/net/9p/trans_rdma.c +++ b/net/9p/trans_rdma.c @@ -22,7 +22,7 @@ #include <linux/uaccess.h> #include <linux/inet.h> #include <linux/file.h> -#include <linux/parser.h> +#include <linux/fs_context.h> #include <linux/semaphore.h> #include <linux/slab.h> #include <linux/seq_file.h> @@ -106,26 +106,6 @@ struct p9_rdma_context { }; }; -/* - * Option Parsing (code inspired by NFS code) - */ -enum { - /* Options that take integer arguments */ - Opt_port, Opt_rq_depth, Opt_sq_depth, Opt_timeout, - /* Options that take no argument */ - Opt_privport, - Opt_err, -}; - -static match_table_t tokens = { - {Opt_port, "port=%u"}, - {Opt_sq_depth, "sq=%u"}, - {Opt_rq_depth, "rq=%u"}, - {Opt_timeout, "timeout=%u"}, - {Opt_privport, "privport"}, - {Opt_err, NULL}, -}; - static int p9_rdma_show_options(struct seq_file *m, struct p9_client *clnt) { struct p9_trans_rdma *rdma = clnt->trans; @@ -143,77 +123,6 @@ static int p9_rdma_show_options(struct seq_file *m, struct p9_client *clnt) return 0; } -/** - * parse_opts - parse mount options into rdma options structure - * @params: options string passed from mount - * @opts: rdma transport-specific structure to parse options into - * - * Returns 0 upon success, -ERRNO upon failure - */ -static int parse_opts(char *params, struct p9_rdma_opts *opts) -{ - char *p; - substring_t args[MAX_OPT_ARGS]; - int option; - char *options, *tmp_options; - - opts->port = P9_RDMA_PORT; - opts->sq_depth = P9_RDMA_SQ_DEPTH; - opts->rq_depth = P9_RDMA_RQ_DEPTH; - opts->timeout = P9_RDMA_TIMEOUT; - opts->privport = false; - - if (!params) - return 0; - - tmp_options = kstrdup(params, GFP_KERNEL); - if (!tmp_options) { - p9_debug(P9_DEBUG_ERROR, - "failed to allocate copy of option string\n"); - return -ENOMEM; - } - options = tmp_options; - - while ((p = strsep(&options, ",")) != NULL) { - int token; - int r; - if (!*p) - continue; - token = match_token(p, tokens, args); - if ((token != Opt_err) && (token != Opt_privport)) { - r = match_int(&args[0], &option); - if (r < 0) { - p9_debug(P9_DEBUG_ERROR, - "integer field, but no integer?\n"); - continue; - } - } - switch (token) { - case Opt_port: - opts->port = option; - break; - case Opt_sq_depth: - opts->sq_depth = option; - break; - case Opt_rq_depth: - opts->rq_depth = option; - break; - case Opt_timeout: - opts->timeout = option; - break; - case Opt_privport: - opts->privport = true; - break; - default: - continue; - } - } - /* RQ must be at least as large as the SQ */ - opts->rq_depth = max(opts->rq_depth, opts->sq_depth); - kfree(tmp_options); - return 0; -} - static int p9_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event) { @@ -607,14 +516,15 @@ static int p9_rdma_bind_privport(struct p9_trans_rdma *rdma) /** * rdma_create_trans - Transport method for creating a transport instance * @client: client instance - * @addr: IP address string - * @args: Mount options string + * @fc: The filesystem context */ static int -rdma_create_trans(struct p9_client *client, const char *addr, char *args) +rdma_create_trans(struct p9_client *client, struct fs_context *fc) { + const char *addr = fc->source; + struct v9fs_context *ctx = fc->fs_private; + struct p9_rdma_opts opts = ctx->rdma_opts; int err; - struct p9_rdma_opts opts; struct p9_trans_rdma *rdma; struct rdma_conn_param conn_param; struct ib_qp_init_attr qp_attr; @@ -622,10 +532,8 @@ rdma_create_trans(struct p9_client *client, const char *addr, char *args) if (addr == NULL) return -EINVAL; - /* Parse the transport specific mount options */ - err = parse_opts(args, &opts); - if (err < 0) - return err; + /* options are already parsed, in the fs context */ + opts = ctx->rdma_opts; /* Create and initialize the RDMA transport structure */ rdma = alloc_rdma(&opts); diff --git a/net/9p/trans_usbg.c b/net/9p/trans_usbg.c index 6b694f117aef..61197dae52cb 100644 --- a/net/9p/trans_usbg.c +++ b/net/9p/trans_usbg.c @@ -27,6 +27,7 @@ #include <linux/cleanup.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/fs_context.h> #include <linux/usb/composite.h> #include <linux/usb/func_utils.h> @@ -366,8 +367,9 @@ enable_usb9pfs(struct usb_composite_dev *cdev, struct f_usb9pfs *usb9pfs) return ret; } -static int p9_usbg_create(struct p9_client *client, const char *devname, char *args) +static int p9_usbg_create(struct p9_client *client, struct fs_context *fc) { + const char *devname = fc->source; struct f_usb9pfs_dev *dev; struct f_usb9pfs *usb9pfs; int ret = -ENOENT; diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index 0b8086f58ad5..580d74ca92b0 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -26,7 +26,7 @@ #include <linux/highmem.h> #include <linux/slab.h> #include <net/9p/9p.h> -#include <linux/parser.h> +#include <linux/fs_context.h> #include <net/9p/client.h> #include <net/9p/transport.h> #include <linux/scatterlist.h> @@ -679,8 +679,7 @@ static int p9_virtio_probe(struct virtio_device *vdev) /** * p9_virtio_create - allocate a new virtio channel * @client: client instance invoking this transport - * @devname: string identifying the channel to connect to (unused) - * @args: args passed from sys_mount() for per-transport options (unused) + * @fc: the filesystem context * * This sets up a transport channel for 9p communication. Right now * we only match the first available channel, but eventually we could look up @@ -691,8 +690,9 @@ static int p9_virtio_probe(struct virtio_device *vdev) */ static int -p9_virtio_create(struct p9_client *client, const char *devname, char *args) +p9_virtio_create(struct p9_client *client, struct fs_context *fc) { + const char *devname = fc->source; struct virtio_chan *chan; int ret = -ENOENT; int found = 0; diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c index b9ff69c7522a..091a99eb51f4 100644 --- a/net/9p/trans_xen.c +++ b/net/9p/trans_xen.c @@ -15,6 +15,7 @@ #include <linux/module.h> #include <linux/spinlock.h> +#include <linux/fs_context.h> #include <net/9p/9p.h> #include <net/9p/client.h> #include <net/9p/transport.h> @@ -66,8 +67,9 @@ static int p9_xen_cancel(struct p9_client *client, struct p9_req_t *req) return 1; } -static int p9_xen_create(struct p9_client *client, const char *addr, char *args) +static int p9_xen_create(struct p9_client *client, struct fs_context *fc) { + const char *addr = fc->source; struct xen_9pfs_front_priv *priv; if (addr == NULL) -- 2.51.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-10-10 21:36 ` [PATCH V3 4/4] 9p: convert to the new mount API Eric Sandeen @ 2025-10-13 10:26 ` Dominique Martinet 2025-10-13 18:46 ` Eric Sandeen 2025-11-26 20:16 ` Remi Pommarel 2025-12-02 22:30 ` [PATCH V3 5/4] 9p: fix cache option printing in v9fs_show_options Eric Sandeen 2025-12-02 22:34 ` [PATCH V3 6/4] 9p: fix new mount API cache option handling Eric Sandeen 2 siblings, 2 replies; 23+ messages in thread From: Dominique Martinet @ 2025-10-13 10:26 UTC (permalink / raw) To: Eric Sandeen Cc: v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis 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 ----- I'll try to find some time to test a mix of actual mount options later this week Cheers, -- Dominique Martinet | Asmadeus ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-10-13 10:26 ` Dominique Martinet @ 2025-10-13 18:46 ` Eric Sandeen 2025-10-13 19:04 ` Dominique Martinet 2025-11-26 20:16 ` Remi Pommarel 1 sibling, 1 reply; 23+ messages in thread From: Eric Sandeen @ 2025-10-13 18:46 UTC (permalink / raw) To: Dominique Martinet, Eric Sandeen Cc: v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis On 10/13/25 5:26 AM, Dominique Martinet wrote: > Hi Eric, > > Thanks for this V3! > > I find it much cleaner, hopefully will be easier to debug :) Good news and bad news, I see. > ... 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) I swear I tested this, but you are right, and it fails for me too. Oh ... I know what this is :( Introducing the "ignore unknown mount options" change in V4 caused it to also ignore the unknown "source" option and report success; this made the vfs think "source" was already handled in vfs_parse_fs_param() and therefore it does not call vfs_parse_fs_param_source(). This has bitten me before and it's a bit confusing. I'm not sure how I missed this in my V4 testing, I'm very sorry. > 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?... Not all of them; filesystems that reject unknown options have "source" handled for them in the VFS, but for filesystems like debugfs that ignore unknown parameters it had to handle it explicitly. (Other filesystems may do so for other reasons I suppose). See also a20971c18752 which fixed a20971c18752, though the bug had slightly less of an impact. > 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; Yep, this looks correct, I think. It essentially "steals" the string from the param and sets it in fc->source since the VFS won't do it for us. I can't help but feel like there's maybe a better treewide fix for this to make it all a bit less opaque, but for now this is what other filesystems do, and so I think this is the right fix for my series at this point. Would you like me to send an updated patch with this change, or will you just fix it on your end? Thanks, -Eric > + break; > case Opt_debug: > session_opts->debug = result.uint_32; > #ifdef CONFIG_NET_9P_DEBUG > ----- > > I'll try to find some time to test a mix of actual mount options later > this week > > Cheers, ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-10-13 18:46 ` Eric Sandeen @ 2025-10-13 19:04 ` Dominique Martinet 0 siblings, 0 replies; 23+ messages in thread From: Dominique Martinet @ 2025-10-13 19:04 UTC (permalink / raw) To: Eric Sandeen Cc: Eric Sandeen, v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis Eric Sandeen wrote on Mon, Oct 13, 2025 at 01:46:42PM -0500: > > ... 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) > > I swear I tested this, but you are right, and it fails for me too. > > Oh ... I know what this is :( > > Introducing the "ignore unknown mount options" change in V4 caused it to > also ignore the unknown "source" option and report success; this made the > vfs think "source" was already handled in vfs_parse_fs_param() and > therefore it does not call vfs_parse_fs_param_source(). This has bitten > me before and it's a bit confusing. > > I'm not sure how I missed this in my V4 testing, I'm very sorry. No harm done :) And thanks for the explanation, the vfs parsing being done only if the fs-specific parsing failed was far from obvious for me! > > 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?... > > Not all of them; filesystems that reject unknown options have "source" > handled for them in the VFS, but for filesystems like debugfs that > ignore unknown parameters it had to handle it explicitly. (Other > filesystems may do so for other reasons I suppose). > > See also a20971c18752 which fixed a20971c18752, though the bug had > slightly less of an impact. (I assume the former was 3a987b88a425) > Yep, this looks correct, I think. It essentially "steals" the string from > the param and sets it in fc->source since the VFS won't do it for us. Yes, I copied that from nfs and it looks like debugfs does the same. > I can't help but feel like there's maybe a better treewide fix for this > to make it all a bit less opaque, but for now this is what other > filesystems do, and so I think this is the right fix for my series at > this point. Not much better given it does the work twice but we could return -EINVAL in the case Opt_source statement to optimize for code size... I'm not sure that's quite clearer though, I'll stick to "doing what everyone else" does for now and we/someone can revisit this later. > Would you like me to send an updated patch with this change, or will you > just fix it on your end? Happy to roll it in directly, I'll reply again when I find time to finish testing and push to next. -- Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-10-13 10:26 ` Dominique Martinet 2025-10-13 18:46 ` Eric Sandeen @ 2025-11-26 20:16 ` Remi Pommarel 2025-11-26 22:43 ` Dominique Martinet 1 sibling, 1 reply; 23+ messages in thread From: Remi Pommarel @ 2025-11-26 20:16 UTC (permalink / raw) To: Dominique Martinet Cc: Eric Sandeen, v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-11-26 20:16 ` Remi Pommarel @ 2025-11-26 22:43 ` Dominique Martinet 2025-12-01 22:36 ` Eric Sandeen 0 siblings, 1 reply; 23+ messages in thread From: Dominique Martinet @ 2025-11-26 22:43 UTC (permalink / raw) To: Remi Pommarel Cc: Eric Sandeen, v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis 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. 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? 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! -- Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-11-26 22:43 ` Dominique Martinet @ 2025-12-01 22:36 ` Eric Sandeen 2025-12-02 1:04 ` Dominique Martinet 0 siblings, 1 reply; 23+ messages in thread From: Eric Sandeen @ 2025-12-01 22:36 UTC (permalink / raw) To: Dominique Martinet, Remi Pommarel Cc: v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis 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! ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-12-01 22:36 ` Eric Sandeen @ 2025-12-02 1:04 ` Dominique Martinet 2025-12-02 22:12 ` Eric Sandeen 0 siblings, 1 reply; 23+ messages in thread From: Dominique Martinet @ 2025-12-02 1:04 UTC (permalink / raw) To: Eric Sandeen Cc: Remi Pommarel, v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis Eric Sandeen wrote on Mon, Dec 01, 2025 at 04:36:58PM -0600: > I suppose it would be a terrible hack to just extend the enum to include > hexadecimal "strings" like this, right.... ;) Yeah, that might work for all intent and purposes but we'll get someone who mounted with cache=0x3 next... :) > 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. This sounds good to me! > 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? No strong feeling either way but I think a 5/4 would be better to clarify why we do this -- I could probably do it as well but I'd definietly appreciate if you could do it (and I'll just have to make time to test at the end!) > 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. Thanks for checking as well! -- Dominique ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-12-02 1:04 ` Dominique Martinet @ 2025-12-02 22:12 ` Eric Sandeen 2025-12-03 15:13 ` Dominique Martinet 0 siblings, 1 reply; 23+ messages in thread From: Eric Sandeen @ 2025-12-02 22:12 UTC (permalink / raw) To: Dominique Martinet Cc: Remi Pommarel, v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis On 12/1/25 7:04 PM, Dominique Martinet wrote: > Eric Sandeen wrote on Mon, Dec 01, 2025 at 04:36:58PM -0600: >> I suppose it would be a terrible hack to just extend the enum to include >> hexadecimal "strings" like this, right.... ;) > > Yeah, that might work for all intent and purposes but we'll get someone > who mounted with cache=0x3 next... :) > >> 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. > > This sounds good to me! > >> 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? > > No strong feeling either way but I think a 5/4 would be better to > clarify why we do this -- I could probably do it as well but I'd > definietly appreciate if you could do it (and I'll just have to make > time to test at the end!) Working on this, but something that confuses me about the current (not for-next) code: If I mount with "cache=loose" I see this in /proc/mounts: 127.0.0.1 /mnt 9p rw,relatime,uname=fsgqa,aname=/tmp/9,cache=f,access=user,trans=tcp 0 0 note the "cache=f" thanks to show_options printing "cache=%x" "mount -o cache=f" is rejected, though, because "f" is not a parseable number. Shouldn't it be printing "cache=0xf" instead of "cache=f?" (for some reason, though, in my test "remount -o,ro" does still work even with "cache=f" in /proc/mounts but that seems to be a side effect of mount.9p trying to use the new mount API when it shouldn't, or ...???) I'll send my fix-up patch with a (maybe?) extra bugfix of printing "cache=0x%x" in show_options, and you can see what you think... it could be moved into a pure bugfix patch first if you agree. thanks, -Eric ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-12-02 22:12 ` Eric Sandeen @ 2025-12-03 15:13 ` Dominique Martinet 2025-12-03 16:23 ` Eric Sandeen 2025-12-05 11:53 ` Remi Pommarel 0 siblings, 2 replies; 23+ messages in thread From: Dominique Martinet @ 2025-12-03 15:13 UTC (permalink / raw) To: Eric Sandeen Cc: Remi Pommarel, v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis Eric Sandeen wrote on Tue, Dec 02, 2025 at 04:12:36PM -0600: > Working on this, but something that confuses me about the current > (not for-next) code: > > If I mount with "cache=loose" I see this in /proc/mounts: > > 127.0.0.1 /mnt 9p rw,relatime,uname=fsgqa,aname=/tmp/9,cache=f,access=user,trans=tcp 0 0 > > note the "cache=f" thanks to show_options printing "cache=%x" > > "mount -o cache=f" is rejected, though, because "f" is not a parseable > number. > > Shouldn't it be printing "cache=0xf" instead of "cache=f?" Definitely should be! > (for some reason, though, in my test "remount -o,ro" does still work even with > "cache=f" in /proc/mounts but that seems to be a side effect of mount.9p trying > to use the new mount API when it shouldn't, or ...???) ... and Remi explicitly had cache=loose in his command line, so I'm also surprised it worked... > I'll send my fix-up patch with a (maybe?) extra bugfix of printing > "cache=0x%x" in show_options, and you can see what you think... it could > be moved into a pure bugfix patch first if you agree. Thank you! I would have been happy to see both together but it does make more sense separately, I've just tested and pushed both your patches to -next I also agree the other show_options look safe enough as they either print a string or int. . . . Ah, actually I spotted another one: if (v9ses->debug) seq_printf(m, ",debug=%x", v9ses->debug); This needs to be prefixed by 0x as well -- Eric, do you mind if I amend your patch 5 with that as well? Remi - I did check rootfstype=9p as well and all seems fine but I'd appreciate if you could test as well Thanks! -- Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-12-03 15:13 ` Dominique Martinet @ 2025-12-03 16:23 ` Eric Sandeen 2025-12-05 11:53 ` Remi Pommarel 1 sibling, 0 replies; 23+ messages in thread From: Eric Sandeen @ 2025-12-03 16:23 UTC (permalink / raw) To: Dominique Martinet, Eric Sandeen Cc: Remi Pommarel, v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis On 12/3/25 9:13 AM, Dominique Martinet wrote: > Eric Sandeen wrote on Tue, Dec 02, 2025 at 04:12:36PM -0600: ... > I also agree the other show_options look safe enough as they either > print a string or int. . . . > Ah, actually I spotted another one: > if (v9ses->debug) > seq_printf(m, ",debug=%x", v9ses->debug); > This needs to be prefixed by 0x as well -- Eric, do you mind if I amend > your patch 5 with that as well? Doh ... sure, please go ahead and fix it however you'd like. -Eric > > Remi - I did check rootfstype=9p as well and all seems fine but I'd > appreciate if you could test as well > > > Thanks! ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-12-03 15:13 ` Dominique Martinet 2025-12-03 16:23 ` Eric Sandeen @ 2025-12-05 11:53 ` Remi Pommarel 2025-12-05 12:56 ` Dominique Martinet 1 sibling, 1 reply; 23+ messages in thread From: Remi Pommarel @ 2025-12-05 11:53 UTC (permalink / raw) To: Dominique Martinet Cc: Eric Sandeen, v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis On Thu, Dec 04, 2025 at 12:13:33AM +0900, Dominique Martinet wrote: > Eric Sandeen wrote on Tue, Dec 02, 2025 at 04:12:36PM -0600: > > Working on this, but something that confuses me about the current > > (not for-next) code: > > > > If I mount with "cache=loose" I see this in /proc/mounts: > > > > 127.0.0.1 /mnt 9p rw,relatime,uname=fsgqa,aname=/tmp/9,cache=f,access=user,trans=tcp 0 0 > > > > note the "cache=f" thanks to show_options printing "cache=%x" > > > > "mount -o cache=f" is rejected, though, because "f" is not a parseable > > number. > > > > Shouldn't it be printing "cache=0xf" instead of "cache=f?" > > Definitely should be! > > > (for some reason, though, in my test "remount -o,ro" does still work even with > > "cache=f" in /proc/mounts but that seems to be a side effect of mount.9p trying > > to use the new mount API when it shouldn't, or ...???) > > ... and Remi explicitly had cache=loose in his command line, so I'm also > surprised it worked... > > > I'll send my fix-up patch with a (maybe?) extra bugfix of printing > > "cache=0x%x" in show_options, and you can see what you think... it could > > be moved into a pure bugfix patch first if you agree. > > Thank you! I would have been happy to see both together but it does make > more sense separately, I've just tested and pushed both your patches to > -next > > > I also agree the other show_options look safe enough as they either > print a string or int. . . . > Ah, actually I spotted another one: > if (v9ses->debug) > seq_printf(m, ",debug=%x", v9ses->debug); > This needs to be prefixed by 0x as well -- Eric, do you mind if I amend > your patch 5 with that as well? > > > Remi - I did check rootfstype=9p as well and all seems fine but I'd > appreciate if you could test as well I just tried your 9p-next branch and the issue is gone for rootfstype=9p using cache=loose (I've also made sure that I reproduce the issue without the last two commits of your branch). So yes for me that fixes it, thanks for the patches. -- Remi ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 4/4] 9p: convert to the new mount API 2025-12-05 11:53 ` Remi Pommarel @ 2025-12-05 12:56 ` Dominique Martinet 0 siblings, 0 replies; 23+ messages in thread From: Dominique Martinet @ 2025-12-05 12:56 UTC (permalink / raw) To: Remi Pommarel Cc: Eric Sandeen, v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis Remi Pommarel wrote on Fri, Dec 05, 2025 at 12:53:04PM +0100: > > Remi - I did check rootfstype=9p as well and all seems fine but I'd > > appreciate if you could test as well > > I just tried your 9p-next branch and the issue is gone for rootfstype=9p > using cache=loose (I've also made sure that I reproduce the issue without > the last two commits of your branch). > > So yes for me that fixes it, thanks for the patches. Thanks for testing! I've added your Tested-by to both patches. And I'll submit this all to Linus on Sunday, hopefully won't get much more breakage... :) -- Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH V3 5/4] 9p: fix cache option printing in v9fs_show_options 2025-10-10 21:36 ` [PATCH V3 4/4] 9p: convert to the new mount API Eric Sandeen 2025-10-13 10:26 ` Dominique Martinet @ 2025-12-02 22:30 ` Eric Sandeen 2025-12-02 23:13 ` Al Viro 2025-12-02 22:34 ` [PATCH V3 6/4] 9p: fix new mount API cache option handling Eric Sandeen 2 siblings, 1 reply; 23+ messages in thread From: Eric Sandeen @ 2025-12-02 22:30 UTC (permalink / raw) To: v9fs Cc: linux-fsdevel, linux-kernel, ericvh, lucho, asmadeus, linux_oss, eadavis, Remi Pommarel commit 4eb3117888a92 changed the cache= option to accept either string shortcuts or bitfield values. It also changed /proc/mounts to emit the option as the hexadecimal numeric value rather than the shortcut string. However, by printing "cache=%x" without the leading 0x, shortcuts such as "cache=loose" will emit "cache=f" and 'f' is not a string that is parseable by kstrtoint(), so remounting may fail if a remount with "cache=f" is attempted. Fix this by adding the 0x prefix to the hexadecimal value shown in /proc/mounts. Fixes: 4eb3117888a92 ("fs/9p: Rework cache modes and add new options to Documentation") Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c index 05fc2ba3c5d4..d684cb406ed6 100644 --- a/fs/9p/v9fs.c +++ b/fs/9p/v9fs.c @@ -148,7 +148,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=0x%x", v9ses->cache); #ifdef CONFIG_9P_FSCACHE if (v9ses->cachetag && (v9ses->cache & CACHE_FSCACHE)) seq_printf(m, ",cachetag=%s", v9ses->cachetag); ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH V3 5/4] 9p: fix cache option printing in v9fs_show_options 2025-12-02 22:30 ` [PATCH V3 5/4] 9p: fix cache option printing in v9fs_show_options Eric Sandeen @ 2025-12-02 23:13 ` Al Viro 2025-12-03 1:09 ` Eric Sandeen 0 siblings, 1 reply; 23+ messages in thread From: Al Viro @ 2025-12-02 23:13 UTC (permalink / raw) To: Eric Sandeen Cc: v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, asmadeus, linux_oss, eadavis, Remi Pommarel On Tue, Dec 02, 2025 at 04:30:53PM -0600, Eric Sandeen wrote: > commit 4eb3117888a92 changed the cache= option to accept either string > shortcuts or bitfield values. It also changed /proc/mounts to emit the > option as the hexadecimal numeric value rather than the shortcut string. > > However, by printing "cache=%x" without the leading 0x, shortcuts such > as "cache=loose" will emit "cache=f" and 'f' is not a string that is > parseable by kstrtoint(), so remounting may fail if a remount with > "cache=f" is attempted. > > Fix this by adding the 0x prefix to the hexadecimal value shown in > /proc/mounts. > > Fixes: 4eb3117888a92 ("fs/9p: Rework cache modes and add new options to Documentation") > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c > index 05fc2ba3c5d4..d684cb406ed6 100644 > --- a/fs/9p/v9fs.c > +++ b/fs/9p/v9fs.c > @@ -148,7 +148,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=0x%x", v9ses->cache); What's wrong with "cache=%#x"? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 5/4] 9p: fix cache option printing in v9fs_show_options 2025-12-02 23:13 ` Al Viro @ 2025-12-03 1:09 ` Eric Sandeen 2025-12-03 15:04 ` Dominique Martinet 0 siblings, 1 reply; 23+ messages in thread From: Eric Sandeen @ 2025-12-03 1:09 UTC (permalink / raw) To: Al Viro Cc: v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, asmadeus, linux_oss, eadavis, Remi Pommarel On 12/2/25 5:13 PM, Al Viro wrote: > On Tue, Dec 02, 2025 at 04:30:53PM -0600, Eric Sandeen wrote: >> commit 4eb3117888a92 changed the cache= option to accept either string >> shortcuts or bitfield values. It also changed /proc/mounts to emit the >> option as the hexadecimal numeric value rather than the shortcut string. >> >> However, by printing "cache=%x" without the leading 0x, shortcuts such >> as "cache=loose" will emit "cache=f" and 'f' is not a string that is >> parseable by kstrtoint(), so remounting may fail if a remount with >> "cache=f" is attempted. >> >> Fix this by adding the 0x prefix to the hexadecimal value shown in >> /proc/mounts. >> >> Fixes: 4eb3117888a92 ("fs/9p: Rework cache modes and add new options to Documentation") >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c >> index 05fc2ba3c5d4..d684cb406ed6 100644 >> --- a/fs/9p/v9fs.c >> +++ b/fs/9p/v9fs.c >> @@ -148,7 +148,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=0x%x", v9ses->cache); > > What's wrong with "cache=%#x"? > Nothing, presumably - I did not know this existed TBH. (looks like that usage is about 1/10 of 0x%x currently) -Eric ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 5/4] 9p: fix cache option printing in v9fs_show_options 2025-12-03 1:09 ` Eric Sandeen @ 2025-12-03 15:04 ` Dominique Martinet 2025-12-03 18:04 ` Al Viro 0 siblings, 1 reply; 23+ messages in thread From: Dominique Martinet @ 2025-12-03 15:04 UTC (permalink / raw) To: Eric Sandeen Cc: Al Viro, v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis, Remi Pommarel Eric Sandeen wrote on Tue, Dec 02, 2025 at 07:09:42PM -0600: > >> - seq_printf(m, ",cache=%x", v9ses->cache); > >> + seq_printf(m, ",cache=0x%x", v9ses->cache); > > > > What's wrong with "cache=%#x"? > > Nothing, presumably - I did not know this existed TBH. > > (looks like that usage is about 1/10 of 0x%x currently) I don't have any preference here, but I've folded in %#x when applying because why not -- I've been seeing it slightly more often lately so I guess it's the "modern way" of doing this. (I got curious and this SPECIAL flag in lib/vsprintf.c has been around since at least the first 2.6.12-rc2 git commit, so there's nothing new about it and I suspect it'll never quite be popular...) -- Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH V3 5/4] 9p: fix cache option printing in v9fs_show_options 2025-12-03 15:04 ` Dominique Martinet @ 2025-12-03 18:04 ` Al Viro 0 siblings, 0 replies; 23+ messages in thread From: Al Viro @ 2025-12-03 18:04 UTC (permalink / raw) To: Dominique Martinet Cc: Eric Sandeen, v9fs, linux-fsdevel, linux-kernel, ericvh, lucho, linux_oss, eadavis, Remi Pommarel On Thu, Dec 04, 2025 at 12:04:21AM +0900, Dominique Martinet wrote: > Eric Sandeen wrote on Tue, Dec 02, 2025 at 07:09:42PM -0600: > > >> - seq_printf(m, ",cache=%x", v9ses->cache); > > >> + seq_printf(m, ",cache=0x%x", v9ses->cache); > > > > > > What's wrong with "cache=%#x"? > > > > Nothing, presumably - I did not know this existed TBH. > > > > (looks like that usage is about 1/10 of 0x%x currently) > > I don't have any preference here, but I've folded in %#x when applying > because why not -- I've been seeing it slightly more often lately so I > guess it's the "modern way" of doing this. In 4BSD libc by October 1980, part of ANSI C variants all way back to C89. Covered in K&R 2nd edition ('88); 3BSD didn't have it, neither did v7, so I'd guess that it was done at some point in 1980, possibly late 1979. So it's hardly something newfangled... ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH V3 6/4] 9p: fix new mount API cache option handling 2025-10-10 21:36 ` [PATCH V3 4/4] 9p: convert to the new mount API Eric Sandeen 2025-10-13 10:26 ` Dominique Martinet 2025-12-02 22:30 ` [PATCH V3 5/4] 9p: fix cache option printing in v9fs_show_options Eric Sandeen @ 2025-12-02 22:34 ` Eric Sandeen 2 siblings, 0 replies; 23+ messages in thread From: Eric Sandeen @ 2025-12-02 22:34 UTC (permalink / raw) To: v9fs Cc: linux-fsdevel, linux-kernel, ericvh, lucho, asmadeus, linux_oss, eadavis, Remi Pommarel After commit 4eb3117888a92, 9p needs to be able to accept numerical cache= mount options as well as the string "shortcuts" because the option is printed numerically in /proc/mounts rather than by string. This was missed in the mount API conversion, which used an enum for the shortcuts and therefore could not handle a numeric equivalent as an argument to the cache option. Fix this by removing the enum and reverting to the slightly more open-coded option handling for Opt_cache, with the reinstated get_cache_mode() helper. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- If you wanted to fold this into my original mount api conversion patch so there's no regression point, that would be fine with me as well, of course. Sorry for the error. diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c index d684cb406ed6..dea2c5347933 100644 --- a/fs/9p/v9fs.c +++ b/fs/9p/v9fs.c @@ -72,15 +72,6 @@ static const struct constant_table p9_versions[] = { {} }; -static const struct constant_table p9_cache_mode[] = { - { "loose", CACHE_SC_LOOSE }, - { "fscache", CACHE_SC_FSCACHE }, - { "mmap", CACHE_SC_MMAP }, - { "readahead", CACHE_SC_READAHEAD }, - { "none", CACHE_SC_NONE }, - {} -}; - /* * This structure contains all parameters used for the core code, * the client, and all the transports. @@ -97,7 +88,7 @@ const struct fs_parameter_spec v9fs_param_spec[] = { fsparam_flag ("noxattr", Opt_noxattr), fsparam_flag ("directio", Opt_directio), fsparam_flag ("ignoreqv", Opt_ignoreqv), - fsparam_enum ("cache", Opt_cache, p9_cache_mode), + fsparam_string ("cache", Opt_cache), fsparam_string ("cachetag", Opt_cachetag), fsparam_string ("access", Opt_access), fsparam_flag ("posixacl", Opt_posixacl), @@ -124,6 +115,33 @@ const struct fs_parameter_spec v9fs_param_spec[] = { {} }; +/* Interpret mount options for cache mode */ +static int get_cache_mode(char *s) +{ + int version = -EINVAL; + + if (!strcmp(s, "loose")) { + version = CACHE_SC_LOOSE; + p9_debug(P9_DEBUG_9P, "Cache mode: loose\n"); + } else if (!strcmp(s, "fscache")) { + version = CACHE_SC_FSCACHE; + p9_debug(P9_DEBUG_9P, "Cache mode: fscache\n"); + } else if (!strcmp(s, "mmap")) { + version = CACHE_SC_MMAP; + p9_debug(P9_DEBUG_9P, "Cache mode: mmap\n"); + } else if (!strcmp(s, "readahead")) { + version = CACHE_SC_READAHEAD; + p9_debug(P9_DEBUG_9P, "Cache mode: readahead\n"); + } else if (!strcmp(s, "none")) { + version = CACHE_SC_NONE; + p9_debug(P9_DEBUG_9P, "Cache mode: none\n"); + } else if (kstrtoint(s, 0, &version) != 0) { + version = -EINVAL; + pr_info("Unknown Cache mode or invalid value %s\n", s); + } + return version; +} + /* * Display the mount options in /proc/mounts. */ @@ -269,8 +287,10 @@ int v9fs_parse_param(struct fs_context *fc, struct fs_parameter *param) #endif break; case Opt_cache: - session_opts->cache = result.uint_32; - p9_debug(P9_DEBUG_9P, "Cache mode: %s\n", param->string); + r = get_cache_mode(param->string); + if (r < 0) + return r; + session_opts->cache = r; break; case Opt_access: s = param->string; ^ permalink raw reply related [flat|nested] 23+ messages in thread
end of thread, other threads:[~2025-12-05 12:57 UTC | newest] Thread overview: 23+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-10 21:36 [PATCH V3 0/4] 9p: Convert to the new mount API Eric Sandeen 2025-10-10 21:36 ` [PATCH V3 1/4] fs/fs_parse: add back fsparam_u32hex Eric Sandeen 2025-10-10 21:36 ` [PATCH V3 2/4] net/9p: move structures and macros to header files Eric Sandeen 2025-10-10 21:36 ` [PATCH V3 3/4] 9p: create a v9fs_context structure to hold parsed options Eric Sandeen 2025-10-10 21:36 ` [PATCH V3 4/4] 9p: convert to the new mount API Eric Sandeen 2025-10-13 10:26 ` Dominique Martinet 2025-10-13 18:46 ` Eric Sandeen 2025-10-13 19:04 ` Dominique Martinet 2025-11-26 20:16 ` Remi Pommarel 2025-11-26 22:43 ` Dominique Martinet 2025-12-01 22:36 ` Eric Sandeen 2025-12-02 1:04 ` Dominique Martinet 2025-12-02 22:12 ` Eric Sandeen 2025-12-03 15:13 ` Dominique Martinet 2025-12-03 16:23 ` Eric Sandeen 2025-12-05 11:53 ` Remi Pommarel 2025-12-05 12:56 ` Dominique Martinet 2025-12-02 22:30 ` [PATCH V3 5/4] 9p: fix cache option printing in v9fs_show_options Eric Sandeen 2025-12-02 23:13 ` Al Viro 2025-12-03 1:09 ` Eric Sandeen 2025-12-03 15:04 ` Dominique Martinet 2025-12-03 18:04 ` Al Viro 2025-12-02 22:34 ` [PATCH V3 6/4] 9p: fix new mount API cache option handling Eric Sandeen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).