Re: [PATCH v2] mm: introduce validity check on vm dirtiness settings

[Yafang Shao]

2017-09-19 16:35 GMT+08:00 Jan Kara <jack@xxxxxxx>:
> On Tue 19-09-17 06:53:00, 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@xxxxxxxxx>
>
> The patch looks mostly good now. Just some small comments below.
>
>> diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
>> index 9baf66a..5de02f6 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
>> +available_memory / 100 * dirty_ratio.
>
> I would phrase this like:
>
> 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.
>

Thanks :)

>>  ==============================================================
>>
>> @@ -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.
>> +
>
> And similarly here:
>
> Note: dirty_ratio cannot be set lower than dirty_background_ratio or
> ratio corresponding to dirty_background_bytes.
>
Thanks :)

>
>> @@ -511,15 +511,68 @@ 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;
>> +                     goto out;
>> +             }
>> +     }
>> +
>> +     if (vm_dirty_bytes == 0 && vm_dirty_ratio == 0 &&
>> +             (dirty_background_bytes != 0 || dirty_background_ratio != 0))
>> +             ret = false;
>
> Hum, why not just:
>         if ((vm_dirty_bytes == 0 && vm_dirty_ratio) ||
>             (dirty_background_bytes == 0 && dirty_background_ratio == 0))
>                 ret = false;
>
> IMHO setting either tunable to 0 is just wrong and actively dangerous...
>

Because these four variables all could be set to 0 before, and I'm not
sure if this
is needed under some certain conditions, although I think this is
dangerous but I have
to keep it as before.

If you think that is wrong, then I will modified it as you suggested.

>> +out:
>> +     if (!ret)
>> +             pr_err("vm dirtiness can't less than vm background dirtiness\n");
>
> I would refrain from spamming logs with the error message. In my opinion it
> is not needed.
>
>>  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) {
>> +             if (dirty_background_ratio != old_ratio &&
>> +                     !vm_dirty_settings_valid()) {
>
> Why do you check whether new ratio is different here? If it is really
> needed, it would deserve a comment.
>

There're two reseaons,
1.  if you set a value same with the old value, it's needn't to do this check.
2. there's another behavior that I'm not sure whether it is reaonable.  i.e.
     if the old value is,
            vm.dirty_background_bytes = 0;
            vm.dirty_background_ratio=10;
      then I execute the bellow command,
            sysctl -w vm.dirty_background_bytes=0
     at the end these two values will be,
            vm.dirty_background_bytes = 0;
            vm.dirty_background_ratio=0;
I'm not sure if this is needed under some certain conditons, So I have
to keep it as before.




>> +                     dirty_background_ratio = old_ratio;
>> +                     ret = -EINVAL;
>> +             } else
>> +                     dirty_background_bytes = 0;
>> +     }
>> +
>>       return ret;
>>  }
>>
>> @@ -528,10 +581,17 @@ 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) {
>> +             if (dirty_background_bytes != old_bytes &&
>> +                     !vm_dirty_settings_valid()) {
>
> The same here...
>



>                                                                 Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR

Thanks
Yafang