Before commit 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()"),
blkg's stats is only flushed if they are online. 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. For safety,
revert back to the old behavior by ignoring offline blkg's.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 block/blk-cgroup.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 21cc88349f21..c466aef0d467 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -885,6 +885,12 @@ static void blkcg_rstat_flush(struct cgroup_subsys_state *css, int cpu)
 
 		WRITE_ONCE(bisc->lqueued, false);
 
+		/* Don't flush its stats if blkg is offline */
+		if (unlikely(!blkg->online)) {
+			percpu_ref_put(&blkg->refcnt);
+			continue;
+		}
+
 		/* fetch the current per-cpu values */
 		do {
 			seq = u64_stats_fetch_begin(&bisc->sync);
-- 
2.31.1

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].)

Regards,
Michal

[1] https://lore.kernel.org/r/YqEfNgUc8jxlAq8D@blackbook/

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.

Thanks.

-- 
tejun

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

On 12/12/22 07:59, 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].)
The original cgroup_rstat_flush() iterates the list of blkg's in the 
blkg_list. A blkg will be removed from the list when it is offlined. 
This patch just reverts its behavior to that previous behavior.
>
>> 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.
I guess it is likely to be safe to flush an offline blkg. I am just 
being conservative in case there is any corner case where it may be a 
problem which I haven't foreseen.
>
>> 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].)

It is possible that a blkg may be updated before it becomes offline, but 
the flush isn't done in time before that happens. The next patch will 
catch some of that.

Cheers,
Longman