Page MenuHomeFreeBSD

remove the definition of the "nfs" module type
ClosedPublic

Authored by rmacklem on May 4 2021, 3:31 PM.

Details

Summary

The nfs client module nfscl.ko has definitions for both
"nfscl" and "nfs" module names. This can cause confusion
and it appears the "nfs" module name is not needed.

mount_nfs.c as far back as FreeBSD8 knows to use
"nfscl" as the name for the client module.
I thought that very old versions of mount_nfs used
"nfs" as the module name to load, but that is not
the case for at least FreeBSD8 and newer.

This patch removes the definition of the "nffs" module,
to simplify the code and avoid confusion w.r.t. the
module's name.

Suggested by kib@.

Test Plan

Built a kernel without any NFS options and then loaded
the NFS client via both:

kldload nfscl.ko

and

mount -t nfs ...

after booting and both worked fine.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rmacklem created this revision.
sys/fs/nfsclient/nfs_clvfsops.c
158–161

This line alone defines the module 'nfs'. Look at the definition of VFS_SET().
This is why there is no DECLARE_MODULE(nfs) in the source.

I am aware of one use of nfscl name, as the dependency for dtnfscl module.
Is nfscl name used by userspace anywhere ?

I suspect that removal of nfs name might result in 'mount -t nfs ...' not loading the nfscl.ko automatically if it is not present in kernel.

So may be remove nfscl instead?

sys/fs/nfsclient/nfs_clvfsops.c
158–161

kldload("nfscl") is in mount_nfs(8) and nfscbd(8).
Maybe others, I didn't bother to find/grep it.

It is also in assorted scripts in /etc/rc.d.

As such, I think both module names should probably
remain.

This patch might still be useful, to get rid of seemingly
unnecessary MODULE_DEPEND() lines.
--> Or we should keep them consistent?

Oh, and there is also nfscl_modevent() in nfs_clport.c.
If you get rid of "nfscl", how do you wire that in so
that it gets called during kldload/kldunload?

This is a carryover from when there were two NFS clients.

Oh, and there is also nfscl_modevent() in nfs_clport.c.
If you get rid of "nfscl", how do you wire that in so
that it gets called during kldload/kldunload?

This is a carryover from when there were two NFS clients.

In other words, everything expects 'nfscl', right? Then why not do VFS_SET(nfs_vfsops, nfscl, ...) and get rid of nfs?

Assorted things expect the file system type to be "nfs".
I mostly think that is the duplicate MODULE_DEPEND()
macros that are confusing.

If you really want the "nfs" module name to go away,
we could define a macro that sets the file system
type but does not define a module of the same name
and use that instead of VFS_SET().
--> nfscl_modevent() could call vfs_modevent() to register

 the file system type, or just call vfs_register(),
which is all vfs_modevent() does for MOD_LOAD. nfscl cannot be unloaded
safely, which is one of the things nfscl_modevent() handles.

I'll let you decide what is preferred?

Personally, I think getting rid of the duplicate
MODULE_DEPEND() calls is sufficient.

Assorted things expect the file system type to be "nfs".
I mostly think that is the duplicate MODULE_DEPEND()
macros that are confusing.

If you really want the "nfs" module name to go away,
we could define a macro that sets the file system
type but does not define a module of the same name
and use that instead of VFS_SET().
--> nfscl_modevent() could call vfs_modevent() to register

 the file system type, or just call vfs_register(),
which is all vfs_modevent() does for MOD_LOAD. nfscl cannot be unloaded
safely, which is one of the things nfscl_modevent() handles.

I'll let you decide what is preferred?

Personally, I think getting rid of the duplicate
MODULE_DEPEND() calls is sufficient.

Ok, but please keep MOD_VERSION(nfs) then. It is not really complete module declaration without version. Depends are enough for one module in linker file.

Leave MODULE_VERSION(nfs, 1) in nfs_clvfsops.c,
as requested by kib@.

The summary now needs to be changed to describe
the removal of unnecessary MODULE_DEPENDS()
lines, but not the removal of the "nfs" module
name, created by VFS_SET(), which also names
the file system type "nfs".

This revision is now accepted and ready to land.May 8 2021, 10:56 PM

One thing that I thought about later, is that it is probably useful to cross-reference both definitions, or even move nfscl one just below nfs, stating that both are used.

By cross-referencing, do you mean adding comments
explaining the other definition exists or ?

By cross-referencing, do you mean adding comments
explaining the other definition exists or ?

Yes, pointing out that the other definition exists, where, and why. If you move both definitions to one place, only 'why' is needed.