[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87tuy2ml9j.fsf@dmws.yandex.net>
Date: Mon, 20 Jul 2020 15:19:36 +0300
From: Dmitry Monakhov <dmonakhov@...il.com>
To: Paolo Valente <paolo.valente@...aro.org>
Cc: LKML <linux-kernel@...r.kernel.org>, linux-block@...r.kernel.org,
axboe@...nel.dk
Subject: Re: [PATCH] bfq: fix blkio cgroup leakage
Paolo Valente <paolo.valente@...aro.org> writes:
>> Il giorno 9 lug 2020, alle ore 10:19, Dmitry Monakhov <dmonakhov@...il.com> ha scritto:
>>
>> Paolo Valente <paolo.valente@...aro.org> writes:
>>
>>>> Il giorno 8 lug 2020, alle ore 19:48, Dmitry Monakhov <dmonakhov@...il.com> ha scritto:
>>>>
>>>> Paolo Valente <paolo.valente@...aro.org> writes:
>>>>
>>>>> Hi,
>>>>> sorry for the delay. The commit you propose to drop fix the issues
>>>>> reported in [1].
>>>>>
>>>>> Such a commit does introduce the leak that you report (thank you for
>>>>> spotting it). Yet, according to the threads mentioned in [1],
>>>>> dropping that commit would take us back to those issues.
>>>>>
>>>>> Maybe the solution is to fix the unbalance that you spotted?
>>>> I'm not quite shure that do I understand which bug was addressed for commit db37a34c563b.
>>>> AFAIU both bugs mentioned in original patchset was fixed by:
>>>> 478de3380 ("block, bfq: deschedule empty bfq_queues not referred by any proces")
>>>> f718b0932 ( block, bfq: do not plug I/O for bfq_queues with no proc refs)"
>>>>
>>>> So I review commit db37a34c563b as independent one.
>>>> It introduces extra reference for bfq_groups via bfqg_and_blkg_get(),
>>>> but do we actually need it here?
>>>>
>>>> #IF CONFIG_BFQ_GROUP_IOSCHED is enabled:
>>>> bfqd->root_group is holded by bfqd from bfq_init_queue()
>>>> other bfq_queue objects are owned by corresponding blkcg from bfq_pd_alloc()
>>>> So bfq_queue can not disappear under us.
>>>>
>>>
>>> You are right, but incomplete. No extra ref is needed for an entity
>>> that represents a bfq_queue. And this consideration mistook me before
>>> I realized that that commit was needed. The problem is that an entity
>>> may also represent a group of entities. In that case no reference is
>>> taken through any bfq_queue. The commit you want to remove takes this
>>> missing reference.
>> Sorry, It looks like I've mistyped sentance above, I ment to say bfq_group.
>> So here is my statement corrected:
>> #IF CONFIG_BFQ_GROUP_IOSCHED is enabled:
>> bfqd->root_group is holded by bfqd from bfq_init_queue()
>> other *bfq_group* objects are owned by corresponding blkcg, reference get from bfq_pd_alloc()
>> So *bfq_group* can not disappear under us.
>>
>> So no extra reference is required for entity represents bfq_group. Commit is not required.
>
> No, the entity may remain alive and on some tree after bfq_pd_offline has been invoked.
Ok you right, we should drop the group reference inside __bfq_bfqd_reset_in_service()
as we do for queue's entities. Please see updated patch version.
View attachment "0001-block-bfq-fix-blkio-cgroup-leakage-v2.patch" of type "text/x-diff" (4158 bytes)
Powered by blists - more mailing lists