Re: [PATCH RESEND] remove the queue unlock in scsi_requset_fn

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Thu, 2012-08-16 at 10:35 +0900, Chanho Min wrote:
> > functions will occur in line.  I also don't see why the sdev reference
> > couldn't drop to zero here.
> scsi_request_fn is called under the lock of request_queue->queue_lock.
> If we drop the sdev reference to zero here,
> scsi_device_dev_release_usercontext is
> invoked and make request_queue to NULL. When caller of scsi_request_fn try to
> unlock request_queue->queue_lock, the oops is occurred.

I don't understand this explanation.

sdev->request_queue goes to NULL if the sdev refcount goes to zero (and
blk.  We have a copy though in the q variable, which is what we unlock.
That q variable only goes invalid if the queue ref count goes to zero.
If that happens, the queue release function will try to take the lock to
free the elevator and your patch will cause a deadlock.

There are only two possibilities here:

     1. The queue refcount can never reach zero within a request
        function because block ensures that it can unlock the queue lock
        on exit.  We could then remove this lock drop and acquire on the
        grounds that it is superfluous.
     2. The queue refcount does indeed go to zero and the queue gets
        released.  This would mean that all our lock; request_fn; unlock
        patterns do a use after free (in the block layer).  Your
        proposed patch doesn't fix this (and indeed would cause a
        deadlock on the release path).

I've cc'd Jens, because I don't entirely see why our

lock; request_fn; unlock

is safe against a racing blk_cleanup_queue().

James