linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v4] mm: introduce validity check on vm dirtiness settings
  2017-09-21 13:59 [PATCH v4] mm: introduce validity check on vm dirtiness settings Yafang Shao
@ 2017-09-21 11:52 ` Jan Kara
  0 siblings, 0 replies; 9+ messages in thread
From: Jan Kara @ 2017-09-21 11:52 UTC (permalink / raw)
  To: Yafang Shao
  Cc: jack, akpm, hannes, mhocko, vdavydov.dev, jlayton, nborisov,
	tytso, mawilcox, linux-mm, linux-kernel, mcgrof, keescook,
	wuqixuan

On Thu 21-09-17 21:59:52, Yafang Shao wrote:
> we can find the logic in domain_dirty_limits() that
> when dirty bg_thresh is bigger than dirty thresh,
> bg_thresh will be set as thresh * 1 / 2.
> 	if (bg_thresh >= thresh)
> 		bg_thresh = thresh / 2;
> 
> But actually we can set vm background dirtiness bigger than
> vm dirtiness successfully. This behavior may mislead us.
> We'd better do this validity check at the beginning.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Looks good. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

Just one nit below:

> +
> +    /* needn't do validity check if the value is not different. */
> +	if (ret == 0 && write && dirty_background_ratio != old_ratio) {

Whitespace before the comment is broken. Generally I don't think the
comment brings much so I'd just delete it.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4] mm: introduce validity check on vm dirtiness settings
@ 2017-09-21 13:59 Yafang Shao
  2017-09-21 11:52 ` Jan Kara
  0 siblings, 1 reply; 9+ messages in thread
From: Yafang Shao @ 2017-09-21 13:59 UTC (permalink / raw)
  To: jack
  Cc: akpm, hannes, mhocko, vdavydov.dev, jlayton, nborisov, tytso,
	mawilcox, linux-mm, linux-kernel, laoar.shao, mcgrof, keescook,
	wuqixuan

we can find the logic in domain_dirty_limits() that
when dirty bg_thresh is bigger than dirty thresh,
bg_thresh will be set as thresh * 1 / 2.
	if (bg_thresh >= thresh)
		bg_thresh = thresh / 2;

But actually we can set vm background dirtiness bigger than
vm dirtiness successfully. This behavior may mislead us.
We'd better do this validity check at the beginning.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 Documentation/sysctl/vm.txt |  6 ++++
 kernel/sysctl.c             |  4 +--
 mm/page-writeback.c         | 80 ++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 80 insertions(+), 10 deletions(-)

diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
index 9baf66a..0bab85d 100644
--- a/Documentation/sysctl/vm.txt
+++ b/Documentation/sysctl/vm.txt
@@ -156,6 +156,9 @@ read.
 Note: the minimum value allowed for dirty_bytes is two pages (in bytes); any
 value lower than this limit will be ignored and the old configuration will be
 retained.
+Note: the value of dirty_bytes also cannot be set lower than
+dirty_background_bytes or the amount of memory corresponding to
+dirty_background_ratio.
 
 ==============================================================
 
@@ -176,6 +179,9 @@ generating disk writes will itself start writing out dirty data.
 
 The total available memory is not equal to total system memory.
 
+Note: dirty_ratio cannot be set lower than dirty_background_ratio or
+ratio corresponding to dirty_background_bytes.
+
 ==============================================================
 
 dirty_writeback_centisecs
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6648fbb..7b525cf 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1293,7 +1293,7 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
 		.maxlen		= sizeof(dirty_background_ratio),
 		.mode		= 0644,
 		.proc_handler	= dirty_background_ratio_handler,
-		.extra1		= &zero,
+		.extra1		= &one,
 		.extra2		= &one_hundred,
 	},
 	{
@@ -1310,7 +1310,7 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
 		.maxlen		= sizeof(vm_dirty_ratio),
 		.mode		= 0644,
 		.proc_handler	= dirty_ratio_handler,
-		.extra1		= &zero,
+		.extra1		= &one,
 		.extra2		= &one_hundred,
 	},
 	{
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 0b9c5cb..8dfd222 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -511,15 +511,61 @@ bool node_dirty_ok(struct pglist_data *pgdat)
 	return nr_pages <= limit;
 }
 
+static bool vm_dirty_settings_valid(void)
+{
+	bool ret = true;
+	unsigned long bytes;
+
+	if (vm_dirty_ratio > 0) {
+		if (dirty_background_ratio >= vm_dirty_ratio) {
+			ret = false;
+			goto out;
+		}
+
+		bytes = global_dirtyable_memory() * PAGE_SIZE / 100 *
+				vm_dirty_ratio;
+		if (dirty_background_bytes >= bytes) {
+			ret = false;
+			goto out;
+		}
+	}
+
+	if (vm_dirty_bytes > 0) {
+		if (dirty_background_bytes >= vm_dirty_bytes) {
+			ret = false;
+			goto out;
+		}
+
+		bytes = global_dirtyable_memory() * PAGE_SIZE / 100 *
+				dirty_background_ratio;
+
+		if (bytes >= vm_dirty_bytes)
+			ret = false;
+	}
+
+out:
+	return ret;
+}
+
 int dirty_background_ratio_handler(struct ctl_table *table, int write,
 		void __user *buffer, size_t *lenp,
 		loff_t *ppos)
 {
 	int ret;
+	int old_ratio = dirty_background_ratio;
 
 	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
-	if (ret == 0 && write)
-		dirty_background_bytes = 0;
+
+    /* needn't do validity check if the value is not different. */
+	if (ret == 0 && write && dirty_background_ratio != old_ratio) {
+		if (vm_dirty_settings_valid())
+			dirty_background_bytes = 0;
+		else {
+			dirty_background_ratio = old_ratio;
+			ret = -EINVAL;
+		}
+	}
+
 	return ret;
 }
 
@@ -528,10 +574,18 @@ int dirty_background_bytes_handler(struct ctl_table *table, int write,
 		loff_t *ppos)
 {
 	int ret;
+	unsigned long old_bytes = dirty_background_bytes;
 
 	ret = proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
-	if (ret == 0 && write)
-		dirty_background_ratio = 0;
+	if (ret == 0 && write && dirty_background_bytes != old_bytes) {
+		if (vm_dirty_settings_valid())
+			dirty_background_ratio = 0;
+		else {
+			dirty_background_bytes = old_bytes;
+			ret = -EINVAL;
+		}
+	}
+
 	return ret;
 }
 
@@ -544,8 +598,13 @@ int dirty_ratio_handler(struct ctl_table *table, int write,
 
 	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 	if (ret == 0 && write && vm_dirty_ratio != old_ratio) {
-		writeback_set_ratelimit();
-		vm_dirty_bytes = 0;
+		if (vm_dirty_settings_valid()) {
+			writeback_set_ratelimit();
+			vm_dirty_bytes = 0;
+		} else {
+			vm_dirty_ratio = old_ratio;
+			ret = -EINVAL;
+		}
 	}
 	return ret;
 }
@@ -559,8 +618,13 @@ int dirty_bytes_handler(struct ctl_table *table, int write,
 
 	ret = proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
 	if (ret == 0 && write && vm_dirty_bytes != old_bytes) {
-		writeback_set_ratelimit();
-		vm_dirty_ratio = 0;
+		if (vm_dirty_settings_valid()) {
+			writeback_set_ratelimit();
+			vm_dirty_ratio = 0;
+		} else {
+			vm_dirty_bytes = old_bytes;
+			ret = -EINVAL;
+		}
 	}
 	return ret;
 }
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v4] mm: introduce validity check on vm dirtiness settings
@ 2017-09-21 23:12 Yafang Shao
  2017-09-26 23:59 ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Yafang Shao @ 2017-09-21 23:12 UTC (permalink / raw)
  To: akpm; +Cc: linux-mm, Yafang Shao

we can find the logic in domain_dirty_limits() that
when dirty bg_thresh is bigger than dirty thresh,
bg_thresh will be set as thresh * 1 / 2.
	if (bg_thresh >= thresh)
		bg_thresh = thresh / 2;

But actually we can set vm background dirtiness bigger than
vm dirtiness successfully. This behavior may mislead us.
We'd better do this validity check at the beginning.

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 Documentation/sysctl/vm.txt |  6 ++++
 kernel/sysctl.c             |  4 +--
 mm/page-writeback.c         | 78 ++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 78 insertions(+), 10 deletions(-)

diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
index 9baf66a..0bab85d 100644
--- a/Documentation/sysctl/vm.txt
+++ b/Documentation/sysctl/vm.txt
@@ -156,6 +156,9 @@ read.
 Note: the minimum value allowed for dirty_bytes is two pages (in bytes); any
 value lower than this limit will be ignored and the old configuration will be
 retained.
+Note: the value of dirty_bytes also cannot be set lower than
+dirty_background_bytes or the amount of memory corresponding to
+dirty_background_ratio.
 
 ==============================================================
 
@@ -176,6 +179,9 @@ generating disk writes will itself start writing out dirty data.
 
 The total available memory is not equal to total system memory.
 
+Note: dirty_ratio cannot be set lower than dirty_background_ratio or
+ratio corresponding to dirty_background_bytes.
+
 ==============================================================
 
 dirty_writeback_centisecs
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6648fbb..7b525cf 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1293,7 +1293,7 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
 		.maxlen		= sizeof(dirty_background_ratio),
 		.mode		= 0644,
 		.proc_handler	= dirty_background_ratio_handler,
-		.extra1		= &zero,
+		.extra1		= &one,
 		.extra2		= &one_hundred,
 	},
 	{
@@ -1310,7 +1310,7 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
 		.maxlen		= sizeof(vm_dirty_ratio),
 		.mode		= 0644,
 		.proc_handler	= dirty_ratio_handler,
-		.extra1		= &zero,
+		.extra1		= &one,
 		.extra2		= &one_hundred,
 	},
 	{
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 0b9c5cb..a0dad7b 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -511,15 +511,59 @@ bool node_dirty_ok(struct pglist_data *pgdat)
 	return nr_pages <= limit;
 }
 
+static bool vm_dirty_settings_valid(void)
+{
+	bool ret = true;
+	unsigned long bytes;
+
+	if (vm_dirty_ratio > 0) {
+		if (dirty_background_ratio >= vm_dirty_ratio) {
+			ret = false;
+			goto out;
+		}
+
+		bytes = global_dirtyable_memory() * PAGE_SIZE / 100 *
+				vm_dirty_ratio;
+		if (dirty_background_bytes >= bytes) {
+			ret = false;
+			goto out;
+		}
+	}
+
+	if (vm_dirty_bytes > 0) {
+		if (dirty_background_bytes >= vm_dirty_bytes) {
+			ret = false;
+			goto out;
+		}
+
+		bytes = global_dirtyable_memory() * PAGE_SIZE / 100 *
+				dirty_background_ratio;
+
+		if (bytes >= vm_dirty_bytes)
+			ret = false;
+	}
+
+out:
+	return ret;
+}
+
 int dirty_background_ratio_handler(struct ctl_table *table, int write,
 		void __user *buffer, size_t *lenp,
 		loff_t *ppos)
 {
 	int ret;
+	int old_ratio = dirty_background_ratio;
 
 	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
-	if (ret == 0 && write)
-		dirty_background_bytes = 0;
+	if (ret == 0 && write && dirty_background_ratio != old_ratio) {
+		if (vm_dirty_settings_valid())
+			dirty_background_bytes = 0;
+		else {
+			dirty_background_ratio = old_ratio;
+			ret = -EINVAL;
+		}
+	}
+
 	return ret;
 }
 
@@ -528,10 +572,18 @@ int dirty_background_bytes_handler(struct ctl_table *table, int write,
 		loff_t *ppos)
 {
 	int ret;
+	unsigned long old_bytes = dirty_background_bytes;
 
 	ret = proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
-	if (ret == 0 && write)
-		dirty_background_ratio = 0;
+	if (ret == 0 && write && dirty_background_bytes != old_bytes) {
+		if (vm_dirty_settings_valid())
+			dirty_background_ratio = 0;
+		else {
+			dirty_background_bytes = old_bytes;
+			ret = -EINVAL;
+		}
+	}
+
 	return ret;
 }
 
@@ -544,8 +596,13 @@ int dirty_ratio_handler(struct ctl_table *table, int write,
 
 	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 	if (ret == 0 && write && vm_dirty_ratio != old_ratio) {
-		writeback_set_ratelimit();
-		vm_dirty_bytes = 0;
+		if (vm_dirty_settings_valid()) {
+			writeback_set_ratelimit();
+			vm_dirty_bytes = 0;
+		} else {
+			vm_dirty_ratio = old_ratio;
+			ret = -EINVAL;
+		}
 	}
 	return ret;
 }
@@ -559,8 +616,13 @@ int dirty_bytes_handler(struct ctl_table *table, int write,
 
 	ret = proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
 	if (ret == 0 && write && vm_dirty_bytes != old_bytes) {
-		writeback_set_ratelimit();
-		vm_dirty_ratio = 0;
+		if (vm_dirty_settings_valid()) {
+			writeback_set_ratelimit();
+			vm_dirty_ratio = 0;
+		} else {
+			vm_dirty_bytes = old_bytes;
+			ret = -EINVAL;
+		}
 	}
 	return ret;
 }
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] mm: introduce validity check on vm dirtiness settings
  2017-09-21 23:12 Yafang Shao
@ 2017-09-26 23:59 ` Andrew Morton
  2017-09-27  1:38   ` Yafang Shao
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2017-09-26 23:59 UTC (permalink / raw)
  To: Yafang Shao; +Cc: linux-mm

On Fri, 22 Sep 2017 07:12:32 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:

> we can find the logic in domain_dirty_limits() that
> when dirty bg_thresh is bigger than dirty thresh,
> bg_thresh will be set as thresh * 1 / 2.
> 	if (bg_thresh >= thresh)
> 		bg_thresh = thresh / 2;
> 
> But actually we can set vm background dirtiness bigger than
> vm dirtiness successfully. This behavior may mislead us.
> We'd better do this validity check at the beginning.
> 
> ...
>
> --- a/Documentation/sysctl/vm.txt
> +++ b/Documentation/sysctl/vm.txt
> @@ -156,6 +156,9 @@ read.
>  Note: the minimum value allowed for dirty_bytes is two pages (in bytes); any
>  value lower than this limit will be ignored and the old configuration will be
>  retained.
> +Note: the value of dirty_bytes also cannot be set lower than
> +dirty_background_bytes or the amount of memory corresponding to
> +dirty_background_ratio.

I think this means that a script which alters both dirty_bytes and
dirty_background_bytes must alter dirty_background_bytes first if they
are being decreased and must alter dirty_bytes first if they are being
increased.  Or something like that.

And existing scripts which do not do this will cease to work correctly,
no?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] mm: introduce validity check on vm dirtiness settings
  2017-09-26 23:59 ` Andrew Morton
@ 2017-09-27  1:38   ` Yafang Shao
  2017-09-27  2:54     ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Yafang Shao @ 2017-09-27  1:38 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm

2017-09-27 7:59 GMT+08:00 Andrew Morton <akpm@linux-foundation.org>:
> On Fri, 22 Sep 2017 07:12:32 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
>
>> we can find the logic in domain_dirty_limits() that
>> when dirty bg_thresh is bigger than dirty thresh,
>> bg_thresh will be set as thresh * 1 / 2.
>>       if (bg_thresh >= thresh)
>>               bg_thresh = thresh / 2;
>>
>> But actually we can set vm background dirtiness bigger than
>> vm dirtiness successfully. This behavior may mislead us.
>> We'd better do this validity check at the beginning.
>>
>> ...
>>
>> --- a/Documentation/sysctl/vm.txt
>> +++ b/Documentation/sysctl/vm.txt
>> @@ -156,6 +156,9 @@ read.
>>  Note: the minimum value allowed for dirty_bytes is two pages (in bytes); any
>>  value lower than this limit will be ignored and the old configuration will be
>>  retained.
>> +Note: the value of dirty_bytes also cannot be set lower than
>> +dirty_background_bytes or the amount of memory corresponding to
>> +dirty_background_ratio.
>
> I think this means that a script which alters both dirty_bytes and
> dirty_background_bytes must alter dirty_background_bytes first if they
> are being decreased and must alter dirty_bytes first if they are being
> increased.  Or something like that.
>

Yes.

> And existing scripts which do not do this will cease to work correctly,
> no?
>

The existing scritpts won't work correctly. That's also what I have
worried before.

But under this condition, there's a error message generated by "sysctl
-w" to tell them the first setting was failure.
This error message may be a reminder to them that there are some
connections between background and direct limit, and should not set
arbitrary.
May that's better. I'm not sure.

Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] mm: introduce validity check on vm dirtiness settings
  2017-09-27  1:38   ` Yafang Shao
@ 2017-09-27  2:54     ` Andrew Morton
  2017-09-27  4:14       ` Yafang Shao
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2017-09-27  2:54 UTC (permalink / raw)
  To: Yafang Shao; +Cc: linux-mm

On Wed, 27 Sep 2017 09:38:21 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:

> > And existing scripts which do not do this will cease to work correctly,
> > no?
> >
> 
> The existing scritpts won't work correctly. That's also what I have
> worried before.
> 
> But under this condition, there's a error message generated by "sysctl
> -w" to tell them the first setting was failure.
> This error message may be a reminder to them that there are some
> connections between background and direct limit, and should not set
> arbitrary.
> May that's better. I'm not sure.

Maybe we can leave the logic as-is and simply print a warning when an
illogical state exists.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] mm: introduce validity check on vm dirtiness settings
  2017-09-27  2:54     ` Andrew Morton
@ 2017-09-27  4:14       ` Yafang Shao
  2017-09-27 19:33         ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Yafang Shao @ 2017-09-27  4:14 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm

2017-09-27 10:54 GMT+08:00 Andrew Morton <akpm@linux-foundation.org>:
> On Wed, 27 Sep 2017 09:38:21 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
>
>> > And existing scripts which do not do this will cease to work correctly,
>> > no?
>> >
>>
>> The existing scritpts won't work correctly. That's also what I have
>> worried before.
>>
>> But under this condition, there's a error message generated by "sysctl
>> -w" to tell them the first setting was failure.
>> This error message may be a reminder to them that there are some
>> connections between background and direct limit, and should not set
>> arbitrary.
>> May that's better. I'm not sure.
>
> Maybe we can leave the logic as-is and simply print a warning when an
> illogical state exists.
>

You mean, just modified the  code as bellow ?
in function  domain_dirty_limits()
-        if (bg_thresh >= thresh)
+     if (bg_thresh >= thresh) {
+        pr_warn("vm direct limit should greater than background limit.\n");
          bg_thresh = thresh / 2;
+     }

will this generate lots of log ?

Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] mm: introduce validity check on vm dirtiness settings
  2017-09-27  4:14       ` Yafang Shao
@ 2017-09-27 19:33         ` Andrew Morton
  2017-09-28  1:27           ` Yafang Shao
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2017-09-27 19:33 UTC (permalink / raw)
  To: Yafang Shao; +Cc: linux-mm

On Wed, 27 Sep 2017 12:14:10 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:

> 2017-09-27 10:54 GMT+08:00 Andrew Morton <akpm@linux-foundation.org>:
> > On Wed, 27 Sep 2017 09:38:21 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> >> > And existing scripts which do not do this will cease to work correctly,
> >> > no?
> >> >
> >>
> >> The existing scritpts won't work correctly. That's also what I have
> >> worried before.
> >>
> >> But under this condition, there's a error message generated by "sysctl
> >> -w" to tell them the first setting was failure.
> >> This error message may be a reminder to them that there are some
> >> connections between background and direct limit, and should not set
> >> arbitrary.
> >> May that's better. I'm not sure.
> >
> > Maybe we can leave the logic as-is and simply print a warning when an
> > illogical state exists.
> >
> 
> You mean, just modified the  code as bellow ?
> in function  domain_dirty_limits()
> -        if (bg_thresh >= thresh)
> +     if (bg_thresh >= thresh) {
> +        pr_warn("vm direct limit should greater than background limit.\n");
>           bg_thresh = thresh / 2;
> +     }

Something like that.

> will this generate lots of log ?

Well, it's one message per write to a procfs file, when that write
causes an errant state.  Sounds manageable?  It would be nice if we
could somehow help the operator to figure out that writing in a
different order will prevent the incorrect state (and hence the
warning).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] mm: introduce validity check on vm dirtiness settings
  2017-09-27 19:33         ` Andrew Morton
@ 2017-09-28  1:27           ` Yafang Shao
  0 siblings, 0 replies; 9+ messages in thread
From: Yafang Shao @ 2017-09-28  1:27 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm

Got it.
I will submit a new patch then.

Thanks
Yafang

2017-09-28 3:33 GMT+08:00 Andrew Morton <akpm@linux-foundation.org>:
> On Wed, 27 Sep 2017 12:14:10 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
>
>> 2017-09-27 10:54 GMT+08:00 Andrew Morton <akpm@linux-foundation.org>:
>> > On Wed, 27 Sep 2017 09:38:21 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
>> >
>> >> > And existing scripts which do not do this will cease to work correctly,
>> >> > no?
>> >> >
>> >>
>> >> The existing scritpts won't work correctly. That's also what I have
>> >> worried before.
>> >>
>> >> But under this condition, there's a error message generated by "sysctl
>> >> -w" to tell them the first setting was failure.
>> >> This error message may be a reminder to them that there are some
>> >> connections between background and direct limit, and should not set
>> >> arbitrary.
>> >> May that's better. I'm not sure.
>> >
>> > Maybe we can leave the logic as-is and simply print a warning when an
>> > illogical state exists.
>> >
>>
>> You mean, just modified the  code as bellow ?
>> in function  domain_dirty_limits()
>> -        if (bg_thresh >= thresh)
>> +     if (bg_thresh >= thresh) {
>> +        pr_warn("vm direct limit should greater than background limit.\n");
>>           bg_thresh = thresh / 2;
>> +     }
>
> Something like that.
>
>> will this generate lots of log ?
>
> Well, it's one message per write to a procfs file, when that write
> causes an errant state.  Sounds manageable?  It would be nice if we
> could somehow help the operator to figure out that writing in a
> different order will prevent the incorrect state (and hence the
> warning).
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-09-28  1:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-21 13:59 [PATCH v4] mm: introduce validity check on vm dirtiness settings Yafang Shao
2017-09-21 11:52 ` Jan Kara
  -- strict thread matches above, loose matches on Subject: below --
2017-09-21 23:12 Yafang Shao
2017-09-26 23:59 ` Andrew Morton
2017-09-27  1:38   ` Yafang Shao
2017-09-27  2:54     ` Andrew Morton
2017-09-27  4:14       ` Yafang Shao
2017-09-27 19:33         ` Andrew Morton
2017-09-28  1:27           ` Yafang Shao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).