Trond Myklebust wrote:
> On Fri, 2007-01-19 at 17:46 -0500, Chuck Lever wrote:
>> Trond Myklebust wrote:
>>> On Thu, 2007-01-18 at 18:30 -0500, Chuck Lever wrote:
>>>> The RPC buffer size estimation logic in net/sunrpc/clnt.c always
>>>> significantly overestimates the requirements for the buffer size.
>>>> A little instrumentation demonstrated that in fact rpc_malloc was never
>>>> allocating the buffer from the mempool, but almost always called kmalloc.
>>>>
>>>> To compute the size of the RPC buffer more precisely, split p_bufsiz into
>>>> two fields; one for the argument size, and one for the result size.  The
>>>> goal is to keep the size requirement for RPC buffers at or below 2KiB.
>>>>
>>>> So now we will compute the sum of the exact call and reply header sizes,
>>>> and split the RPC buffer precisely between the two.  That should keep all
>>>> RPCs within the 2KiB buffer mempool limit.
>>> How about adding a temporary BUG_ON() into call_allocate to ensure
>>> task->tk_msg.rpc_proc->p_arglen is always non-zero. We also want to
>>> check for non-zero values of task->tk_msg.rpc_proc->p_replen when
>>> task->tk_msg.rpc_proc->p_decode is non-null.
>> Hmmm.  A sanity check is reasonable, but do we really want a runtime 
>> check for values that are fixed at compile time?  I'm also not in love 
>> with BUG_ON() -- maybe something like aborting the task and printing a 
>> console warning would be less disruptive.
> 
> How is that less disruptive? A bug by any other name....

Depending on what locks or semaphores are held at the time the bug 
trips, it could freeze up the whole system.  If we cleanly abort the 
faulty RPC, that localizes the failure without affecting the rest of the 
system.

> The point is just to make sure that all existing structures have been
> converted and that we haven't missed anything. Feel free to queue up a
> patch for the release after this patch has been merged in order to
> remove it.

OK.

>>> Finally, what is the purpose of req->rq_callsize and req->rq_rcvsize?
>>> They don't appear to be used anywhere.
>> In my code, they are used in call_encode and xprt_release in addition to 
>> call_allocate.  They basically replace rq_bufsize.
> 
> In that case, why not just add them in the patch that contains the code
> that uses them?

I'm not sure why you don't have those hunks.  They should be in the same 
patch.  I'm going to resend these patches sometime today or tomorrow.