* [PATCH] mm: introduce sanity check on dirty ratio sysctl value
@ 2017-09-17 17:39 Yafang Shao
2017-09-18 10:22 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: Yafang Shao @ 2017-09-17 17:39 UTC (permalink / raw)
To: akpm, jack, hannes, mhocko, vdavydov.dev, jlayton, nborisov,
tytso, mawilcox
Cc: linux-mm, linux-kernel, laoar.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 dirty_background_raio bigger than
dirty_ratio successfully. This behavior may mislead us.
So we should do this sanity check at the beginning.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
Documentation/sysctl/vm.txt | 5 +++
mm/page-writeback.c | 84 ++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 81 insertions(+), 8 deletions(-)
diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
index 9baf66a..b87e238 100644
--- a/Documentation/sysctl/vm.txt
+++ b/Documentation/sysctl/vm.txt
@@ -156,6 +156,8 @@ 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.
+dirty_bytes can't less than dirty_background_bytes or
+dirty_ratio * available_memory / 100.
==============================================================
@@ -176,6 +178,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 can't less than dirty_background_ratio or
+dirty_background_bytes / available_memory * 100.
+
==============================================================
dirty_writeback_centisecs
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 0b9c5cb..1dcb8f7 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -515,11 +515,29 @@ int dirty_background_ratio_handler(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp,
loff_t *ppos)
{
+ int old_ratio = dirty_background_ratio;
+ unsigned long bytes;
int ret;
ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
- if (ret == 0 && write)
- dirty_background_bytes = 0;
+
+ if (ret == 0 && write) {
+ if (vm_dirty_ratio > 0) {
+ if (dirty_background_ratio >= vm_dirty_ratio)
+ ret = -EINVAL;
+ } else if (vm_dirty_bytes > 0) {
+ bytes = global_dirtyable_memory() * PAGE_SIZE *
+ dirty_background_ratio / 100;
+ if (bytes >= vm_dirty_bytes)
+ ret = -EINVAL;
+ }
+
+ if (ret == 0)
+ dirty_background_bytes = 0;
+ else
+ dirty_background_ratio = old_ratio;
+ }
+
return ret;
}
@@ -527,11 +545,29 @@ int dirty_background_bytes_handler(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp,
loff_t *ppos)
{
+ unsigned long old_bytes = dirty_background_bytes;
+ unsigned long bytes;
int ret;
ret = proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
- if (ret == 0 && write)
- dirty_background_ratio = 0;
+
+ if (ret == 0 && write) {
+ if (vm_dirty_bytes > 0) {
+ if (dirty_background_bytes >= vm_dirty_bytes)
+ ret = -EINVAL;
+ } else if (vm_dirty_ratio > 0) {
+ bytes = global_dirtyable_memory() * PAGE_SIZE *
+ vm_dirty_ratio / 100;
+ if (dirty_background_bytes >= bytes)
+ ret = -EINVAL;
+ }
+
+ if (ret == 0)
+ dirty_background_ratio = 0;
+ else
+ dirty_background_bytes = old_bytes;
+ }
+
return ret;
}
@@ -540,13 +576,29 @@ int dirty_ratio_handler(struct ctl_table *table, int write,
loff_t *ppos)
{
int old_ratio = vm_dirty_ratio;
+ unsigned long bytes;
int ret;
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 (dirty_background_ratio > 0) {
+ if (vm_dirty_ratio <= dirty_background_ratio)
+ ret = -EINVAL;
+ } else if (dirty_background_bytes > 0) {
+ bytes = global_dirtyable_memory() * PAGE_SIZE *
+ vm_dirty_ratio / 100;
+ if (bytes <= dirty_background_bytes)
+ ret = -EINVAL;
+ }
+
+ if (ret == 0) {
+ writeback_set_ratelimit();
+ vm_dirty_bytes = 0;
+ } else
+ vm_dirty_ratio = old_ratio;
}
+
return ret;
}
@@ -555,13 +607,29 @@ int dirty_bytes_handler(struct ctl_table *table, int write,
loff_t *ppos)
{
unsigned long old_bytes = vm_dirty_bytes;
+ unsigned long bytes;
int ret;
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 (dirty_background_ratio > 0) {
+ bytes = global_dirtyable_memory() * PAGE_SIZE *
+ dirty_background_ratio / 100;
+ if (vm_dirty_bytes <= bytes)
+ ret = -EINVAL;
+ } else if (dirty_background_bytes > 0) {
+ if (vm_dirty_bytes <= dirty_background_bytes)
+ ret = -EINVAL;
+ }
+
+ if (ret == 0) {
+ writeback_set_ratelimit();
+ vm_dirty_ratio = 0;
+ } else
+ vm_dirty_bytes = old_bytes;
}
+
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] 3+ messages in thread* Re: [PATCH] mm: introduce sanity check on dirty ratio sysctl value
2017-09-17 17:39 [PATCH] mm: introduce sanity check on dirty ratio sysctl value Yafang Shao
@ 2017-09-18 10:22 ` Jan Kara
2017-09-18 11:36 ` Yafang Shao
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2017-09-18 10:22 UTC (permalink / raw)
To: Yafang Shao
Cc: akpm, jack, hannes, mhocko, vdavydov.dev, jlayton, nborisov,
tytso, mawilcox, linux-mm, linux-kernel
On Mon 18-09-17 01:39:28, 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 dirty_background_raio bigger than
> dirty_ratio successfully. This behavior may mislead us.
> So we should do this sanity check at the beginning.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
...
> {
> + int old_ratio = dirty_background_ratio;
> + unsigned long bytes;
> int ret;
>
> ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> - if (ret == 0 && write)
> - dirty_background_bytes = 0;
> +
> + if (ret == 0 && write) {
> + if (vm_dirty_ratio > 0) {
> + if (dirty_background_ratio >= vm_dirty_ratio)
> + ret = -EINVAL;
> + } else if (vm_dirty_bytes > 0) {
> + bytes = global_dirtyable_memory() * PAGE_SIZE *
> + dirty_background_ratio / 100;
> + if (bytes >= vm_dirty_bytes)
> + ret = -EINVAL;
> + }
> +
> + if (ret == 0)
> + dirty_background_bytes = 0;
> + else
> + dirty_background_ratio = old_ratio;
> + }
> +
How about implementing something like
bool vm_dirty_settings_valid(void)
helper which would validate whether current dirtiness settings are
consistent. That way we would not have to repeat very similar checks four
times. Also the arithmetics in:
global_dirtyable_memory() * PAGE_SIZE * dirty_background_ratio / 100
could overflow so I'd prefer to first divide by 100 and then multiply by
dirty_background_ratio...
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] 3+ messages in thread* Re: [PATCH] mm: introduce sanity check on dirty ratio sysctl value
2017-09-18 10:22 ` Jan Kara
@ 2017-09-18 11:36 ` Yafang Shao
0 siblings, 0 replies; 3+ messages in thread
From: Yafang Shao @ 2017-09-18 11:36 UTC (permalink / raw)
To: Jan Kara
Cc: akpm, Johannes Weiner, mhocko, vdavydov.dev, jlayton, nborisov,
Theodore Ts'o, mawilcox, linux-mm, linux-kernel
2017-09-18 18:22 GMT+08:00 Jan Kara <jack@suse.cz>:
> On Mon 18-09-17 01:39:28, 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 dirty_background_raio bigger than
>> dirty_ratio successfully. This behavior may mislead us.
>> So we should do this sanity check at the beginning.
>>
>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>
> ...
>
>> {
>> + int old_ratio = dirty_background_ratio;
>> + unsigned long bytes;
>> int ret;
>>
>> ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>> - if (ret == 0 && write)
>> - dirty_background_bytes = 0;
>> +
>> + if (ret == 0 && write) {
>> + if (vm_dirty_ratio > 0) {
>> + if (dirty_background_ratio >= vm_dirty_ratio)
>> + ret = -EINVAL;
>> + } else if (vm_dirty_bytes > 0) {
>> + bytes = global_dirtyable_memory() * PAGE_SIZE *
>> + dirty_background_ratio / 100;
>> + if (bytes >= vm_dirty_bytes)
>> + ret = -EINVAL;
>> + }
>> +
>> + if (ret == 0)
>> + dirty_background_bytes = 0;
>> + else
>> + dirty_background_ratio = old_ratio;
>> + }
>> +
>
> How about implementing something like
>
> bool vm_dirty_settings_valid(void)
>
> helper which would validate whether current dirtiness settings are
> consistent. That way we would not have to repeat very similar checks four
> times.
That seems a smarter way.
> Also the arithmetics in:
>
> global_dirtyable_memory() * PAGE_SIZE * dirty_background_ratio / 100
>
> could overflow so I'd prefer to first divide by 100 and then multiply by
> dirty_background_ratio...
>
Oh, yes. It could overflow.
> Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
I will reimplement it and submit a new patch.
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] 3+ messages in thread
end of thread, other threads:[~2017-09-18 11:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-17 17:39 [PATCH] mm: introduce sanity check on dirty ratio sysctl value Yafang Shao
2017-09-18 10:22 ` Jan Kara
2017-09-18 11:36 ` 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).