Date: Wed, 3 Apr 2024 16:50:15 +0800
From: Kemeng Shi <shikemeng@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: Tejun Heo <tj@...nel.org>, akpm@...ux-foundation.org, linux-mm@...ck.org,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 willy@...radead.org, bfoster@...hat.com, dsterba@...e.com,
 mjguzik@...il.com, dhowells@...hat.com, peterz@...radead.org
Subject: Re: [PATCH 6/6] writeback: remove unneeded GDTC_INIT_NO_WB



on 4/2/2024 9:53 PM, Jan Kara wrote:
> On Thu 28-03-24 09:49:59, Kemeng Shi wrote:
>> 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@...weicloud.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.
> 
> Yeah, please keep the code as is for now. I agree this needs some cleanups
> but what you suggest is IMHO not an improvement.
Sure, will drop this in next version.

Thanks,
Kemeng
> 
> 								Honza
> 

Powered by blists - more mailing lists