[openib-general] NFS/RDMA client release for Linux 2.6.15

[Christoph Hellwig]

On Sun, Feb 19, 2006 at 09:56:18AM -0500, Talpey, Thomas wrote:
> >  - please don't build the rdma transport unconditional, but make it
> >    a user-visible config option
> 
> It's an option, but it's located in fs/Kconfig not net/. This is the way
> SUNRPC is selected, so we simply followed that. BTW, Chuck's transport
> switch doesn't support dynamically loading modules yet so there is a
> dependency to work out until that's in place.

Right now it's an option, but not a user-selectable one:

--- snip ---
+config SUNRPC_XPRT_RDMA
+       depends on INFINIBAND
+	tristate
--- snip ---

to make it user-visible you need to add an option description after
the tristate, e.g.

	tristate "RDMA transport for sunrpc"

not strictly required but very useful is an additional help text using
the help verb of the kconfig language.  In the end form the select on
config SUNRPC shouldn't be there either.

> >  - the CONFIG_HIGHMEM ifdef block in RPC_SEND_COPY is wrong.  Please
> >    always use kmap, it does the right thing for non-highmem aswell.
> >    The PageHighMem check and using kmap_high directly is always
> >    wrong, they are internal implementation details.  I'd also suggest
> >    evaluating kmap_atomic because it scales much better on SMP systems.
> 
> Yes, there are some issues here which we're still working out. In fact, we
> can't use kunmap() in the context you mention because in 2.6.14 (or is it
> .15) it started to check for being invoked in interrupt context. There is
> one configuration in which we do call it in bh context. The call won't block
> but the kernel BUG_ON's. This is something on our list to address.

That's one more reason to use kmap_atomic/kunmap_atomic which is fine
from interrupt context.  You'll have to carefully check whether you can
use an existing KM_ slot or allocate a new one, though.

> >  - structures like rpcrdma_msg that are on the wire should use __be*
> >    for endianess annotations, and the cpu_to_be*/be*_to_cpu accessor
> >    functions instead of hton?/ntoh?.  Please verify that these annotations
> >    are correct using sparse -D__CHECK_ENDIAN__=1
> 
> Hmm, okay but existing RPC and NFS code don't do this. I'm reluctant to
> differ from the style of the governing subsystem. I'll check w/Trond.

The nfs code is in the process of beeing converted currently.

> >  - in transport.c please don't use CamelCase variable names.
> 
> This is just for module parameters? These are going away but we don't have
> the new NFS mount API yet. There is a comment to that effect but maybe
> it doesn't mention the module stuff.

It's for all variables, but afaik you only use mixed case for the module
paramaters.