Re: [PATCH] mm: introduce sanity check on dirty ratio sysctl value

[Yafang Shao]

2017-09-18 18:22 GMT+08:00 Jan Kara <jack@xxxxxxx>:
> 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@xxxxxxxxx>
>
> ...
>
>>  {
>> +     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@xxxxxxxx>
> SUSE Labs, CR


I will reimplement it and submit a new patch.

Thanks
Yafang