Re: [PATCH, resend] Fix race between starved list processing and device removal

[Bart Van Assche <bvanassche@acm.org>]

On 10/07/12 12:47, James Bottomley wrote:
> On Mon, 2012-09-24 at 15:14 +0200, Bart Van Assche wrote:
>> Avoid that the sdev reference count can drop to zero before
>> the queue is run by scsi_run_queue(). Also avoid that the sdev
>> reference count can drop to zero in the same function by invoking
>> __blk_run_queue().
> [...]		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
>> @@ -977,6 +978,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
>>   	blk_cleanup_queue(sdev->request_queue);
>>   	cancel_work_sync(&sdev->requeue_work);
>>
>> +	spin_lock_irqsave(shost->host_lock, flags);
>> +	list_del(&sdev->starved_entry);
>> +	spin_unlock_irqrestore(shost->host_lock, flags);
>> +
>
> This hunk doesn't make much sense.  It seems to be orthogonal to the
> problem listed in the changelog and this action is done on last put
> anyway.

Removing an sdev from the starved list in __scsi_remove_device() has the 
advantage that it is guaranteed that the get_device() call added in 
scsi_run_queue() will succeed. A possible alternative is to leave the 
starved list removal code in scsi_device_dev_release_usercontext() and 
to invoke __blk_run_queue() in scsi_run_queue() only if the get_device() 
call in that function succeeded. Does this mean that you prefer the 
second option - something like the (untested) code below ?

		if (get_device(&sdev->sdev_gendev)) {
			spin_unlock(shost->host_lock);

			spin_lock(sdev->request_queue->queue_lock);
			__blk_run_queue(sdev->request_queue);
			spin_unlock(sdev->request_queue->queue_lock);

			put_device(&sdev->sdev_gendev);
			spin_lock(shost->host_lock);
		}

Bart.