NFS Performance

[Steven Johnson]

Sorry about the delay in replying, I had to rush of overseas with no 
notice, and haven't been able to do anything till now.

Till Straumann wrote:
> Let me first thank Steven for bringing this issue to our attention and
> for testing the proposed fix and for providing patches.
No problem.  Improving RTEMS and submitting changes is small price to 
pay for such a great RTOS.  My sincerest thanks go to Joel and his team, 
Till and Eric, and everyone else who makes this possible.  Also, I must 
say this is one of the best run mailing lists for an open source project 
I am involved with.  In all the years I have tracked the list I have 
never witnessed anything closely resembling a flame war, or other 
intimidating list bad behaviour, and that is a credit to the 
professionalism of the primary developers.
>
> Eric Norum wrote:
>> I'd like to state my objection to this.   I don't think that it's a 
>> good idea to have an API with different arguments to the same function.
> I agree.
We argued about it internally as well.  We will do it the other way and 
have no problem at all in doing it, but at the moment we are busy 
tracking a big fault we need to fix, so it may be a little while before 
a fresh patch shows up from us.
>>
>> May I suggest instead that:
>> 1) All the ifdefs be removed fromt the nfs code.
> Yes. Note that there is no need to test for HAVE_BLKSIZE. It is 
> completely
> OK to always let the nfs code set st_blksize to what it believes is best.
> If newlib is compiled without defining HAVE_BLKSIZE then that hint
> is simply ignored [but might still be honoured by application code].
Will be done.
>> 2) A new function be added:
>>   nfsInitWithBufferSize(int smallPoolDepth, int bigPoolDepth, int 
>> bufferSize);
> If anything, I would prefer to make this a *mount* option. This would 
> (a) make
> more sense [per 'connection' rather than global setting]
> and (b) it would be possible to extend the existing API [by e.g., having
> options appended to the server string] w/o breaking existing code or 
> APIs.
hmm, I am not convinced.  I don't think the buffer issue is server 
dependent, but target hardware dependent (I could be wrong) at least it 
was in our case.  Every server we use needs the same buffer size to get 
the best performance.  I don't know of a case where different server's 
would benefit from different buffer sizes.  But, in any event there 
could still be a global value and if there was a mount option, it would 
just over-ride the global for that mount.
>
> For now, however, I believe a simple global variable should be enough. 
> The new
> behavior cannot do any harm (as far as I can see). If anyone want the 
> old behavior
> then they can change the global parameter - we can make the 
> default/init setting
> an option in the Makefile.
We can make the existing behaviour occur if the user initialises the nfs 
client with the existing nfsInit call or the new nfsInitWithBufferSize, 
and bufferSize is set to 0.  If there is value in that?  Obviously the 
existing behaviour will also still occur, regardless of what we do here 
unless the #define HAVE_BLKSIZE is used when compiling.  Which was one 
of our issues with the patch, having a call like nfsInitWithBufferSize 
take a parameter, that will do nothing unless the standard library is 
built with HAVE_BLKSIZE will potentially create confusion.  Would a 
warning be in-appropriate.  Something like

#ifndef HAVE_BLKSIZE
#warning "nfsInitWithBufferSize: HAVE_BLKSIZE must be defined when 
compiling the standard library before the bufferSize parameter will have 
effect"
#endif
>
> With regard to the 'pool size' -- let me clarify [and reiterate what 
> the comment
> in the code says]: 'small' and 'big' refer to the *send* buffer size, 
> so this is only
> needed for write and symlink requests. reads don't send much data ;-). 
> So IMO,
> the existing code was correct.
I understand the comment, but we were led to believe it would be of 
benefit by our understanding of the code, such as it is.  But I will not 
debate the matter, I will take on-board what you say and will re-look at 
it when we fix the buffer size patch up.  If I still think its an 
advantage to do this, ill post it separate with a rationale.  I will 
also test the performance impact it has (if any).
>
> If there are no further objections then I'd go ahead and make the 
> respective
> changes using the patches where appropriate.
Do you mean you are making the changes Till?  Or do you want me to 
submit a new patch? I will try and get a patch to the list this week if 
you still want me to.

Steven J
>
> -- Till