Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751902AbdITPdb (ORCPT ); Wed, 20 Sep 2017 11:33:31 -0400 Received: from mx2.suse.de ([195.135.220.15]:32871 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751872AbdITPd3 (ORCPT ); Wed, 20 Sep 2017 11:33:29 -0400 Date: Wed, 20 Sep 2017 17:33:26 +0200 From: Jan Kara To: Yafang Shao Cc: Jan Kara , 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 Subject: Re: [PATCH v2] mm: introduce validity check on vm dirtiness settings Message-ID: <20170920153326.GH11106@quack2.suse.cz> References: <1505775180-12014-1-git-send-email-laoar.shao@gmail.com> <20170919083554.GC3216@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2564 Lines: 66 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. Honza -- Jan Kara SUSE Labs, CR