Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1755626AbdIRLhA (ORCPT <rfc822;w@1wt.eu>);
        Mon, 18 Sep 2017 07:37:00 -0400
Received: from mail-it0-f68.google.com ([209.85.214.68]:37408 "EHLO
        mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1755471AbdIRLg5 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 18 Sep 2017 07:36:57 -0400
X-Google-Smtp-Source: AOwi7QDGKrKw4B5dFYGlUq+sqS9zSYhJ9RhEh0TGZ/oRm8OlMJ7V09pD6RBXnd5ZlZZEh9Lm4Cx0ykAO2pJBKN5SaWU=
MIME-Version: 1.0
In-Reply-To: <20170918102244.GJ32516@quack2.suse.cz>
References: <1505669968-12593-1-git-send-email-laoar.shao@gmail.com> <20170918102244.GJ32516@quack2.suse.cz>
From: Yafang Shao <laoar.shao@gmail.com>
Date: Mon, 18 Sep 2017 19:36:56 +0800
Message-ID: <CALOAHbDe+k9Wh401bDh_ZsvFDr22kWPNJfMrfg_Xcy_RBSYFxg@mail.gmail.com>
Subject: Re: [PATCH] mm: introduce sanity check on dirty ratio sysctl value
To: Jan Kara <jack@suse.cz>
Cc: akpm@linux-foundation.org, Johannes Weiner <hannes@cmpxchg.org>,
        mhocko@suse.com, vdavydov.dev@gmail.com, jlayton@redhat.com,
        nborisov@suse.com, "Theodore Ts'o" <tytso@mit.edu>,
        mawilcox@microsoft.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2212
Lines: 72

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