Paolo Valente [off-list ref] writes:

quoted

Il giorno 9 lug 2020, alle ore 10:19, Dmitry Monakhov [off-list ref] ha scritto:

Paolo Valente [off-list ref] writes:

quoted

quoted

Il giorno 8 lug 2020, alle ore 19:48, Dmitry Monakhov [off-list ref] ha scritto:

Paolo Valente [off-list ref] writes:

quoted

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.