nfsd: cleanup setting of default max_block_size

From: "J. Bruce Fields" <[email protected]>

The rpc buffers will be allocated out of low memory, so we should really only be taking that into account.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfssvc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/nfsd/nfssvc.c +1 -1

index 2ad5ffe..53c89f7 100644
@@ -314,7 +314,7 @@ static int nfsd_get_default_max_blksize(void)
 	unsigned long bytes;
 
 	si_meminfo(&i);
-	target = i.totalram << PAGE_SHIFT;
+	target = (i.totalram - i.totalhigh) << PAGE_SHIFT;
 	/*
 	 * Aim for 1/4096 of memory per thread This gives 1MB on 4Gig
 	 * machines, but only uses 32K on 128M machines.  Bottom out at

Signature · 6 lines

1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: "J. Bruce Fields" <[email protected]>

Move calculation of the default into a helper function.

Get rid of an unused variable "err" while we're there.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfssvc.c |   44 ++++++++++++++++++++++++--------------------
 1 files changed, 24 insertions(+), 20 deletions(-)

fs/nfsd/nfssvc.c +3 -2

index eda7d7e..2ad5ffe 100644
@@ -307,33 +307,37 @@ static void set_max_drc(void)
 	dprintk("%s nfsd_drc_max_mem %u \n", __func__, nfsd_drc_max_mem);
 }
 
-int nfsd_create_serv(void)
+static int nfsd_get_default_max_blksize(void)
 {
-	int err = 0;
+	struct sysinfo i;
+	unsigned long target;

fs/nfsd/nfssvc.c +11 -18

+
+	bytes = NFSSVC_MAXBLKSIZE;
+	while (bytes > target && bytes >= 8*1024*2)
+		bytes /= 2;
+	return bytes;
+}
 
+int nfsd_create_serv(void)
+{
 	WARN_ON(!mutex_is_locked(&nfsd_mutex));
 	if (nfsd_serv) {
 		svc_get(nfsd_serv);
 		return 0;
 	}
-	if (nfsd_max_blksize == 0) {
-		/* choose a suitable default */
-		struct sysinfo i;
-		si_meminfo(&i);
-		/* Aim for 1/4096 of memory per thread
-		 * This gives 1MB on 4Gig machines
-		 * But only uses 32K on 128M machines.
-		 * Bottom out at 8K on 32M and smaller.
-		 * Of course, this is only a default.
-		 */
-		nfsd_max_blksize = NFSSVC_MAXBLKSIZE;
-		i.totalram <<= PAGE_SHIFT - 12;
-		while (nfsd_max_blksize > i.totalram &&
-		       nfsd_max_blksize >= 8*1024*2)
-			nfsd_max_blksize /= 2;
-	}
+	if (nfsd_max_blksize == 0)
+		nfsd_max_blksize = nfsd_get_default_max_blksize();
 	nfsd_reset_versions();
-
 	nfsd_serv = svc_create_pooled(&nfsd_program, nfsd_max_blksize,
 				      nfsd_last_thread, nfsd, THIS_MODULE);
 	if (nfsd_serv == NULL)

@@ -341,7 +345,7 @@ int nfsd_create_serv(void)
 
 	set_max_drc();
 	do_gettimeofday(&nfssvc_boot);		/* record boot time */
-	return err;
+	return 0;
 }
 
 int nfsd_nrpools(void)

Quoted 33 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Move calculation of the default into a helper function.

Get rid of an unused variable "err" while we're there.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfssvc.c |   44 ++++++++++++++++++++++++--------------------
 1 files changed, 24 insertions(+), 20 deletions(-)

fs/nfsd/nfssvc.c +11 -2

index eda7d7e..2ad5ffe 100644
@@ -307,33 +307,37 @@ static void set_max_drc(void)
 	dprintk("%s nfsd_drc_max_mem %u ¥n", __func__, nfsd_drc_max_mem);
 }
 
-int nfsd_create_serv(void)
+static int nfsd_get_default_max_blksize(void)
 {
-	int err = 0;
+	struct sysinfo i;
+	unsigned long target;
+	unsigned long bytes;
+
+	si_meminfo(&i);
+	target = i.totalram << PAGE_SHIFT;
+	/*
+	 * Aim for 1/4096 of memory per thread This gives 1MB on 4Gig
+	 * machines, but only uses 32K on 128M machines.  Bottom out at
+	 * 8K on 32M and smaller.  Of course, this is only a default.

fs/nfsd/nfssvc.c static void set_max_drc(void) · line 323

Quoted 2 lines

fs/nfsd/nfssvc.c +2 -0

+	 */
+	target <<= 12;

  Should use target = i.totalram << (PAGE_SHIFT - 12);

  target = i.totalram << PAGE_SHIFT; and 
  target <<= 12;
  means target = i.totalram << (PAGE_SHIFT + 12);

Quoted 5 lines · Mi Jinlong

  Should use target = i.totalram << (PAGE_SHIFT - 12);

  target = i.totalram << PAGE_SHIFT; and 
  target <<= 12;
  means target = i.totalram << (PAGE_SHIFT + 12);

Yes, thanks for catching that.

Also, splitting up the calculation as I did above risks overflow at the first step.

I'll fix that....

From 87b0fc7deb5feccf93b022f6a976e8441152dbb2 Mon Sep 17 00:00:00 2001

From 508f92275624fc755104b17945bdc822936f1918 Mon Sep 17 00:00:00 2001

Quoted 7 lines · bfields

Quoted 5 lines · Mi Jinlong

   Should use target = i.totalram<<  (PAGE_SHIFT - 12);

   target = i.totalram<<  PAGE_SHIFT; and
   target<<= 12;
   means target = i.totalram<<  (PAGE_SHIFT + 12);

Quoted 6 lines

Yes, thanks for catching that.

Also, splitting up the calculation as I did above risks overflow at the first step.

I'll fix that....

Here are the fixed patches.--b.

Quoted 1 line

From 87b0fc7deb5feccf93b022f6a976e8441152dbb2 Mon Sep 17 00:00:00 2001

From: "J. Bruce Fields"<[email protected]>
Date: Mon, 30 Jan 2012 16:18:35 -0500
Subject: [PATCH 1/2] nfsd: cleanup setting of default max_block_size

Move calculation of the default into a helper function.

Get rid of an unused variable "err" while we're there.

Thanks to Mi Jinlong for catching an arithmetic error in a previous version.

Cc Mi Jinlong<[email protected]>

Signed-off-by J. Bruce Fields<[email protected]>

---
  fs/nfsd/nfssvc.c |   44 ++++++++++++++++++++++++--------------------
  1 files changed, 24 insertions(+), 20 deletions(-)

fs/nfsd/nfssvc.c +3 -2

index eda7d7e..e9eb408 100644
@@ -307,33 +307,37 @@ static void set_max_drc(void)
  	dprintk("%s nfsd_drc_max_mem %u \n", __func__, nfsd_drc_max_mem);
  }
 
-int nfsd_create_serv(void)
+static int nfsd_get_default_max_blksize(void)
  {
-	int err = 0;
+	struct sysinfo i;
+	unsigned long long target;

fs/nfsd/nfssvc.c +11 -18

+
+	ret = NFSSVC_MAXBLKSIZE;
+	while (ret>  target&&  ret>= 8*1024*2)
+		ret /= 2;
+	return ret;
+}
 
+int nfsd_create_serv(void)
+{
  	WARN_ON(!mutex_is_locked(&nfsd_mutex));
  	if (nfsd_serv) {
  		svc_get(nfsd_serv);
  		return 0;
  	}
-	if (nfsd_max_blksize == 0) {
-		/* choose a suitable default */
-		struct sysinfo i;
-		si_meminfo(&i);
-		/* Aim for 1/4096 of memory per thread
-		 * This gives 1MB on 4Gig machines
-		 * But only uses 32K on 128M machines.
-		 * Bottom out at 8K on 32M and smaller.
-		 * Of course, this is only a default.
-		 */
-		nfsd_max_blksize = NFSSVC_MAXBLKSIZE;
-		i.totalram<<= PAGE_SHIFT - 12;
-		while (nfsd_max_blksize>  i.totalram&&
-		       nfsd_max_blksize>= 8*1024*2)
-			nfsd_max_blksize /= 2;
-	}
+	if (nfsd_max_blksize == 0)
+		nfsd_max_blksize = nfsd_get_default_max_blksize();
  	nfsd_reset_versions();
-
  	nfsd_serv = svc_create_pooled(&nfsd_program, nfsd_max_blksize,
  				      nfsd_last_thread, nfsd, THIS_MODULE);
  	if (nfsd_serv == NULL)

@@ -341,7 +345,7 @@ int nfsd_create_serv(void)
 
  	set_max_drc();
  	do_gettimeofday(&nfssvc_boot);		/* record boot time */
-	return err;
+	return 0;
  }
 
  int nfsd_nrpools(void)

Quoted 53 lines · J. Bruce Fields

Quoted 7 lines · bfields

Quoted 5 lines · Mi Jinlong

  Should use target = i.totalram<<  (PAGE_SHIFT - 12);

  target = i.totalram<<  PAGE_SHIFT; and
  target<<= 12;
  means target = i.totalram<<  (PAGE_SHIFT + 12);

Quoted 6 lines

Yes, thanks for catching that.

Also, splitting up the calculation as I did above risks overflow at the first step.

I'll fix that....

Here are the fixed patches.--b.

Quoted 1 line

From 87b0fc7deb5feccf93b022f6a976e8441152dbb2 Mon Sep 17 00:00:00 2001

From: "J. Bruce Fields"<[email protected]>
Date: Mon, 30 Jan 2012 16:18:35 -0500
Subject: [PATCH 1/2] nfsd: cleanup setting of default max_block_size

Move calculation of the default into a helper function.

Get rid of an unused variable "err" while we're there.

Thanks to Mi Jinlong for catching an arithmetic error in a previous version.

Cc Mi Jinlong<[email protected]>

Signed-off-by J. Bruce Fields<[email protected]>

---
 fs/nfsd/nfssvc.c |   44 ++++++++++++++++++++++++--------------------
 1 files changed, 24 insertions(+), 20 deletions(-)

fs/nfsd/nfssvc.c +7 -2

index eda7d7e..e9eb408 100644
@@ -307,33 +307,37 @@ static void set_max_drc(void)
 	dprintk("%s nfsd_drc_max_mem %u \n", __func__, nfsd_drc_max_mem);
 }
 
-int nfsd_create_serv(void)
+static int nfsd_get_default_max_blksize(void)
 {
-	int err = 0;
+	struct sysinfo i;
+	unsigned long long target;
+	unsigned long ret;
+
+	si_meminfo(&i);
+	target = i.totalram<<  PAGE_SHIFT;

fs/nfsd/nfssvc.c static void set_max_drc(void)

Quoted 6 lines

fs/nfsd/nfssvc.c +6 -0

+	/*
+	 * Aim for 1/4096 of memory per thread This gives 1MB on 4Gig
+	 * machines, but only uses 32K on 128M machines.  Bottom out at
+	 * 8K on 32M and smaller.  Of course, this is only a default.
+	 */
+	target>>= 12;

Why don't using target = i.totalram << (PAGE_SHIFT - 12) as before?

The result of the two forms is more likely different.

Is there some other reason ?

thanks,
Mi Jinlong

Quoted 46 lines

fs/nfsd/nfssvc.c +11 -18

+
+	ret = NFSSVC_MAXBLKSIZE;
+	while (ret>  target&&  ret>= 8*1024*2)
+		ret /= 2;
+	return ret;
+}
 
+int nfsd_create_serv(void)
+{
 	WARN_ON(!mutex_is_locked(&nfsd_mutex));
 	if (nfsd_serv) {
 		svc_get(nfsd_serv);
 		return 0;
 	}
-	if (nfsd_max_blksize == 0) {
-		/* choose a suitable default */
-		struct sysinfo i;
-		si_meminfo(&i);
-		/* Aim for 1/4096 of memory per thread
-		 * This gives 1MB on 4Gig machines
-		 * But only uses 32K on 128M machines.
-		 * Bottom out at 8K on 32M and smaller.
-		 * Of course, this is only a default.
-		 */
-		nfsd_max_blksize = NFSSVC_MAXBLKSIZE;
-		i.totalram<<= PAGE_SHIFT - 12;
-		while (nfsd_max_blksize>  i.totalram&&
-		       nfsd_max_blksize>= 8*1024*2)
-			nfsd_max_blksize /= 2;
-	}
+	if (nfsd_max_blksize == 0)
+		nfsd_max_blksize = nfsd_get_default_max_blksize();
 	nfsd_reset_versions();
-
 	nfsd_serv = svc_create_pooled(&nfsd_program, nfsd_max_blksize,
 				      nfsd_last_thread, nfsd, THIS_MODULE);
 	if (nfsd_serv == NULL)

@@ -341,7 +345,7 @@ int nfsd_create_serv(void)
 
 	set_max_drc();
 	do_gettimeofday(&nfssvc_boot);		/* record boot time */
-	return err;
+	return 0;
 }
 
 int nfsd_nrpools(void)

Quoted 54 lines · Mi Jinlong

Quoted 53 lines · J. Bruce Fields

Quoted 7 lines · bfields

Quoted 5 lines · Mi Jinlong

   Should use target = i.totalram<<   (PAGE_SHIFT - 12);

   target = i.totalram<<   PAGE_SHIFT; and
   target<<= 12;
   means target = i.totalram<<   (PAGE_SHIFT + 12);

Quoted 6 lines

Yes, thanks for catching that.

Also, splitting up the calculation as I did above risks overflow at the first step.

I'll fix that....

Here are the fixed patches.--b.

Quoted 1 line

From 87b0fc7deb5feccf93b022f6a976e8441152dbb2 Mon Sep 17 00:00:00 2001

From: "J. Bruce Fields"<[email protected]>
Date: Mon, 30 Jan 2012 16:18:35 -0500
Subject: [PATCH 1/2] nfsd: cleanup setting of default max_block_size

Move calculation of the default into a helper function.

Get rid of an unused variable "err" while we're there.

Thanks to Mi Jinlong for catching an arithmetic error in a previous version.

Cc Mi Jinlong<[email protected]>

Signed-off-by J. Bruce Fields<[email protected]>

---
  fs/nfsd/nfssvc.c |   44 ++++++++++++++++++++++++--------------------
  1 files changed, 24 insertions(+), 20 deletions(-)

fs/nfsd/nfssvc.c +7 -2

index eda7d7e..e9eb408 100644
@@ -307,33 +307,37 @@ static void set_max_drc(void)
  	dprintk("%s nfsd_drc_max_mem %u \n", __func__, nfsd_drc_max_mem);
  }
 
-int nfsd_create_serv(void)
+static int nfsd_get_default_max_blksize(void)
  {
-	int err = 0;
+	struct sysinfo i;
+	unsigned long long target;
+	unsigned long ret;
+
+	si_meminfo(&i);
+	target = i.totalram<<   PAGE_SHIFT;

Quoted 8 lines

fs/nfsd/nfssvc.c static void set_max_drc(void)

Quoted 6 lines

fs/nfsd/nfssvc.c +6 -0

+	/*
+	 * Aim for 1/4096 of memory per thread This gives 1MB on 4Gig
+	 * machines, but only uses 32K on 128M machines.  Bottom out at
+	 * 8K on 32M and smaller.  Of course, this is only a default.
+	 */
+	target>>= 12;

Why don't using target = i.totalram<< (PAGE_SHIFT - 12) as before?

Dividing the calculation into two steps (first convert from pages to bytes, then divide by 4096) makes it more obvious, and allows me to stick the comment before the part of the calculation it explains.

So the result seems easier to read.

Quoted 2 lines

The result of the two forms is more likely different.

The result is the same as long as there's no overflow at the first step (which would require a machine with an exabyte of ram).

Seem reasonable?