Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751686AbdIUBmZ (ORCPT ); Wed, 20 Sep 2017 21:42:25 -0400 Received: from mail-it0-f45.google.com ([209.85.214.45]:55991 "EHLO mail-it0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751634AbdIUBmX (ORCPT ); Wed, 20 Sep 2017 21:42:23 -0400 X-Google-Smtp-Source: AOwi7QBObz0UPzfPAR20lsgyfOn8MGZEsbEvu4cv8P2vlo9ejQ7Am9yT2cawX9Pfe0xuYtuMen05bywoBZGf/pYovfU= MIME-Version: 1.0 In-Reply-To: <20170920153326.GH11106@quack2.suse.cz> References: <1505775180-12014-1-git-send-email-laoar.shao@gmail.com> <20170919083554.GC3216@quack2.suse.cz> <20170920153326.GH11106@quack2.suse.cz> From: Yafang Shao Date: Thu, 21 Sep 2017 09:42:21 +0800 Message-ID: Subject: Re: [PATCH v2] mm: introduce validity check on vm dirtiness settings To: Jan Kara Cc: akpm@linux-foundation.org, Johannes Weiner , mhocko@suse.com, vdavydov.dev@gmail.com, jlayton@redhat.com, nborisov@suse.com, "Theodore Ts'o" , 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: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2785 Lines: 70 2017-09-20 23:33 GMT+08:00 Jan Kara : > On Tue 19-09-17 19:48:00, Yafang Shao wrote: >> 2017-09-19 16:35 GMT+08:00 Jan Kara : >> > On Tue 19-09-17 06:53:00, Yafang Shao wrote: >> >> + 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 == 0) || >> > (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. > > OK, I see but see below. > >> >> 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. > > OK, this is somewhat the problem of the switching logic between _bytes and > _ratio bytes and also the fact that '0' has a special meaning in these > files. I think the cleanest would be to just refuse writing of '0' into any > of these files which would deal with the problem as well. Got it. I will submit a new patch then. > > Honza > -- > Jan Kara > SUSE Labs, CR