NFS Performance

[Eric Norum]

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.

May I suggest instead that:
1) All the ifdefs be removed fromt the nfs code.
2) A new function be added:
   nfsInitWithBufferSize(int smallPoolDepth, int bigPoolDepth, int  
bufferSize);

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