2012-01-30 22:42:48

by J. Bruce Fields

[permalink] [raw]
Subject: [PATCH 1/2] nfsd: cleanup setting of default max_block_size

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(-)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index eda7d7e..2ad5ffe 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -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.
+ */
+ target <<= 12;
+
+ 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)
--
1.7.5.4



2012-01-31 01:15:19

by Mi Jinlong

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfsd: cleanup setting of default max_block_size



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(-)
>
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index eda7d7e..2ad5ffe 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -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.
> + */
> + 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);

thanks,
Mi Jinlong

> +
> + 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)


2012-01-30 22:47:12

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 2/2] nfsd: fix default iosize calculation on 32bit

On Mon, Jan 30, 2012 at 05:42:45PM -0500, J. Bruce Fields wrote:
> 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.

I could have sworm this problem had already been fixed, or at least
discussed, but can't find any reference now.

--b.

>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfsd/nfssvc.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 2ad5ffe..53c89f7 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -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
> --
> 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

2012-01-30 22:42:47

by J. Bruce Fields

[permalink] [raw]
Subject: [PATCH 2/2] nfsd: fix default iosize calculation on 32bit

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(-)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 2ad5ffe..53c89f7 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -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
--
1.7.5.4


2012-02-04 12:14:22

by Kinglong Mee

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfsd: cleanup setting of default max_block_size

于 2012-2-4 10:16, J. Bruce Fields 写道:
> On Sat, Feb 04, 2012 at 12:24:31AM +0800, Mi Jinlong wrote:
>> 于 2012-2-4 4:49, J. Bruce Fields 写道:
>>> On Wed, Feb 01, 2012 at 04:46:56PM -0500, bfields wrote:
>>>> On Tue, Jan 31, 2012 at 09:11:48AM +0800, Mi Jinlong wrote:
>>>>> 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....
>>>
>>> Here are the fixed patches.--b.
>>>
>>> > 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(-)
>>>
>>> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
>>> index eda7d7e..e9eb408 100644
>>> --- a/fs/nfsd/nfssvc.c
>>> +++ b/fs/nfsd/nfssvc.c
>>> @@ -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;
>>> + /*
>>> + * 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.

Yes,

>
>> 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?

Yes, that is my only concern.

thanks,
Mi Jinlong


2012-02-04 02:16:41

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfsd: cleanup setting of default max_block_size

On Sat, Feb 04, 2012 at 12:24:31AM +0800, Mi Jinlong wrote:
> 于 2012-2-4 4:49, J. Bruce Fields 写道:
> >On Wed, Feb 01, 2012 at 04:46:56PM -0500, bfields wrote:
> >>On Tue, Jan 31, 2012 at 09:11:48AM +0800, Mi Jinlong wrote:
> >>> 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....
> >
> >Here are the fixed patches.--b.
> >
> >>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(-)
> >
> >diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> >index eda7d7e..e9eb408 100644
> >--- a/fs/nfsd/nfssvc.c
> >+++ b/fs/nfsd/nfssvc.c
> >@@ -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;
> >+ /*
> >+ * 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.

> 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?

--b.

> Is there some other reason ?
>
> thanks,
> Mi Jinlong
>
>
> >+
> >+ 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)
>

2012-02-03 20:49:34

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfsd: cleanup setting of default max_block_size

On Wed, Feb 01, 2012 at 04:46:56PM -0500, bfields wrote:
> On Tue, Jan 31, 2012 at 09:11:48AM +0800, Mi Jinlong wrote:
> > 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....

Here are the fixed patches.--b.

>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(-)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index eda7d7e..e9eb408 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -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;
+ /*
+ * 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;
+
+ 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)
--
1.7.5.4


>From 508f92275624fc755104b17945bdc822936f1918 Mon Sep 17 00:00:00 2001
From: "J. Bruce Fields" <[email protected]>
Date: Mon, 30 Jan 2012 16:21:11 -0500
Subject: [PATCH 2/2] nfsd: fix default iosize calculation on 32bit

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(-)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index e9eb408..aacf1f4 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -314,7 +314,7 @@ static int nfsd_get_default_max_blksize(void)
unsigned long ret;

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
--
1.7.5.4


2012-02-01 21:46:58

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfsd: cleanup setting of default max_block_size

On Tue, Jan 31, 2012 at 09:11:48AM +0800, Mi Jinlong wrote:
>
>
> 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(-)
> >
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index eda7d7e..2ad5ffe 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -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.
> > + */
> > + 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);

Yes, thanks for catching that.

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

I'll fix that....

--b.

2012-02-04 00:24:38

by Kinglong Mee

[permalink] [raw]
Subject: Re: [PATCH 1/2] nfsd: cleanup setting of default max_block_size

于 2012-2-4 4:49, J. Bruce Fields 写道:
> On Wed, Feb 01, 2012 at 04:46:56PM -0500, bfields wrote:
>> On Tue, Jan 31, 2012 at 09:11:48AM +0800, Mi Jinlong wrote:
>>> 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....
>
> Here are the fixed patches.--b.
>
>> 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(-)
>
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index eda7d7e..e9eb408 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -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;
> + /*
> + * 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


> +
> + 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)