| Message ID | 20200702105751.20482-1-dmonakhov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bfq: fix blkio cgroup leakage | expand |
Dmitry Monakhov <dmonakhov@gmail.com> writes: Ping. Do you have any objections against this patch? > 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
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'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 >
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. #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 >>
> 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. 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 >>>
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. > > 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 >>>>
> 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
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.
On 7/20/20 6:19 AM, Dmitry Monakhov wrote: > 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. Can you please send that out as a proper v2?
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. But bfq_group's entity stil holded by child's entity from here, -> bfq_init_entity() ->bfqg_and_blkg_get(bfqg); ->entity->parent = bfqg->my_entity -> bfq_put_queue(bfqq) FINAL_PUT ->bfqg_and_blkg_put(bfqq_group(bfqq)) ->kmem_cache_free(bfq_pool, bfqq); so group can not just disappear us while tree is in service. Please corect me if I'm wrong. BTW, I've send new version with updated description here [1] Footnotes: [1] https://lore.kernel.org/linux-block/20200811064340.31284-1-dmtrmonakhov@yandex-team.ru > > 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
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)); } /**
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(-)