ZFS should depend on XDR, not full RPC

[glebius]

Summary

ZFS uses a few functions from the kernel XDR library. However, this pulls
all of kernel RPC as dependency.

• Use separate malloc type for XDR allocations.
• Split krpc.ko into krpc.ko and xdr.ko
• Make zfs.ko depend on xdr.ko only.

Need a nod from NFS maintainer as main consumer of RPC and from any
ZFS expert.

[rmacklem]

I commented on a couple of the places where it checks
for a NULL return from malloc() and then got lazy, so
I didn't do the rest.

My only concern is that there is a mem_free() call done
somewhere in the rpc code for one of these malloc()s,
but I couldn't find one.
If this happens, it will be easily spotted during testing
when "vmstat -m" generates crap for "rpc" and "xdr".

I'm fine with it as it. The comments just point to possible changes.

sys/conf/options

472 ↗

(On Diff #70527)

You might want to add "opt_xdr.h" here.
It might be useful in the future?

sys/xdr/xdr_reference.c

85 ↗

(On Diff #70527)

No need to check for NULL return, but it's harmless.
(I believe this code was ported from a userland library
where malloc() could fail.)

[rmacklem]

Oh, one other thing.
I can never remember without doing a build, but maybe a
MODULE_DEPEND(xxx, xdr, 1, 1, 1)
needs to be added to all the modules that currently have
MODULE_DEPEND(xxx, krpc, 1, 1, 1)?

[rmacklem]

One case that needs to be tested is building a kernel with only "options KRPC".

sys/conf/files

4991 ↗

(On Diff #70527)

I think you need "optional xdr | krpc | nfslockd | nfscl | nfsd".
Otherwise, a kernel built with only "options KRPC" will not link, I think?
(I haven't tried these builds.)

[glebius]

sys/conf/files

4991 ↗

(On Diff #70527)

Thanks, you are right!

sys/conf/options

472 ↗

(On Diff #70527)

opt_foo is used only when there are sources outside of foo itself, that need to be compiled differently in presence of foo. Since XDR is a stub library, it isn't needed here and zero probability would ever be needed.

sys/xdr/xdr_reference.c

85 ↗

(On Diff #70527)

Right. And it seems possible that someday it will be same source for userland and kernel, not a copy and paste as it is now, that's why I decided not to remove the lines that potentially would be useful in future.

[glebius]

In D24408#536859, @rmacklem wrote:

Oh, one other thing.
I can never remember without doing a build, but maybe a
MODULE_DEPEND(xxx, xdr, 1, 1, 1)
needs to be added to all the modules that currently have
MODULE_DEPEND(xxx, krpc, 1, 1, 1)?

AFAIK, dependencies are chained, so there is no need for that.

[glebius]

In D24408#536854, @rmacklem wrote:

My only concern is that there is a mem_free() call done
somewhere in the rpc code for one of these malloc()s,
but I couldn't find one.
If this happens, it will be easily spotted during testing
when "vmstat -m" generates crap for "rpc" and "xdr".

I double checked that. Here is my finds:

o xdr_array()'s malloc. xdr_array() isn't used by kernel RPC
o xdr_reference()'s malloc. xdr_reference() is used by rpcb_prot.c. Reviewing I see that it never would free memory by itself. Would always call down to xdr_reference() to do a free.
o xdr_sizeof()'s malloc. Always assigns result to x_private member that is never used by RPC.
o xdr.c xdr_bytes() malloc.

• Used by rpcb_clnt.c The whole file seems to be #ifdef 0. At least I don't see any of its symbols in krpc.ko. Putting typos in functions around calling into xdr_bytes() doesn't break compilation. krpc.ko can link without rpcb_clnt.o.
• Used by rpc_prot.c's xdr_opaque_auth(). This one is problematic. There is at least one path that would lead to allocing in xdr_bytes() and then freeing in authunix_validate() or in authunix_destroy(). To fix that I'm going to modify xdr_opaque_auth, to always preallocate with M_RPC type.

o xdr.c xdr_string() malloc. Used by xdr_rpcb() and xdr_rpcb_entry(). These two functions are not used in kernel. I can #ifdef 0 them for safety.

[glebius]

Address Rick's comment and split into more commits.

• In xdr_opaque_auth() always preallocate memory for our base. If we
• Disable xdr_rpcb() and xdr_rpcb_entry(). They aren't used in kernel,
• Use separate malloc(9) type for XDR.
• Split XDR into separate kernel module. Make krpc depend on xdr.
• Make ZFS depend on xdr only.

[rmacklem]

It seems that a lot of the complexity of this patch is a result of defining
a new malloc() type.
Is a new type really needed?
I'll leave it up to you, but I'd just stick with M_RPC everywhere and
that would keep the patch a lot simpler.

Btw, I think I got bit once thinking that the MODULE_DEPEND()s recurse,
but that was a long time ago.
Easy to test. Just build a kernel with none of the options and then
try and start "nfsd".

sys/conf/options

472 ↗

(On Diff #70527)

I'm not sure anything is zero probability ever needed.
For example, if someday the ZFS code has an alternate
way of doing encoding instead of using XDR (I was slightly
surprised it did use XDR at all), then #ifdef XDR might
be useful.

But it could easily be changed if/when it is needed.

[mmacy]

I don't see anything wrong with this review, but FYI there is now a live upstream https://github.com/openzfs/zfs/commit/9f0a21e6411aa0bac23fba0ddb220342a48c7cc7 and the code in HEAD will some time later this year be replaced with vendor code. Most of the changes to ZFS in HEAD at this point will only live on through MFCs to stable/12 and stable/11.

[glebius]

In D24408#537225, @rmacklem wrote:

It seems that a lot of the complexity of this patch is a result of defining
a new malloc() type.
Is a new type really needed?
I'll leave it up to you, but I'd just stick with M_RPC everywhere and
that would keep the patch a lot simpler.

The malloc type is a symbol. xdr.ko can't load if it uses symbol that lives in rpc.ko. Other option would be to move MALLOC_DECLARE(M_RPC) out of rpc into xdr. What would you prefer? I'd prefer to separate malloc types, since the libraries are already separated. The only problem was xdr_opaqueauth(), which I'd call a design bug.

sys/conf/options

472 ↗

(On Diff #70527)

If this happens than opt_xdr.h can be created. Today that would be include that no one includes.

[glebius]

In D24408#537268, @mmacy wrote:

I don't see anything wrong with this review, but FYI there is now a live upstream https://github.com/openzfs/zfs/commit/9f0a21e6411aa0bac23fba0ddb220342a48c7cc7 and the code in HEAD will some time later this year be replaced with vendor code. Most of the changes to ZFS in HEAD at this point will only live on through MFCs to stable/12 and stable/11.

This review changes one line in ZFS code. Will you be able to upstream it?

[mmacy]

I can for the very near future, but as we get closer to replacing what’s in HEAD the burden should shift to the individual contributor to interact directly with upstream.

[rmacklem]

Well, I'll admit I suspect there may be more surprises related to
using a separate malloc type of M_XDR.
If you commit this to head, I will test it and look for malloc stats
being messed up. I am curious to see if there are any.

Obviously, from my last comment/question I, personally, would
not have defined a new malloc type to keep the patch simpler.
But it is your patch and up to you.

[freqlabs]

Out of curiosity, how does options XDR (in a kernel config) avoid a name collision with the XDR typedef?

[freqlabs]

sys/rpc/rpcb_prot.c

151 ↗

(On Diff #70571)

Build fails for me, xdr_rpcb is used here

242 ↗

(On Diff #70571)

And xdr_rpcb_entry is used here. May be more hiding, I'll let you track them down

[glebius]

Since separating malloc types goes more and more complicated, follow
Rick's suggestion and abandon that idea. Move MALLOC_DECLARE(M_RPC)
into XDR. Three commits remain:

• Move M_RPC malloc type into XDR. Both RPC and XDR libraries use
• Split XDR into separate kernel module. Make krpc depend on xdr.
• Make ZFS depend on xdr only.

[glebius]

In D24408#537610, @freqlabs wrote:

Out of curiosity, how does options XDR (in a kernel config) avoid a name collision with the XDR typedef?

Kernel config options and C typedefs are totally orthogonal namespaces.

[glebius]

sys/rpc/rpcb_prot.c

151 ↗

(On Diff #70571)

Yeah, that was the last straw. I went too far with malloc types separation, and decided to abandon that idea. New diff uploaded.

[glebius]

In D24408#536859, @rmacklem wrote:

Oh, one other thing.
I can never remember without doing a build, but maybe a
MODULE_DEPEND(xxx, xdr, 1, 1, 1)
needs to be added to all the modules that currently have
MODULE_DEPEND(xxx, krpc, 1, 1, 1)?

I have checked. Dependencies are correctly chained. 'kldload nfs' now loads all required dependency chain:
root@bobrik:/home/glebius:|>kldload nfs
root@bobrik:/home/glebius:|>kldstat
Id Refs Address Size Name
1 12 0xffffffff80200000 ff4400 kernel
2 1 0xffffffff81310000 63d18 nfscl.ko
3 2 0xffffffff81374000 20e30 nfscommon.ko
4 3 0xffffffff81395000 21a8 nfssvc.ko
5 3 0xffffffff81398000 1d900 krpc.ko
6 1 0xffffffff813b6000 5128 xdr.ko

[rmacklem]

This looks fine to me.