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

[Christoph Hellwig]

On Wed, Feb 08, 2006 at 03:58:56PM -0500, Talpey, Thomas wrote:
> We have released an updated NFS/RDMA client for Linux at
> the project's Sourceforge site:

Thanks, this looks much better than the previous patch.

Comments:

  - please don't build the rdma transport unconditional, but make it
    a user-visible config option
  - please use the kernel u*/s* types instead of (u)int*_t
  - please include your local headers after the <linux/*.h> headers,
    and keep all the includes at the beginning of the files, just after
    the licence comment block
  - chunktype shouldn't be a typedef but a pure enum, and the
    names look a bit too generic, please add an rdma_ prefix
  - please kill the XDR_TARGET and pos0 macros, maybe RPC_SEND_SEG0
    and RPC_SEND_LEN0, too
  - RPC_SEND_VECS should become an inline functions and be spelled
    lowercase
  - RPC_SEND_COPY is probably too large to be inlined and should be
    spelled lowercase
  - 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.
  - RPC_RECV_VECS should be an inline and spelled lowercase
  - RPC_RECV_SEG0 and PC_RECV_LEN0 should probably go away.
  - RPC_RECV_COPY is probably too large to be inlined and should be
    spelled lowercase
  - RPC_RECV_COPY same comment about highmem and kmap as in
    RPC_SEND_COPY
  - please try to avoid file-scope forward-prototypes but try to order the
    code in the natural flow where they aren't required
  - 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
  - rdma_convert_physiov/rdma_convert_phys are completely broken.
    page_to_phys can't be used by driver/fs code.  RDMA only deals with bus
    addresses, not physical addresses.  You must use the dma mapping API
    instead. Also coalescing decisions are made by the dma layer, because
    they are platform dependent and much more complex then what the code
    in this patch does.
  - transport.c is missing a GPL license statement
  - in transport.c please don't use CamelCase variable names.
  - MODULE_PARM shouldn't be used in new code, but module_param instead.
  - please don't use the (void) function() style, it just obsfucates the
    code without benefit.
  - try_module_get(THIS_MODULE) is always wrong.  Reference counting
    should happen from the calling module.
  - please initialize global or file-scope spinlocks with
    DEFINE_SPINLOCK().
  - the traditional name for the second argument to spin_lock_irqsave is
    just flags, not lock_flags.  This doesn't really matter, but
    following such conventions makes it easier to understand the code
    for kernel hackers that just occasionally drop into your code.
  - no need to case the return value from kmalloc/kzalloc/etc.  They
    return void * which can be directly assigned to any pointer type.
  - please avoid typedes for structure types, like struct rdma_ia,
    struct rdma_ep, etc..