Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1fe8b5b3-c066-649b-fa4a-07d67ed7eb17@redhat.com>
Date:   Mon, 12 Dec 2022 19:21:40 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [PATCH-block v2 2/3] blk-cgroup: Don't flush a blkg if destroyed
Content-Language: en-US
To:     Tejun Heo <tj@kernel.org>,
        =?UTF-8?Q?Michal_Koutn=c3=bd?= <mkoutny@suse.com>
Cc:     Jens Axboe <axboe@kernel.dk>, Josef Bacik <josef@toxicpanda.com>,
        Zefan Li <lizefan.x@bytedance.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        cgroups@vger.kernel.org, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        "Dennis Zhou (Facebook)" <dennisszhou@gmail.com>
References: <20221211222058.2946830-1-longman@redhat.com>
 <20221211222058.2946830-3-longman@redhat.com>
 <20221212125953.GE16456@blackbody.suse.cz> <Y5eoQ8UBN8Xpc+Wp@slm.duckdns.org>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <Y5eoQ8UBN8Xpc+Wp@slm.duckdns.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 12/12/22 17:16, Tejun Heo wrote:
> On Mon, Dec 12, 2022 at 01:59:53PM +0100, Michal Koutný wrote:
>> Hello.
>>
>> On Sun, Dec 11, 2022 at 05:20:57PM -0500, Waiman Long <longman@redhat.com> wrote:
>>> Before commit 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()"),
>>> blkg's stats is only flushed if they are online.
>> I'm not sure I follow -- css_release_work_fn/cgroup_rstat_flush may be
>> called on an offlined blkcg (offlined!=released). There's no invariant
>> ensuring offlined blkcg won't be flushed. (There is only current
>> situation when there is no reader of io data that'd need them flushed
>> [1].)
>>
>>> In addition, the stat flushing of blkgs in blkcg_rstat_flush()
>>> includes propagating the rstat data to its parent. However, if a blkg
>>> has been destroyed (offline), the validity of its parent may be
>>> questionable.
>> Parents won't be freed (neither offlined) before children (see
>> css_killed_work_fn). It should be regularly OK to pass data into a
>> parent of an offlined blkcg.
>>
>>> For safety, revert back to the old behavior by ignoring offline
>>> blkg's.
>> I don't know if this is a good reasoning. If you argue that offlined
>> children needn't be taken into parent's account, then I think it's more
>> efficient to exclude the offlined blkcgs from update. (With the caveat I
>> have in [1].)
> Yeah, I'm not sure about this patch either. Doesn't seem necessary.

I wrote this patch because I am not totally sure it is safe to flush 
offline blkgs. Since both you and Michal don't see any problem with it. 
I am fine taking it out.

Cheers,
Longman