NFS Performance

[Till Straumann]

Let me first thank Steven for bringing this issue to our attention and
for testing the proposed fix and for providing patches.

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.
>
> 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].
> 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.

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.

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.

If there are no further objections then I'd go ahead and make the respective
changes using the patches where appropriate.

-- Till
>
> The existing nfsInit would call this with the existing size, so all 
> old code would continue to work.  Code that wants to specify a buffer 
> size would use the new function.
>
> On Sep 15, 2006, at 7:11 PM, Steven Johnson wrote:
>
>> Hi
>>
>>> Sound's fine.  Changing it there is quite easy.  I think it should
>>> default to 8K, and let the user tune it down by way of a sysctl.  Ill
>>> have the code added and tested, if it turns out OK, (which i suspect it
>>> will) I will post the patch.
>>>
>>>
>> As promised, see attached patches. 1 for nfs.h, and 2 for nfs.c
>>
>> The nfs.h patch and 1 of the nfs.c patches
>> (nfs_settable_buffer_size.patch) are for the  HAVE_BLKSIZE define.
>>
>> The way we approached it is that if HAVE_BLKSIZE is defined, then an
>> extra parameter must be passed to nfs_init, specifying the size of the
>> block to use. If 0 is passed for this, it defaults to the best block
>> size (which seems to be 8K for us).  Any other value up to 8K sets the
>> block size to this (over 8K sets it to 8K).  Then the code changes in
>> nfs.c are also controller by the same #def.  The reason we did it this
>> way, is so that if this define is not used, the existing behaviour will
>> prevail.  If this is defined, then the new behaviour kicks in and the
>> block size must be specified when nfs is initialised.  We couldn't see
>> any value in it being able to be changed on the fly after
>> initialisation.  For debug purposes, it is a stored as a static
>> variable, and so could be changed by a debugger to test different block
>> sizes very easily.
>>
>> The second patch to nfs.c is in regard to the big pool which was only
>> being used  on a symlink and a write, it is equally useful for a read so
>> this patch allows  the pool to be big pool on a read, instead of small
>> pool as it does now.  I don't know if this makes things better, but it
>> certainly doesn't hurt (us anyway).
>>
>> Steven Johnson
>>
>> --- librtemsNfs.h    2006-06-10 04:44:03.000000000 +1000
>> +++ /tmp/librtemsNfs.h    2006-09-15 08:43:34.309866851 +1000
>> @@ -110,7 +110,11 @@
>>   *             driver chose reasonable defaults.
>>   */
>>  int
>> +#ifdef HAVE_BLKSIZE
>> +nfsInit(int smallPoolDepth, int bigPoolDepth, int bufferSize);
>> +#else
>>  nfsInit(int smallPoolDepth, int bigPoolDepth);
>> +#endif
>>
>>  /* Driver cleanup code
>>   *
>>
>> --- nfs.c    2006-06-10 04:44:03.000000000 +1000
>> +++ /tmp/nfs.c    2006-09-15 08:40:42.085342962 +1000
>> @@ -1075,6 +1075,7 @@
>>      switch (proc) {
>>          case NFSPROC_SYMLINK:
>>          case NFSPROC_WRITE:
>> +        case NFSPROC_READ:
>>                      pool = bigPool;        break;
>>          default:    pool = smallPool;    break;
>>      }
>>
>> --- nfs.c    2006-06-10 04:44:03.000000000 +1000
>> +++ /tmp/nfs.c    2006-09-15 09:11:13.327813709 +1000
>> @@ -182,6 +182,16 @@
>>  #define UNLOCK(s)    do { rtems_semaphore_release((s)); \
>>                      } while (0)
>>
>> +/*
>> +This nfs client gets its read buffer size from the fattr data from the
>> +NFS server it connects to. If HAVE_BLKSIZE is set then this is
>> +overridden and nfsInit takes a third parameter to set the buffer size.
>> +That parameter is stored in NFS_BUFFER_SIZE
>> +*/                   
>> +#ifdef HAVE_BLKSIZE
>> +static int NFS_BUFFER_SIZE;
>> +#endif
>> +                   
>>  /*****************************************
>>      Types with Associated XDR Routines
>>   *****************************************/
>> @@ -930,7 +940,11 @@
>>   *             on the fly).
>>   */
>>  void
>> +#ifdef HAVE_BLKSIZE
>> +nfsInit(int smallPoolDepth, int bigPoolDepth, int bufferSize)
>> +#else
>>  nfsInit(int smallPoolDepth, int bigPoolDepth)
>> +#endif
>>  {
>>  entry    dummy;
>>
>> @@ -938,7 +952,14 @@
>>      fprintf(stderr,"($Id: nfs.c,v 1.2 2006/06/09 18:44:03 ericn Exp 
>> $)\n\n");
>>      fprintf(stderr,"Till Straumann, Stanford/SLAC/SSRL 2002\n");
>>      fprintf(stderr,"See LICENSE file for licensing info\n");
>> -
>> +   
>> +#ifdef HAVE_BLKSIZE
>> +    if ((bufferSize > 0) && (bufferSize < NFS_MAXDATA)) {
>> +        NFS_BUFFER_SIZE = bufferSize;
>> +    } else {
>> +        NFS_BUFFER_SIZE = NFS_MAXDATA;
>> +    }
>> +#endif
>>      /* Get a major number */
>>
>>      if (RTEMS_SUCCESSFUL != rtems_io_register_driver(0, &drvNfs, 
>> &nfsGlob.nfs_major)) {
>> @@ -2834,7 +2855,11 @@
>>      buf->st_uid        = fa->uid;
>>      buf->st_gid        = fa->gid;
>>      buf->st_size    = fa->size;
>> +#ifdef HAVE_BLKSIZE
>> +    buf->st_blksize = NFS_BUFFER_SIZE;
>> +#else   
>>      /* TODO: set to "preferred size" of this NFS client 
>> implementation */
>>      buf->st_blksize    = fa->blocksize;
>> +#endif
>>      buf->st_rdev    = fa->rdev;
>>      buf->st_blocks    = fa->blocks;
>>
>
> --Eric Norum <norume at aps.anl.gov>
> Advanced Photon Source
> Argonne National Laboratory
> (630) 252-4793
>
>