Paolo Valente writes: >> Il giorno 9 lug 2020, alle ore 10:19, Dmitry Monakhov ha scritto: >> >> Paolo Valente writes: >> >>>> Il giorno 8 lug 2020, alle ore 19:48, Dmitry Monakhov ha scritto: >>>> >>>> Paolo Valente 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.