Re: [PATCH] bfq: fix blkio cgroup leakage

[Paolo Valente <paolo.valente@linaro.org>]

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

Paolo

>> 
>> Paolo
>> 
>>> #IF CONFIG_BFQ_GROUP_IOSCHED is disabled:
>>> we have only one  bfqd->root_group object which allocated from bfq_create_group_hierarch()
>>> and bfqg_and_blkg_get() bfqg_and_blkg_put() are noop
>>> 
>>> Resume: in both cases extra reference is not required, so I continue to
>>> insist that we should revert  commit db37a34c563b because it tries to
>>> solve a non existing issue, but introduce the real one.
>>> 
>>> Please correct me if I'm wrong.
>>>> 
>>>> I'll check it ASAP, unless you do it before me.
>>>> 
>>>> Thanks,
>>>> Paolo
>>>> 
>>>> [1] https://lkml.org/lkml/2020/1/31/94
>>>> 
>>>>> Il giorno 2 lug 2020, alle ore 12:57, Dmitry Monakhov <dmonakhov@gmail.com> ha scritto:
>>>>> 
>>>>> 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.
>>>>> If fact bfq_group objects reference counting are quite different
>>>>> from bfq_queue. bfq_groups object are referenced by blkcg_gq via
>>>>> blkg_policy_data pointer, so  neither nor blkg_get() neither bfqg_get
>>>>> required here.
>>>>> 
>>>>> 
>>>>> This patch drop commit db37a34c563b ("block, bfq: get a ref to a group when adding it to a service tree")
>>>>> and add corresponding comment.
>>>>> 
>>>>> ##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 <dmonakhov@gmail.com>
>>>>> ---
>>>>> block/bfq-cgroup.c  |  2 +-
>>>>> block/bfq-iosched.h |  1 -
>>>>> block/bfq-wf2q.c    | 15 +++++----------
>>>>> 3 files changed, 6 insertions(+), 12 deletions(-)
>>>>> 
>>>>> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
>>>>> index 68882b9..b791e20 100644
>>>>> --- a/block/bfq-cgroup.c
>>>>> +++ b/block/bfq-cgroup.c
>>>>> @@ -332,7 +332,7 @@ static void bfqg_put(struct bfq_group *bfqg)
>>>>> 		kfree(bfqg);
>>>>> }
>>>>> 
>>>>> -void bfqg_and_blkg_get(struct bfq_group *bfqg)
>>>>> +static void bfqg_and_blkg_get(struct bfq_group *bfqg)
>>>>> {
>>>>> 	/* see comments in bfq_bic_update_cgroup for why refcounting bfqg */
>>>>> 	bfqg_get(bfqg);
>>>>> diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
>>>>> index cd224aa..7038952 100644
>>>>> --- a/block/bfq-iosched.h
>>>>> +++ b/block/bfq-iosched.h
>>>>> @@ -986,7 +986,6 @@ struct bfq_group *bfq_find_set_group(struct bfq_data *bfqd,
>>>>> struct blkcg_gq *bfqg_to_blkg(struct bfq_group *bfqg);
>>>>> struct bfq_group *bfqq_group(struct bfq_queue *bfqq);
>>>>> struct bfq_group *bfq_create_group_hierarchy(struct bfq_data *bfqd, int node);
>>>>> -void bfqg_and_blkg_get(struct bfq_group *bfqg);
>>>>> void bfqg_and_blkg_put(struct bfq_group *bfqg);
>>>>> 
>>>>> #ifdef CONFIG_BFQ_GROUP_IOSCHED
>>>>> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
>>>>> index 34ad095..6a363bb 100644
>>>>> --- a/block/bfq-wf2q.c
>>>>> +++ b/block/bfq-wf2q.c
>>>>> @@ -529,13 +529,14 @@ static void bfq_get_entity(struct bfq_entity *entity)
>>>>> {
>>>>> 	struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity);
>>>>> 
>>>>> +	/* Grab reference only for bfq_queue's objects, bfq_group ones
>>>>> +	 * are owned by blkcg_gq
>>>>> +	 */
>>>>> 	if (bfqq) {
>>>>> 		bfqq->ref++;
>>>>> 		bfq_log_bfqq(bfqq->bfqd, bfqq, "get_entity: %p %d",
>>>>> 			     bfqq, bfqq->ref);
>>>>> -	} else
>>>>> -		bfqg_and_blkg_get(container_of(entity, struct bfq_group,
>>>>> -					       entity));
>>>>> +	}
>>>>> }
>>>>> 
>>>>> /**
>>>>> @@ -649,14 +650,8 @@ static void bfq_forget_entity(struct bfq_service_tree *st,
>>>>> 
>>>>> 	entity->on_st_or_in_serv = false;
>>>>> 	st->wsum -= entity->weight;
>>>>> -	if (is_in_service)
>>>>> -		return;
>>>>> -
>>>>> -	if (bfqq)
>>>>> +	if (bfqq && !is_in_service)
>>>>> 		bfq_put_queue(bfqq);
>>>>> -	else
>>>>> -		bfqg_and_blkg_put(container_of(entity, struct bfq_group,
>>>>> -					       entity));
>>>>> }
>>>>> 
>>>>> /**
>>>>> -- 
>>>>> 2.7.4