Re: [PATCH] scsi: core: Call blk_mq_free_tag_set() earlier

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

On 6/28/22 18:17, Ming Lei wrote:
> On Tue, Jun 28, 2022 at 10:56:12AM -0700, Bart Van Assche wrote:
>> There are two .exit_cmd_priv implementations. Both implementations use the
>> SCSI host pointer. Make sure that the SCSI host pointer is valid when
>> .exit_cmd_priv is called by moving the .exit_cmd_priv calls from
>> scsi_device_dev_release() to scsi_forget_host(). Moving
>> blk_mq_free_tag_set() from scsi_device_dev_release() to scsi_forget_host()
>> is safe because scsi_forget_host() drains all the request queues that use
>> the host tag set. This guarantees that no requests are in flight and also
>> that no new requests will be allocated from the host tag set.
> 
> Not sure scsi_forget_host really drains all queues since it bypasses
> sdev which state is SDEV_DEL, so removal for this sdev could be
> in-progress, not done yet.

Ah, that's right. How about making scsi_forget_host() wait until all request
activity on the associated queues has stopped, e.g. by replacing the current
scsi_forget_host() implementation by the one below?

/**
  * scsi_forget_host() - Remove all SCSI devices from a host.
  * @shost: SCSI host to remove devices from.
  *
  * Removes all SCSI devices that have not yet been removed. For the SCSI devices
  * for which removal started before scsi_forget_host(), wait until the
  * associated request queue has reached the "dead" state. In that state it is
  * guaranteed that no new requests will be allocated and also that no requests
  * are in progress anymore.
  */
void scsi_forget_host(struct Scsi_Host *shost)
{
	struct scsi_device *sdev;

	might_sleep();

  restart:
	spin_lock_irq(shost->host_lock);
	list_for_each_entry(sdev, &shost->__devices, siblings) {
		if (sdev->sdev_state == SDEV_DEL &&
		    blk_queue_dead(sdev->request_queue)) {
			continue;
		}
		if (sdev->sdev_state == SDEV_DEL) {
			get_device(&sdev->sdev_gendev);
			spin_unlock_irq(shost->host_lock);

			while (!blk_queue_dead(sdev->request_queue))
				msleep(10);

			spin_lock_irq(shost->host_lock);
			put_device(&sdev->sdev_gendev);
			goto restart;
		}
		spin_unlock_irq(shost->host_lock);
		__scsi_remove_device(sdev);
		goto restart;
	}
	spin_unlock_irq(shost->host_lock);
}

Thanks,

Bart.