From: Dmitry Monakhov <dmonakhov@gmail.com>
To: Paolo Valente <paolo.valente@linaro.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
linux-block@vger.kernel.org, axboe@kernel.dk
Subject: Re: [PATCH] bfq: fix blkio cgroup leakage
Date: Mon, 20 Jul 2020 15:19:36 +0300 [thread overview]
Message-ID: <87tuy2ml9j.fsf@dmws.yandex.net> (raw)
In-Reply-To: <22087F19-BC93-447E-848A-109392E0622D@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]
Paolo Valente <paolo.valente@linaro.org> writes:
>> Il giorno 9 lug 2020, alle ore 10:19, Dmitry Monakhov <dmonakhov@gmail.com> ha scritto:
>>
>> Paolo Valente <paolo.valente@linaro.org> writes:
>>
>>>> Il giorno 8 lug 2020, alle ore 19:48, Dmitry Monakhov <dmonakhov@gmail.com> ha scritto:
>>>>
>>>> Paolo Valente <paolo.valente@linaro.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.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-block-bfq-fix-blkio-cgroup-leakage-v2.patch --]
[-- Type: text/x-diff, Size: 4156 bytes --]
From d708067cfa570f80b43c5716adf81d2a29b3d523 Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
Date: Mon, 20 Jul 2020 15:10:34 +0300
Subject: [PATCH] block: bfq fix blkio cgroup leakage v2
commit db37a34c563b ("block, bfq: get a ref to a group when adding it to a service tree")
introduce leak forbfq_group and blkcg_gq objects because of get/put
imbalance. See trace balow:
-> blkg_alloc
-> bfq_pq_alloc
-> bfqg_get (+1)
->bfq_activate_bfqq
->bfq_activate_requeue_entity
-> __bfq_activate_entity
->bfq_get_entity
->bfqg_and_blkg_get (+1) <==== : Note1
->bfq_del_bfqq_busy
->bfq_deactivate_entity+0x53/0xc0 [bfq]
->__bfq_deactivate_entity+0x1b8/0x210 [bfq]
-> bfq_forget_entity(is_in_service = true)
entity->on_st_or_in_serv = false <=== :Note2
if (is_in_service)
return; ==> do not touch reference
-> blkcg_css_offline
-> blkcg_destroy_blkgs
-> blkg_destroy
-> bfq_pd_offline
-> __bfq_deactivate_entity
if (!entity->on_st_or_in_serv) /* true, because (Note2)
return false;
-> bfq_pd_free
-> bfqg_put() (-1, byt bfqg->ref == 2) because of (Note2)
So bfq_group and blkcg_gq will leak forever, see test-case below.
We should drop group reference once it finaly removed from service
inside __bfq_bfqd_reset_in_service, as we do with queue entities.
##TESTCASE_BEGIN:
#!/bin/bash
max_iters=${1:-100}
#prep cgroup mounts
mount -t tmpfs cgroup_root /sys/fs/cgroup
mkdir /sys/fs/cgroup/blkio
mount -t cgroup -o blkio none /sys/fs/cgroup/blkio
# Prepare blkdev
grep blkio /proc/cgroups
truncate -s 1M img
losetup /dev/loop0 img
echo bfq > /sys/block/loop0/queue/scheduler
grep blkio /proc/cgroups
for ((i=0;i<max_iters;i++))
do
mkdir -p /sys/fs/cgroup/blkio/a
echo 0 > /sys/fs/cgroup/blkio/a/cgroup.procs
dd if=/dev/loop0 bs=4k count=1 of=/dev/null iflag=direct 2> /dev/null
echo 0 > /sys/fs/cgroup/blkio/cgroup.procs
rmdir /sys/fs/cgroup/blkio/a
grep blkio /proc/cgroups
done
##TESTCASE_END:
Signed-off-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
---
block/bfq-wf2q.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
index 8113138..93b236c 100644
--- a/block/bfq-wf2q.c
+++ b/block/bfq-wf2q.c
@@ -635,14 +635,10 @@ static void bfq_idle_insert(struct bfq_service_tree *st,
* @entity: the entity being removed.
* @is_in_service: true if entity is currently the in-service entity.
*
- * Forget everything about @entity. In addition, if entity represents
- * a queue, and the latter is not in service, then release the service
- * reference to the queue (the one taken through bfq_get_entity). In
- * fact, in this case, there is really no more service reference to
- * the queue, as the latter is also outside any service tree. If,
- * instead, the queue is in service, then __bfq_bfqd_reset_in_service
- * will take care of putting the reference when the queue finally
- * stops being served.
+ * Forget everything about @entity. If entity is not in service, then release
+ * the service reference to the entity (the one taken through bfq_get_entity).
+ * If the entity is in service, then __bfq_bfqd_reset_in_service will take care
+ * of putting the reference when the entity finally stops being served.
*/
static void bfq_forget_entity(struct bfq_service_tree *st,
struct bfq_entity *entity,
@@ -1626,9 +1622,16 @@ bool __bfq_bfqd_reset_in_service(struct bfq_data *bfqd)
* execute the final step: reset in_service_entity along the
* path from entity to the root.
*/
- for_each_entity(entity)
+ for_each_entity(entity) {
entity->sched_data->in_service_entity = NULL;
-
+ /*
+ * Release bfq_groups reference if it was not released in
+ * bfq_forget_entity, which was taken in bfq_get_entity.
+ */
+ if (!bfq_entity_to_bfqq(entity) && !entity->on_st_or_in_serv)
+ bfqg_and_blkg_put(container_of(entity, struct bfq_group,
+ entity));
+ }
/*
* in_serv_entity is no longer in service, so, if it is in no
* service tree either, then release the service reference to
--
2.7.4
next prev parent reply other threads:[~2020-07-20 12:19 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-02 10:57 [PATCH] bfq: fix blkio cgroup leakage Dmitry Monakhov
2020-07-08 9:03 ` Dmitry Monakhov
2020-07-08 16:35 ` Paolo Valente
2020-07-08 17:48 ` Dmitry Monakhov
2020-07-09 7:19 ` Paolo Valente
2020-07-09 8:19 ` Dmitry Monakhov
2020-07-09 8:35 ` Paolo Valente
2020-07-20 12:19 ` Dmitry Monakhov [this message]
2020-07-20 16:34 ` Jens Axboe
2020-07-20 17:04 ` [PATCH] block: bfq fix blkio cgroup leakage v2 Dmitry Monakhov
2020-07-26 11:31 ` Oleksandr Natalenko
2020-07-27 8:01 ` [PATCH] block: bfq fix blkio cgroup leakage v3 Dmitry Monakhov
2020-07-27 9:17 ` Oleksandr Natalenko
2020-08-11 8:11 ` [PATCH] bfq: fix blkio cgroup leakage Dmitry Monakhov
-- strict thread matches above, loose matches on Subject: below --
2020-06-28 17:20 Dmitry Monakhov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87tuy2ml9j.fsf@dmws.yandex.net \
--to=dmonakhov@gmail.com \
--cc=axboe@kernel.dk \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=paolo.valente@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).