Re: [PATCH V2] blk-cgroup: Flush stats before releasing blkcg_gq

[Waiman Long <longman@redhat.com>]

On 5/24/23 23:47, Ming Lei wrote:
> On Wed, May 24, 2023 at 10:50:29PM -0400, Waiman Long wrote:
>> On 5/24/23 22:04, Ming Lei wrote:
>>> On Wed, May 24, 2023 at 01:28:41PM -0400, Waiman Long wrote:
>>>> On 5/24/23 11:43, Waiman Long wrote:
>>>>
>>> llist_del_all() moves all 'bis' into one local list, and bis is one percpu
>>> variable of blkg, so in theory same bis won't be flushed at the same
>>> time. And one bis should be touched in just one of stat flush code path
>>> because of llist_del_all().
>>>
>>> So 'bis' still can be thought as being flushed in serialized way.
>>>
>>> However, blk_cgroup_bio_start() runs concurrently with blkcg_rstat_flush(),
>>> so once bis->lqueued is cleared in blkcg_rstat_flush(), this same bis
>>> could be added to the percpu llist and __blkcg_rstat_flush() from blkg_release()
>>> follows. This should be the only chance for concurrent stats update.
>> That is why I have in mind. A __blkcg_rstat_flush() can be from
>> blkg_release() and another one from the regular cgroup_rstat_flush*().
> But the two blkg can't be same because blkg_release() is run from rcu
> callback.

I am not talking about the parent blkg which can have a concurrent 
update. It is probably safe at the blkg level, but the lockless list may 
contain another bisc from a blkg that is not being destroyed.


>
>>
>>> But, blkg_release() is run in RCU callback, so the previous flush has
>>> been done, but new flush can come, and other blkg's stat could be added
>>> with same story above.
>>>
>>>> One way to
>>>> avoid that is to synchronize it by cgroup_rstat_cpu_lock. Another way is to
>>>> use the bisc->lqueued for synchronization.
>>> I'd avoid the external cgroup lock here.
>>>
>>>> In that case, you need to move
>>>> WRITE_ONCE(bisc->lqueued, false) in blkcg_rstat_flush() to the end after all
>>>> the  blkcg_iostat_update() call with smp_store_release() and replace the
>>>> READ_ONCE(bis->lqueued) check in blk_cgroup_bio_start() with
>>>> smp_load_acquire().
>>> This way looks doable, but I guess it still can't avoid concurrent update on parent
>>> stat, such as when  __blkcg_rstat_flush() from blkg_release() is
>>> in-progress, another sibling blkg's bis is added, meantime
>>> blkcg_rstat_flush() is called.
>> I realized that the use of cgroup_rstat_cpu_lock or the alternative was not
>> safe enough for preventing concurrent parent blkg rstat update.
> Indeed.
>
>>> Another way is to add blkcg->stat_lock for covering __blkcg_rstat_flush(), what
>>> do you think of this way?
>> I am thinking of adding a raw spinlock into blkg and take it when doing
>> blkcg_iostat_update(). This can guarantee no concurrent update to rstat
>> data. It has to be a raw spinlock as it will be under the
>> cgroup_rstat_cpu_lock raw spinlock.
> We still need to be careful since both child and parent are updated
> in the following blkcg_iostat_update().
>
>                  if (parent && parent->parent)
>                          blkcg_iostat_update(parent, &blkg->iostat.cur,
>                                              &blkg->iostat.last);
>
> But the concurrent update on parent blkg can be avoided by holding
> parent->stat_lock only.

That is true. Perhaps we can use Yosry's suggestion of adding a helper 
to acquire and release cgroup_rstat_lock before calling 
__blkcg_rstat_flush(). That will also guarantee no concurrent update.

Cheers,
Longman