[PATCH] blk: start bypass mode in blk_unregister_queue

[PATCH] blk: start bypass mode in blk_unregister_queue

[Aaron Lu]

In blk_register_queue, we will end bypass mode for the queue; but in
blk_unregister_queue, we didn't start bypass mode for it. This would
cause the WARN_ON_ONCE(q->bypass_depth < 0) to trigger if the queue gets
registered, unregistered and then again registered, e.g. unload scsi
cdrom module driver sr_mod and then reload it will trigger such a
warning.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 block/blk-sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 6206a93..bd322a1 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -615,4 +615,6 @@ void blk_unregister_queue(struct gendisk *disk)
 	kobject_del(&q->kobj);
 	blk_trace_remove_sysfs(disk_to_dev(disk));
 	kobject_put(&disk_to_dev(disk)->kobj);
+
+	blk_queue_bypass_start(q);
 }
-- 
1.8.1.5

Re: [PATCH] blk: start bypass mode in blk_unregister_queue

[Tejun Heo]

On Thu, Apr 04, 2013 at 10:01:14PM +0800, Aaron Lu wrote:
> In blk_register_queue, we will end bypass mode for the queue; but in
> blk_unregister_queue, we didn't start bypass mode for it. This would
> cause the WARN_ON_ONCE(q->bypass_depth < 0) to trigger if the queue gets
> registered, unregistered and then again registered, e.g. unload scsi
> cdrom module driver sr_mod and then reload it will trigger such a
> warning.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

Is this something which actually happens?  Why would an unregistered
queue registered again?  Do we even support that?

Starting a bypass mode can be very expensive and some drivers create
and destroy a lot of queues during probing.  We don't want a call to
blk_queue_bypass_start() on every queue creation / destruction cycle.

Thanks.

-- 
tejun

Re: [PATCH] blk: start bypass mode in blk_unregister_queue

[Aaron Lu]

On 04/04/2013 10:03 PM, Tejun Heo wrote:
> On Thu, Apr 04, 2013 at 10:01:14PM +0800, Aaron Lu wrote:
>> In blk_register_queue, we will end bypass mode for the queue; but in
>> blk_unregister_queue, we didn't start bypass mode for it. This would
>> cause the WARN_ON_ONCE(q->bypass_depth < 0) to trigger if the queue gets
>> registered, unregistered and then again registered, e.g. unload scsi
>> cdrom module driver sr_mod and then reload it will trigger such a
>> warning.
>>
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> 
> Is this something which actually happens?  Why would an unregistered
> queue registered again?  Do we even support that?

It probably wouldn't happen for normal user, only for some SCSI driver
developers like me, e.g. the sr_mod will normally load during boot, and
when I made some changes to the code, I'll unload the driver and reload
it. This is found when I was developing ZPODD code and I just found some
time to see what happened.

The queue for the scsi device will always be there, the queue(and the
device) will not go away on driver unregistration. So it will be left in
normal mode, and on next blk_register_queue call, the warning will show
up.

> 
> Starting a bypass mode can be very expensive and some drivers create
> and destroy a lot of queues during probing.  We don't want a call to
> blk_queue_bypass_start() on every queue creation / destruction cycle.

By expensive, do you mean the drain of the queue? Since the queue is to
be unregistered, I suppose the queue has to be drained somewhere?

Thanks,
Aaron

Re: [PATCH] blk: start bypass mode in blk_unregister_queue

[Tejun Heo]

Hello,

On Thu, Apr 04, 2013 at 10:16:39PM +0800, Aaron Lu wrote:
> It probably wouldn't happen for normal user, only for some SCSI driver
> developers like me, e.g. the sr_mod will normally load during boot, and
> when I made some changes to the code, I'll unload the driver and reload
> it. This is found when I was developing ZPODD code and I just found some
> time to see what happened.

Does that unregister and register the same queue?  Ah, crap, it does.

> The queue for the scsi device will always be there, the queue(and the
> device) will not go away on driver unregistration. So it will be left in
> normal mode, and on next blk_register_queue call, the warning will show
> up.
> 
> > Starting a bypass mode can be very expensive and some drivers create
> > and destroy a lot of queues during probing.  We don't want a call to
> > blk_queue_bypass_start() on every queue creation / destruction cycle.
> 
> By expensive, do you mean the drain of the queue? Since the queue is to
> be unregistered, I suppose the queue has to be drained somewhere?

The problematic one is synchronize_rcu().  It adds up pretty quickly.
I'd suggest just skipping blk_queue_bypass_end() during registration
if the queue is not bypassing.

Thanks.

-- 
tejun

Re: [PATCH] blk: start bypass mode in blk_unregister_queue

[Aaron Lu]

On 04/04/2013 10:20 PM, Tejun Heo wrote:
> Hello,
> 
> On Thu, Apr 04, 2013 at 10:16:39PM +0800, Aaron Lu wrote:
>> It probably wouldn't happen for normal user, only for some SCSI driver
>> developers like me, e.g. the sr_mod will normally load during boot, and
>> when I made some changes to the code, I'll unload the driver and reload
>> it. This is found when I was developing ZPODD code and I just found some
>> time to see what happened.
> 
> Does that unregister and register the same queue?  Ah, crap, it does.
> 
>> The queue for the scsi device will always be there, the queue(and the
>> device) will not go away on driver unregistration. So it will be left in
>> normal mode, and on next blk_register_queue call, the warning will show
>> up.
>>
>>> Starting a bypass mode can be very expensive and some drivers create
>>> and destroy a lot of queues during probing.  We don't want a call to
>>> blk_queue_bypass_start() on every queue creation / destruction cycle.
>>
>> By expensive, do you mean the drain of the queue? Since the queue is to
>> be unregistered, I suppose the queue has to be drained somewhere?
> 
> The problematic one is synchronize_rcu().  It adds up pretty quickly.
> I'd suggest just skipping blk_queue_bypass_end() during registration
> if the queue is not bypassing.

OK, thanks for the suggestion!

-Aaron