NFS Performance

[Till Straumann]

Steven Johnson wrote:
> 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.
thx :-)
>>
>> 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)
nothing to do with the target hardware but with the NFS client 
implementation (software
issue - no cache/read-ahead).

On platforms with memory constraints it could be desirable making this a 
'per mount' option.
[OTOH, on such platforms applications could manage buffers explicitely].
> 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.
Problem is that this is AFAIK not defined in any exported newlib header 
so I don't know
how an application could check how newlib was configured.

>   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.
It's already done. I didn't have the time to put out a new release yet but
no new patches are needed.

-- Till
>
> Steven J
>>
>> -- Till
>