| Message ID | 20211017134611.4330-1-linux@weissschuh.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/9p: autoload transport modules | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 9 this patch: 9 |
| netdev/kdoc | success | Errors and warnings before: 3 this patch: 3 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 85 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 5 this patch: 5 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
Hi, Sorry for the late reply Thomas Weißschuh wrote on Sun, Oct 17, 2021 at 03:46:11PM +0200: > Automatically load transport modules based on the trans= parameter > passed to mount. > The removes the requirement for the user to know which module to use. This looks good to me, I'll test this briefly on differnet config (=y, =m) and submit to Linus this week for the next cycle. Makes me wonder why trans_fd is included in 9pnet and not in a 9pnet-fd or 9pnet-tcp module but that'll be for another time...
Hi, On 2021-11-02 19:51+0900, Dominique Martinet wrote: > Sorry for the late reply > > Thomas Weißschuh wrote on Sun, Oct 17, 2021 at 03:46:11PM +0200: > > Automatically load transport modules based on the trans= parameter > > passed to mount. > > The removes the requirement for the user to know which module to use. > > This looks good to me, I'll test this briefly on differnet config (=y, > =m) and submit to Linus this week for the next cycle. Thanks. Could you also fix up the typo in the commit message when applying? ("The removes" -> "This removes") > Makes me wonder why trans_fd is included in 9pnet and not in a 9pnet-fd > or 9pnet-tcp module but that'll be for another time... To prepare for the moment when those transport modules are split into their own module(s), we could already add MODULE_ALIAS_9P() calls to net/9p/trans_fd.c. Thomas
Thomas Weißschuh wrote on Tue, Nov 02, 2021 at 11:59:32AM +0100: > On 2021-11-02 19:51+0900, Dominique Martinet wrote: > > Sorry for the late reply > > > > Thomas Weißschuh wrote on Sun, Oct 17, 2021 at 03:46:11PM +0200: > > > Automatically load transport modules based on the trans= parameter > > > passed to mount. > > > The removes the requirement for the user to know which module to use. > > > > This looks good to me, I'll test this briefly on differnet config (=y, > > =m) and submit to Linus this week for the next cycle. > > Thanks. Could you also fix up the typo in the commit message when applying? > ("The removes" -> "This removes") Sure, done -- I hadn't even noticed it.. > > Makes me wonder why trans_fd is included in 9pnet and not in a 9pnet-fd > > or 9pnet-tcp module but that'll be for another time... > > To prepare for the moment when those transport modules are split into their own > module(s), we could already add MODULE_ALIAS_9P() calls to net/9p/trans_fd.c. I guess it wouldn't hurt to have 9p-tcp 9p-unix and 9p-fd aliases to the 9pnet module, but iirc these transports were more closely tied to the rest of 9pnet than the rest so it might take a while to do and I don't have much time for this right now... I'd rather not prepare for something I'll likely never get onto, so let's do this if there is progress. Of course if you'd like to have a look that'd be more than welcome :-)
On 2021-11-02 20:51+0900, Dominique Martinet wrote: > Thomas Weißschuh wrote on Tue, Nov 02, 2021 at 11:59:32AM +0100: > > On 2021-11-02 19:51+0900, Dominique Martinet wrote: > > > Sorry for the late reply > > > > > > Thomas Weißschuh wrote on Sun, Oct 17, 2021 at 03:46:11PM +0200: > > > > Automatically load transport modules based on the trans= parameter > > > > passed to mount. > > > > The removes the requirement for the user to know which module to use. > > > > > > This looks good to me, I'll test this briefly on differnet config (=y, > > > =m) and submit to Linus this week for the next cycle. > > > > Thanks. Could you also fix up the typo in the commit message when applying? > > ("The removes" -> "This removes") > > Sure, done -- I hadn't even noticed it.. > > > > Makes me wonder why trans_fd is included in 9pnet and not in a 9pnet-fd > > > or 9pnet-tcp module but that'll be for another time... > > > > To prepare for the moment when those transport modules are split into their own > > module(s), we could already add MODULE_ALIAS_9P() calls to net/9p/trans_fd.c. > > I guess it wouldn't hurt to have 9p-tcp 9p-unix and 9p-fd aliases to the > 9pnet module, but iirc these transports were more closely tied to the > rest of 9pnet than the rest so it might take a while to do and I don't > have much time for this right now... > I'd rather not prepare for something I'll likely never get onto, so > let's do this if there is progress. > > Of course if you'd like to have a look that'd be more than welcome :-) If you are still testing anyways, you could also try the attached patch. (It requires the autload patch) It builds fine and I see no reason for it not to work. Thomas diff --git a/include/net/9p/9p.h b/include/net/9p/9p.h index 03614de86942..f420f8cb378d 100644 --- a/include/net/9p/9p.h +++ b/include/net/9p/9p.h @@ -553,6 +553,4 @@ struct p9_fcall { int p9_errstr2errno(char *errstr, int len); int p9_error_init(void); -int p9_trans_fd_init(void); -void p9_trans_fd_exit(void); #endif /* NET_9P_H */ diff --git a/net/9p/Makefile b/net/9p/Makefile index aa0a5641e5d0..b7d2ea495f65 100644 --- a/net/9p/Makefile +++ b/net/9p/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_NET_9P) := 9pnet.o +obj-$(CONFIG_NET_9P) += 9pnet_fd.o obj-$(CONFIG_NET_9P_XEN) += 9pnet_xen.o obj-$(CONFIG_NET_9P_VIRTIO) += 9pnet_virtio.o obj-$(CONFIG_NET_9P_RDMA) += 9pnet_rdma.o @@ -9,9 +10,11 @@ obj-$(CONFIG_NET_9P_RDMA) += 9pnet_rdma.o client.o \ error.o \ protocol.o \ - trans_fd.o \ trans_common.o \ +9pnet_fd-objs := \ + trans_fd.o \ + 9pnet_virtio-objs := \ trans_virtio.o \ diff --git a/net/9p/mod.c b/net/9p/mod.c index 5126566850bd..dee263f8e361 100644 --- a/net/9p/mod.c +++ b/net/9p/mod.c @@ -164,7 +164,6 @@ static int __init init_p9(void) p9_error_init(); pr_info("Installing 9P2000 support\n"); - p9_trans_fd_init(); return ret; } @@ -178,7 +177,6 @@ static void __exit exit_p9(void) { pr_info("Unloading 9P2000 support\n"); - p9_trans_fd_exit(); p9_client_exit(); } diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 007bbcc68010..ff95bdf8baa5 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -1092,6 +1092,7 @@ static struct p9_trans_module p9_tcp_trans = { .show_options = p9_fd_show_options, .owner = THIS_MODULE, }; +MODULE_ALIAS_9P("tcp"); static struct p9_trans_module p9_unix_trans = { .name = "unix", @@ -1105,6 +1106,7 @@ static struct p9_trans_module p9_unix_trans = { .show_options = p9_fd_show_options, .owner = THIS_MODULE, }; +MODULE_ALIAS_9P("unix"); static struct p9_trans_module p9_fd_trans = { .name = "fd", @@ -1118,6 +1120,7 @@ static struct p9_trans_module p9_fd_trans = { .show_options = p9_fd_show_options, .owner = THIS_MODULE, }; +MODULE_ALIAS_9P("fd"); /** * p9_poll_workfn - poll worker thread @@ -1167,3 +1170,10 @@ void p9_trans_fd_exit(void) v9fs_unregister_trans(&p9_unix_trans); v9fs_unregister_trans(&p9_fd_trans); } + +module_init(p9_trans_fd_init); +module_exit(p9_trans_fd_exit); + +MODULE_AUTHOR("Eric Van Hensbergen <ericvh@gmail.com>"); +MODULE_DESCRIPTION("Filedescriptor Transport for 9P"); +MODULE_LICENSE("GPL");
Thomas Weißschuh wrote on Tue, Nov 02, 2021 at 03:49:32PM +0100: > > I guess it wouldn't hurt to have 9p-tcp 9p-unix and 9p-fd aliases to the > > 9pnet module, but iirc these transports were more closely tied to the > > rest of 9pnet than the rest so it might take a while to do and I don't > > have much time for this right now... > > I'd rather not prepare for something I'll likely never get onto, so > > let's do this if there is progress. > > > > Of course if you'd like to have a look that'd be more than welcome :-) > > If you are still testing anyways, you could also try the attached patch. > (It requires the autload patch) > > It builds fine and I see no reason for it not to work. Thanks! I'll give it a spin. I was actually just testing the autoload one and I can't get it to work on my minimal VM, I guess there's a problem with the usermodhelper call to load module.. with 9p/9pnet loaded, running "mount -t 9p -o trans=virtio tmp /mnt" request_module("9p-%s", "virtio") returns -2 (ENOENT) Looking at the code it should be running "modprobe -q -- 9p-virtio" which finds the module just fine, hence my supposition usermodhelper is not setup correctly Do you happen to know what I need to do for it? I've run out of time for today but will look tomorrow if you don't know. (And since it doesn't apparently work out of the box on these minimal VMs I think I'll want the trans_fd module split to sit in linux-next for a bit longer than a few days, so will be next merge window) Thanks,
On 2021-11-02 23:58+0900, Dominique Martinet wrote: > Thomas Weißschuh wrote on Tue, Nov 02, 2021 at 03:49:32PM +0100: > > > I guess it wouldn't hurt to have 9p-tcp 9p-unix and 9p-fd aliases to the > > > 9pnet module, but iirc these transports were more closely tied to the > > > rest of 9pnet than the rest so it might take a while to do and I don't > > > have much time for this right now... > > > I'd rather not prepare for something I'll likely never get onto, so > > > let's do this if there is progress. > > > > > > Of course if you'd like to have a look that'd be more than welcome :-) > > > > If you are still testing anyways, you could also try the attached patch. > > (It requires the autload patch) > > > > It builds fine and I see no reason for it not to work. > > Thanks! I'll give it a spin. > > > I was actually just testing the autoload one and I can't get it to work > on my minimal VM, I guess there's a problem with the usermodhelper call > to load module.. > > with 9p/9pnet loaded, > running "mount -t 9p -o trans=virtio tmp /mnt" > request_module("9p-%s", "virtio") returns -2 (ENOENT) Can you retry without 9p/9pnet loaded and see if they are loaded by the mount process? The same autoloading functionality exists for filesystems using request_module("fs-%s") in fs/filesystems.c If that also doesn't work it would indicate an issue with the kernel setup in general. > Looking at the code it should be running "modprobe -q -- 9p-virtio" > which finds the module just fine, hence my supposition usermodhelper is > not setup correctly > > Do you happen to know what I need to do for it? What is the value of CONFIG_MODPROBE_PATH? And the contents of /proc/sys/kernel/modprobe? > I've run out of time for today but will look tomorrow if you don't know. > > (And since it doesn't apparently work out of the box on these minimal > VMs I think I'll want the trans_fd module split to sit in linux-next > for a bit longer than a few days, so will be next merge window) Sure. Thomas
Thomas Weißschuh wrote on Tue, Nov 02, 2021 at 04:32:21PM +0100: > > with 9p/9pnet loaded, > > running "mount -t 9p -o trans=virtio tmp /mnt" > > request_module("9p-%s", "virtio") returns -2 (ENOENT) > > Can you retry without 9p/9pnet loaded and see if they are loaded by the mount > process? > The same autoloading functionality exists for filesystems using > request_module("fs-%s") in fs/filesystems.c > If that also doesn't work it would indicate an issue with the kernel setup in general. Right, that also didn't work, which matches modprobe not being called correctly > > Looking at the code it should be running "modprobe -q -- 9p-virtio" > > which finds the module just fine, hence my supposition usermodhelper is > > not setup correctly > > > > Do you happen to know what I need to do for it? > > What is the value of CONFIG_MODPROBE_PATH? > And the contents of /proc/sys/kernel/modprobe? aha, these two were indeed different from where my modprobe is so it is a setup problem -- I might have been a little rash with this initrd setup and modprobe ended up in /bin with path here in /sbin... Thanks for the pointer, I saw the code setup an environment with a full-blown PATH so didn't think of checking if this kind of setting existed! All looks in order then :)
Nov 3, 2021 00:18:13 Dominique Martinet <asmadeus@codewreck.org>: > Thomas Weißschuh wrote on Tue, Nov 02, 2021 at 04:32:21PM +0100: >>> with 9p/9pnet loaded, >>> running "mount -t 9p -o trans=virtio tmp /mnt" >>> request_module("9p-%s", "virtio") returns -2 (ENOENT) >> >> Can you retry without 9p/9pnet loaded and see if they are loaded by the mount >> process? >> The same autoloading functionality exists for filesystems using >> request_module("fs-%s") in fs/filesystems.c >> If that also doesn't work it would indicate an issue with the kernel setup in general. > > Right, that also didn't work, which matches modprobe not being called > correctly > > >>> Looking at the code it should be running "modprobe -q -- 9p-virtio" >>> which finds the module just fine, hence my supposition usermodhelper is >>> not setup correctly >>> >>> Do you happen to know what I need to do for it? >> >> What is the value of CONFIG_MODPROBE_PATH? >> And the contents of /proc/sys/kernel/modprobe? > > aha, these two were indeed different from where my modprobe is so it is > a setup problem -- I might have been a little rash with this initrd > setup and modprobe ended up in /bin with path here in /sbin... > > Thanks for the pointer, I saw the code setup an environment with a > full-blown PATH so didn't think of checking if this kind of setting > existed! > All looks in order then :) Does it also work for the split out FD transports? If so, I'll resend that patch in a proper form tomorrow. Thomas
Thomas Weißschuh wrote on Tue, Nov 02, 2021 at 11:33:25PM +0000: > > aha, these two were indeed different from where my modprobe is so it is > > a setup problem -- I might have been a little rash with this initrd > > setup and modprobe ended up in /bin with path here in /sbin... > > > > Thanks for the pointer, I saw the code setup an environment with a > > full-blown PATH so didn't think of checking if this kind of setting > > existed! > > All looks in order then :) > > Does it also work for the split out FD transports? > If so, I'll resend that patch in a proper form tomorrow. Sorry haven't tested yet, I need to fiddle a bit to get a tcp server setup right now and got a fscache bug I'd like fixed for this merge window... I've confirmed the module gets loaded but that's as far as I can get right now... it calls p9_fd_create_tcp so it's probably quite ok :) I'd also be tempted to add a new transport config option that defaults to true/NET_9P value -- in my opinion the main advantage of splitting this is not installing tcp/fd transport which can more easily be abused than virtio for VMs who wouldn't need it (most of them), so having a toggle would be handy. Feel free to resend in a proper form though, I could make up a commit message but it might as well be your words!
Dominique Martinet wrote on Wed, Nov 03, 2021 at 09:26:32AM +0900: > Feel free to resend in a proper form though, I could make up a commit > message but it might as well be your words! Ah, just a couple more things: * make with W=1 complains about missing prototypes: net/9p/trans_fd.c:1155:5: warning: no previous prototype for ‘p9_trans_fd_init’ [-Wmissing-prototypes] 1155 | int p9_trans_fd_init(void) | ^~~~~~~~~~~~~~~~ net/9p/trans_fd.c:1164:6: warning: no previous prototype for ‘p9_trans_fd_exit’ [-Wmissing-prototypes] 1164 | void p9_trans_fd_exit(void) | ^~~~~~~~~~~~~~~~ * This actually break the 'no trans=tcp' specified case when no extra module is loaded, but I'm not sure how impactful that is. See v9fs_get_default_trans(), they iterate through loaded transports (through register_trans()), we might want to bake in a list that additionally tries to load modules if no module is loaded at all (in my opinion virtio makes sense before tcp, then fd, unix, xen, rdma?) Well, that can probably come later.
diff --git a/include/net/9p/transport.h b/include/net/9p/transport.h index 3eb4261b2958..581555d88cba 100644 --- a/include/net/9p/transport.h +++ b/include/net/9p/transport.h @@ -11,6 +11,8 @@ #ifndef NET_9P_TRANSPORT_H #define NET_9P_TRANSPORT_H +#include <linux/module.h> + #define P9_DEF_MIN_RESVPORT (665U) #define P9_DEF_MAX_RESVPORT (1023U) @@ -55,4 +57,8 @@ void v9fs_unregister_trans(struct p9_trans_module *m); struct p9_trans_module *v9fs_get_trans_by_name(char *s); struct p9_trans_module *v9fs_get_default_trans(void); void v9fs_put_trans(struct p9_trans_module *m); + +#define MODULE_ALIAS_9P(transport) \ + MODULE_ALIAS("9p-" transport) + #endif /* NET_9P_TRANSPORT_H */ diff --git a/net/9p/mod.c b/net/9p/mod.c index 5126566850bd..aa38b8b0e0f6 100644 --- a/net/9p/mod.c +++ b/net/9p/mod.c @@ -12,6 +12,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include <linux/module.h> +#include <linux/kmod.h> #include <linux/errno.h> #include <linux/sched.h> #include <linux/moduleparam.h> @@ -87,12 +88,7 @@ void v9fs_unregister_trans(struct p9_trans_module *m) } EXPORT_SYMBOL(v9fs_unregister_trans); -/** - * v9fs_get_trans_by_name - get transport with the matching name - * @s: string identifying transport - * - */ -struct p9_trans_module *v9fs_get_trans_by_name(char *s) +static struct p9_trans_module *_p9_get_trans_by_name(char *s) { struct p9_trans_module *t, *found = NULL; @@ -106,6 +102,28 @@ struct p9_trans_module *v9fs_get_trans_by_name(char *s) } spin_unlock(&v9fs_trans_lock); + + return found; +} + +/** + * v9fs_get_trans_by_name - get transport with the matching name + * @s: string identifying transport + * + */ +struct p9_trans_module *v9fs_get_trans_by_name(char *s) +{ + struct p9_trans_module *found = NULL; + + found = _p9_get_trans_by_name(s); + +#ifdef CONFIG_MODULES + if (!found) { + request_module("9p-%s", s); + found = _p9_get_trans_by_name(s); + } +#endif + return found; } EXPORT_SYMBOL(v9fs_get_trans_by_name); diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c index af0a8a6cd3fd..480fd27760d7 100644 --- a/net/9p/trans_rdma.c +++ b/net/9p/trans_rdma.c @@ -767,6 +767,7 @@ static void __exit p9_trans_rdma_exit(void) module_init(p9_trans_rdma_init); module_exit(p9_trans_rdma_exit); +MODULE_ALIAS_9P("rdma"); MODULE_AUTHOR("Tom Tucker <tom@opengridcomputing.com>"); MODULE_DESCRIPTION("RDMA Transport for 9P"); diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index 490a4c900339..bd5a89c4960d 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -794,6 +794,7 @@ static void __exit p9_virtio_cleanup(void) module_init(p9_virtio_init); module_exit(p9_virtio_cleanup); +MODULE_ALIAS_9P("virtio"); MODULE_DEVICE_TABLE(virtio, id_table); MODULE_AUTHOR("Eric Van Hensbergen <ericvh@gmail.com>"); diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c index 3ec1a51a6944..e264dcee019a 100644 --- a/net/9p/trans_xen.c +++ b/net/9p/trans_xen.c @@ -552,6 +552,7 @@ static int p9_trans_xen_init(void) return rc; } module_init(p9_trans_xen_init); +MODULE_ALIAS_9P("xen"); static void p9_trans_xen_exit(void) {
Automatically load transport modules based on the trans= parameter passed to mount. The removes the requirement for the user to know which module to use. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- include/net/9p/transport.h | 6 ++++++ net/9p/mod.c | 30 ++++++++++++++++++++++++------ net/9p/trans_rdma.c | 1 + net/9p/trans_virtio.c | 1 + net/9p/trans_xen.c | 1 + 5 files changed, 33 insertions(+), 6 deletions(-) base-commit: fac3cb82a54a4b7c49c932f96ef196cf5774344c