Oops when SCSI device under multipath is removed

Hi James,

With the attached shell script, blk_insert_cloned_request will cause oops with 3.1-rc1. (This is a regression introduced in 2.6.39)

The regression was introduced by 86cbfb5607d4b81b1a993ff689bbd2addd5d3a9b, "[SCSI] put stricter guards on queue dead checks".
Part of the commit has moved scsi_free_queue(), which calls blk_cleanup_queue(), to scsi_remove_device(), which is called while the device is still open.
The oops occurs because blk_insert_cloned_request() is called after blk_cleanup_queue() is called (which frees elevator
and turns on QUEUE_FLAG_DEAD).

2 patches have been proposed but neither of them included:
1) Add QUEUE_FLAG_DEAD check in blk_insert_cloned_request()

     https://lkml.org/lkml/2011/7/8/457

2) SCSI to call blk_cleanup_queue() from device's ->release() callback (before 2.6.39, it used to work like this)
https://lkml.org/lkml/2011/7/2/106

Quoted 6 lines · Jun'ichi Nomura

2 patches have been proposed but neither of them included:
1) Add QUEUE_FLAG_DEAD check in blk_insert_cloned_request()

     https://lkml.org/lkml/2011/7/8/457

2) SCSI to call blk_cleanup_queue() from device's ->release() callback (before 2.6.39, it used to work like this)
https://lkml.org/lkml/2011/7/2/106

Well, they both have documented objections. I asked why we destroy the elevator in the del case and didn't get any traction, so let me show the actual patch which should fix all of these issues.

Is there a good reason for not doing this as a bug fix now?

James

---

block/blk-core.c +0 -5

index b627558..cc72b7d 100644
@@ -367,11 +367,6 @@ void blk_cleanup_queue(struct request_queue *q)
 	queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q);
 	mutex_unlock(&q->sysfs_lock);
 
-	if (q->elevator)
-		elevator_exit(q->elevator);
-
-	blk_throtl_exit(q);
-
 	blk_put_queue(q);
 }
 EXPORT_SYMBOL(blk_cleanup_queue);

block/blk-sysfs.c +0 -0

index 0ee17b5..a5a756b 100644

I thought a driver could either rely on spin lock provided by request queue or override that by providing its own spinlock.

blk_init_allocated_queue_node()

        /* Override internal queue lock with supplied lock pointer */
        if (lock)
                q->queue_lock           = lock;

So if driver calls blk_cleanup_queue() and drops its reference on queue, then it should be free to release any memory it has allocated for spinlock. So though queue is around there are no gurantees that q->queue_lock is still around. That memory might have been freed by driver and reused.

I see many drivers are providing their own locks. Some samples from drivers/block.

/virtio_blk.c: q = vblk->disk->queue = blk_init_queue(do_virtblk_request,
&vblk->lock);
./xd.c: xd_queue = blk_init_queue(do_xd_request, &xd_lock);
./cpqarray.c: q = blk_init_queue(do_ida_request, &hba[i]->lock);
./sx8.c: q = blk_init_queue(carm_rq_fn, &host->lock);
./sx8.c: q = blk_init_queue(carm_oob_rq_fn, &host->lock);
./floppy.c: disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);
./viodasd.c: q = blk_init_queue(do_viodasd_request, &d->q_lock);
./cciss.c: disk->queue = blk_init_queue(do_cciss_request, &h->lock);
./hd.c: hd_queue = blk_init_queue(do_hd_request, &hd_lock);
./DAC960.c: RequestQueue = blk_init_queue(DAC960_RequestFunction,&Controller->queue_lock);
./z2ram.c: z2_queue = blk_init_queue(do_z2_request, &z2ram_lock);
./amiflop.c: disk->queue = blk_init_queue(do_fd_request, &amiflop_lock);
./xen-blkfront.c: rq = blk_init_queue(do_blkif_request, &blkif_io_lock);
./paride/pd.c: pd_queue = blk_init_queue(do_pd_request, &pd_lock);
./paride/pf.c: pf_queue = blk_init_queue(do_pf_request, &pf_spin_lock);
./paride/pcd.c: pcd_queue = blk_init_queue(do_pcd_request, &pcd_lock);
./mg_disk.c: host->breq = blk_init_queue(mg_request_poll, &host->lock);
./mg_disk.c: host->breq = blk_init_queue(mg_request, &host->lock);
./rbd.c: q = blk_init_queue(rbd_rq_fn, &rbd_dev->lock);

./sunvdc.c: q = blk_init_queue(do_vdc_request, &port->vio.lock);
./swim.c: swd->queue = blk_init_queue(do_fd_request, &swd->lock);
./xsysace.c: ace->queue = blk_init_queue(ace_request, &ace->lock);
./osdblk.c: q = blk_init_queue(osdblk_rq_fn, &osdev->lock);
./ps3disk.c: queue = blk_init_queue(ps3disk_request, &priv->lock);
./swim3.c: swim3_queue = blk_init_queue(do_fd_request, &swim3_lock);
./ub.c: if ((q = blk_init_queue(ub_request_fn, sc->lock)) == NULL)
./nbd.c: disk->queue = blk_init_queue(do_nbd_request, &nbd_lock);

Quoted 10 lines · James Bottomley

Quoted 3 lines · Jun'ichi Nomura

2) SCSI to call blk_cleanup_queue() from device's ->release() callback (before 2.6.39, it used to work like this)
https://lkml.org/lkml/2011/7/2/106

Well, they both have documented objections. I asked why we destroy the elevator in the del case and didn't get any traction, so let me show the actual patch which should fix all of these issues.

Is there a good reason for not doing this as a bug fix now?

I think it doesn't work because elevator_exit() and blk_throtl_exit() take &q->queue_lock, which may be freed by LLD after blk_cleanup_queue, before blk_release_queue.

Hi James,

Quoted 11 lines · Jun'ichi Nomura

Quoted 10 lines · James Bottomley

Quoted 3 lines · Jun'ichi Nomura

2) SCSI to call blk_cleanup_queue() from device's ->release() callback (before 2.6.39, it used to work like this)
https://lkml.org/lkml/2011/7/2/106

Well, they both have documented objections. I asked why we destroy the elevator in the del case and didn't get any traction, so let me show the actual patch which should fix all of these issues.

Is there a good reason for not doing this as a bug fix now?

...

Quoted 3 lines

I think it doesn't work because elevator_exit() and blk_throtl_exit() take &q->queue_lock, which may be freed by LLD after blk_cleanup_queue, before blk_release_queue.

If the reason you moved scsi_free_queue into scsi_remove_device is marking the queue dead, how about the following patch? Do you think it's acceptable?

Quoted 3 lines

If the reason you moved scsi_free_queue into scsi_remove_device is marking the queue dead, how about the following patch? Do you think it's acceptable?

Well, it's just hiding the problem. The essential problem is that only block has the correctly refcounted knowledge to know the last release of the queue reference. Until that time, the holder of the reference can use the queue regardless of whether blk_cleanup_queue() has been called. This is the race you complain about since use of the queue involves the lock which should be guarded by QUEUE_DEAD checks.

This is essentially unfixable with function calls. The only way to fix it is to have a callback model for freeing the external lock.

Quoted 11 lines · James Bottomley

Quoted 3 lines

If the reason you moved scsi_free_queue into scsi_remove_device is marking the queue dead, how about the following patch? Do you think it's acceptable?

Well, it's just hiding the problem. The essential problem is that only block has the correctly refcounted knowledge to know the last release of the queue reference. Until that time, the holder of the reference can use the queue regardless of whether blk_cleanup_queue() has been called. This is the race you complain about since use of the queue involves the lock which should be guarded by QUEUE_DEAD checks.

Quoted 2 lines

This is essentially unfixable with function calls. The only way to fix it is to have a callback model for freeing the external lock.

Assuming the queue is associated with a device, the queue could take a reference to the device, dropping that reference when the queue is freed. Then the lock could safely be freed at the same time as the device.

Quoted 10 lines

Quoted 9 lines

Well, it's just hiding the problem. The essential problem is that only block has the correctly refcounted knowledge to know the last release of the queue reference. Until that time, the holder of the reference can use the queue regardless of whether blk_cleanup_queue() has been called. This is the race you complain about since use of the queue involves the lock which should be guarded by QUEUE_DEAD checks.

This is essentially unfixable with function calls. The only way to fix it is to have a callback model for freeing the external lock.

Quoted 4 lines

Assuming the queue is associated with a device, the queue could take a reference to the device, dropping that reference when the queue is freed. Then the lock could safely be freed at the same time as the device.

If that assumption is correct, there's no point refcounting the queue at all because its use is entirely subordinated to the lifecycle of the associated device.

Plus all the wittering about my previous patch is
pointless, because blk_cleanup_queue() has to do the final put of the queue in the lock free path (otherwise the assumption is violated).

However, much as I'd like to accept this rosy view, the original oops that started all of this in 2.6.38 was someone caught something with a reference to a SCSI queue after the device release function had been called.

Quoted 4 lines · James Bottomley

However, much as I'd like to accept this rosy view, the original oops that started all of this in 2.6.38 was someone caught something with a reference to a SCSI queue after the device release function had been called.

Not according to your commit log. You wrote that the reference was taken after scsi_remove_device() had been called -- but the device release function is scsi_device_dev_release_usercontext().

The commit log of 86cbfb5607d4b81b1a993ff689bbd2addd5d3a9b ("[SCSI] put stricter guards on queue dead checks") does not explain about the move of scsi_free_queue().

But according to the discussion below, it seems the move was motivated to solve the following self-deadlock:
https://lkml.org/lkml/2011/4/12/9

[in the context of kblockd_workqueue]
blk_delay_work
__blk_run_queue
scsi_request_fn
put_device
(puts final sdev refcount)
scsi_device_dev_release
execute_in_process_context(scsi_device_dev_release_usercontext)
[execute immediately because it's in process context]
scsi_device_dev_release_usercontext
scsi_free_queue
blk_cleanup_queue
blk_sync_queue
(wait for blk_delay_work to complete...)

James, is my understanding correct?

If so, isn't it possible to move the scsi_free_queue back to the original place and solve the deadlock instead by avoiding the wait in the same context?

Hi James,

Quoted 23 lines · Jun'ichi Nomura

The commit log of 86cbfb5607d4b81b1a993ff689bbd2addd5d3a9b ("[SCSI] put stricter guards on queue dead checks") does not explain about the move of scsi_free_queue().

But according to the discussion below, it seems the move was motivated to solve the following self-deadlock:
https://lkml.org/lkml/2011/4/12/9

[in the context of kblockd_workqueue]
blk_delay_work
__blk_run_queue
scsi_request_fn
put_device
(puts final sdev refcount)
scsi_device_dev_release
execute_in_process_context(scsi_device_dev_release_usercontext)
[execute immediately because it's in process context]
scsi_device_dev_release_usercontext
scsi_free_queue
blk_cleanup_queue
blk_sync_queue
(wait for blk_delay_work to complete...)

Quoted 5 lines

James, is my understanding correct?

If so, isn't it possible to move the scsi_free_queue back to the original place and solve the deadlock instead by avoiding the wait in the same context?

Actually, Tejun has posted a patch to replace
execute_in_process_context() with queue_work()
and asking your review:

[PATCH RESEND] scsi: don't use execute_in_process_context()
https://lkml.org/lkml/2011/4/30/87

Do you think you can take the patch and revert the move of scsi_free_queue()?

Thanks,

Signature · 2 lines

Jun'ichi Nomura, NEC Corporation
--

Quoted 7 lines · Jun'ichi Nomura

Actually, Tejun has posted a patch to replace
execute_in_process_context() with queue_work()
and asking your review:

[PATCH RESEND] scsi: don't use execute_in_process_context()
https://lkml.org/lkml/2011/4/30/87

Quoted 2 lines

Do you think you can take the patch and revert the move of scsi_free_queue()?

I've tested with your suggestion (reverting the move of scsi_free_queue) and it works like a charm. I did not get any oops after that. I tested with a multipath setup on top of two iscsi targets. Using dd after logging out of some of one of the iscsi targets would trigger the oops. With this patch, it could not be triggered anymore.