Received-SPF: pass (google.com: domain of linux-kernel+bounces-122305-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Subject: Re: [PATCH 6/6] writeback: remove unneeded GDTC_INIT_NO_WB
To: Jan Kara <jack@suse.cz>
Cc: Tejun Heo <tj@kernel.org>, akpm@linux-foundation.org, linux-mm@kvack.org,
 linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
 willy@infradead.org, bfoster@redhat.com, dsterba@suse.com,
 mjguzik@gmail.com, dhowells@redhat.com, peterz@infradead.org
References: <20240320110222.6564-1-shikemeng@huaweicloud.com>
 <20240320110222.6564-7-shikemeng@huaweicloud.com>
 <Zfr9my_tfxO-N6HS@mtj.duckdns.org>
 <becdb16b-a318-ec05-61d2-d190541ae997@huaweicloud.com>
 <20240327093309.ejuzjus2zcixb4qt@quack3>
From: Kemeng Shi <shikemeng@huaweicloud.com>
Message-ID: <c2fc01e2-f15a-d331-6c4f-64319f3adc8a@huaweicloud.com>
Date: Thu, 28 Mar 2024 09:49:59 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240327093309.ejuzjus2zcixb4qt@quack3>
Content-Type: text/plain; charset=gbk
Content-Transfer-Encoding: 7bit


on 3/27/2024 5:33 PM, Jan Kara wrote:
> On Thu 21-03-24 15:12:21, Kemeng Shi wrote:
>>
>>
>> on 3/20/2024 11:15 PM, Tejun Heo wrote:
>>> Hello,
>>>
>>> On Wed, Mar 20, 2024 at 07:02:22PM +0800, Kemeng Shi wrote:
>>>> We never use gdtc->dom set with GDTC_INIT_NO_WB, just remove unneeded
>>>> GDTC_INIT_NO_WB
>>>>
>>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>>> ...
>>>>  void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)
>>>>  {
>>>> -	struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
>>>> +	struct dirty_throttle_control gdtc = { };
>>>
>>> Even if it's currently not referenced, wouldn't it still be better to always
>>> guarantee that a dtc's dom is always initialized? I'm not sure what we get
>>> by removing this.
>> As we explicitly use GDTC_INIT_NO_WB to set global_wb_domain before
>> calculating dirty limit with domain_dirty_limits, I intuitively think the
>> dirty limit calculation in domain_dirty_limits is related to
>> global_wb_domain when CONFIG_WRITEBACK_CGROUP is enabled while the truth
>> is not. So this is a little confusing to me.
> 
Hi Jan,
> I'm not sure I understand your confusion. domain_dirty_limits() calculates
> the dirty limit (and background dirty limit) for the dirty_throttle_control
> passed in. If you pass dtc initialized with GDTC_INIT[_NO_WB], it will
> compute global dirty limits. If the dtc passed in is initialized with
> MDTC_INIT() it will compute cgroup specific dirty limits.
No doubt about this.
> 
> Now because domain_dirty_limits() does not scale the limits based on each
> device throughput - that is done only later in __wb_calc_thresh() to avoid> relatively expensive computations when we don't need them - and also
> because the effective dirty limit (dtc->dom->dirty_limit) is not updated by
> domain_dirty_limits(), domain_dirty_limits() does not need dtc->dom at all.
Acutally, here is the thing confusing me. For wb_calc_thresh, we always pass
dtc initialized with a wb (GDTC_INIT(wb) or MDTC_INIT(wb,..). The dtc
initialized with _NO_WB is only passed to domain_dirty_limits. However, The
dom initialized by _NO_WB for domain_dirty_limits is not needed at all.
> But that is a technical detail of implementation and I don't want this
> technical detail to be relied on by even more code.
Yes, I agree with this. So I wonder if it's acceptable to simply define
GDTC_INIT_NO_WB to empty for now instead of remove defination of
GDTC_INIT_NO_WB. When implementation of domain_dirty_limits() or any
other low level function in future using GDTC_INIT(_NO_WB) changes to
need dtc->domain, we re-define GDTC_INIT_NO_WB to proper value.
As this only looks confusing to me. I will drop this one in next version
if you still prefer to keep definatino of GDTC_INIT_NO_WB in the old way.

Thanks,
Kemeng
> 
> What might have confused you is that GDTC_INIT_NO_WB is defined to be empty
> when CONFIG_CGROUP_WRITEBACK is disabled. But this is only because in that
> case dtc_dom() function unconditionally returns global_wb_domain so we
> don't bother with initializing (or even having) the 'dom' field anywhere.
> 
> Now I agree this whole code is substantially confusing and complex and it
> would all deserve some serious thought how to make it more readable. But
> even after thinking about it again I don't think removing GDTC_INIT_NO_WB is
> the right way to go.
> 
> 								Honza
>