We got a reference of the io scheduler module in
blk_mq_elv_switch_none to prevent the module from
being removed. We need to put that reference back
once we are done.

Signed-off-by: Jinlong Chen <chenjinlong2016@outlook.com>
---
---
Changes in v2:
 - reword the commit message because the patch is for blk-mq precisely 

 block/blk-mq.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8070b6c10e8d..8dfe3bf3e599 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4595,6 +4595,13 @@ static void blk_mq_elv_switch_back(struct list_head *head,
 
 	mutex_lock(&q->sysfs_lock);
 	elevator_switch(q, t);
+	/**
+	 * We got a reference of the io scheduler module in
+	 * blk_mq_elv_switch_none to prevent the module from
+	 * being removed. We need to put that reference back
+	 * once we are done.
+	 */
+	module_put(t->elevator_owner);
 	mutex_unlock(&q->sysfs_lock);
 }
 
-- 
2.34.1

Sorry for the disturbance.

This patch is just wrong. elevator_switch_mq does not increase the reference
 count of the io scheduler module to which we are switching. Hence, we do not
 need to put the reference back manually.

Sincerely
Jinlong Chen

Hi,

在 2022/10/13 21:03, Jinlong Chen 写道:
> Sorry for the disturbance.
> 
> This patch is just wrong. elevator_switch_mq does not increase the reference
>   count of the io scheduler module to which we are switching. Hence, we do not
>   need to put the reference back manually.

I'm confused here, cause I do think this patch make sense.

blk_mq_update_nr_hw_queues
  // for each queue using the tagset
  blk_mq_freeze_queue
  // if current elevator is not none, swith to none
  blk_mq_elv_switch_none
   // elevator need to be switched back, got a reference to
   // prevent module to be removed.
   __module_get
   elevator_switch(q, NULL);

  // switch back from none elevator
  blk_mq_elv_switch_back
   -> should release the module reference here
  blk_mq_unfreeze_queue

By the way, I do not test yet, but I think problem can be reporduced:

1. modprobe bfq
2. switch elevator to bfq
3. trigger blk_mq_update_nr_hw_queues
4. switch elevator to none
5. rmmod bfq will fail

Thanks,
Kuai
> 
> Sincerely
> Jinlong Chen
> .
> 

Hi, Yu Kuai!

> 
> I'm confused here, cause I do think this patch make sense.
> 
> blk_mq_update_nr_hw_queues
>   // for each queue using the tagset
>   blk_mq_freeze_queue
>   // if current elevator is not none, swith to none
>   blk_mq_elv_switch_none
>    // elevator need to be switched back, got a reference to
>    // prevent module to be removed.
>    __module_get
>    elevator_switch(q, NULL);
> 
>   // switch back from none elevator
>   blk_mq_elv_switch_back
>    -> should release the module reference here
>   blk_mq_unfreeze_queue
> 

We need to release the reference only if blk_mq_elv_switch_back got its own
 reference. However, blk_mq_elv_switch_back (precisely elevator_switch_mq)
 does not increase the reference of the module it is switching to. 
 Hence, the reference got in blk_mq_elv_switch_none does not need to be released,
 or we'll have to re-increase the reference count manually.

> 
> By the way, I do not test yet, but I think problem can be reporduced:
> 
> 
> 1. modprobe bfq
> 2. switch elevator to bfq
> 3. trigger blk_mq_update_nr_hw_queues
> 4. switch elevator to none
> 5. rmmod bfq will fail
> 

I tried to reproduce the problem but failed, so I found my mistake.

Sincerely,
Jinlong Chen

在 2022/10/13 21:47, Jinlong Chen 写道:
> Hi, Yu Kuai!
> 
>>
>> I'm confused here, cause I do think this patch make sense.
>>
>> blk_mq_update_nr_hw_queues
>>    // for each queue using the tagset
>>    blk_mq_freeze_queue
>>    // if current elevator is not none, swith to none
>>    blk_mq_elv_switch_none
>>     // elevator need to be switched back, got a reference to
>>     // prevent module to be removed.
>>     __module_get
>>     elevator_switch(q, NULL);
>>
>>    // switch back from none elevator
>>    blk_mq_elv_switch_back
>>     -> should release the module reference here
>>    blk_mq_unfreeze_queue
>>
> 
> We need to release the reference only if blk_mq_elv_switch_back got its own
>   reference. However, blk_mq_elv_switch_back (precisely elevator_switch_mq)
>   does not increase the reference of the module it is switching to.
>   Hence, the reference got in blk_mq_elv_switch_none does not need to be released,
>   or we'll have to re-increase the reference count manually.'

But I don't see elevator_switch() release the referenct of the module
it is switching from. It's still not balance to me.

Thanks,
Kuai
> 
>>
>> By the way, I do not test yet, but I think problem can be reporduced:
>>
>>
>> 1. modprobe bfq
>> 2. switch elevator to bfq
>> 3. trigger blk_mq_update_nr_hw_queues
>> 4. switch elevator to none
>> 5. rmmod bfq will fail
>>
> 
> I tried to reproduce the problem but failed, so I found my mistake.
> 
> Sincerely,
> Jinlong Chen
> .
> 

> 
> But I don't see elevator_switch() release the referenct of the module
> it is switching from. It's still not balance to me.
> 

The reference count is released here:

elevator_switch_mq()
  --> elevator_exit()
    --> __elevator_exit()
      --> kobject_put()
        --> kobject_release()
          --> elevator_release()
            --> elevator_put()

What a deep call stack. :)

Sincerely,
Jinlong Chen

Hi!

在 2022/10/13 22:18, Jinlong Chen 写道:
>>
>> But I don't see elevator_switch() release the referenct of the module
>> it is switching from. It's still not balance to me.
>>
> 
> The reference count is released here:
> 
> elevator_switch_mq()
>    --> elevator_exit()
>      --> __elevator_exit()
>        --> kobject_put()
>          --> kobject_release()
>            --> elevator_release()
>              --> elevator_put()
> 
> What a deep call stack. :)

Yes, you're right.

Thanks,
Kuai
> 
> Sincerely,
> Jinlong Chen
> .
>