Re: [PATCH RFC 5/6] scsi_transport_spi: Freeze request queues instead of quiescing

[Alan Stern <stern@rowland.harvard.edu>]

On Sun, Aug 30, 2020 at 07:53:56PM -0700, Bart Van Assche wrote:
> Instead of quiescing the request queues involved in domain validation,
> freeze these. As a result, the struct request_queue pm_only member is no
> longer set during domain validation. That will allow to modify
> scsi_execute() such that it stops setting the BLK_MQ_REQ_PREEMPT flag.
> Three additional changes in this patch are that scsi_mq_alloc_queue() is
> exported, that scsi_device_quiesce() is no longer exported and that
> scsi_target_{quiesce,resume}() have been changed into
> scsi_target_{freeze,unfreeze}().
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

> --- a/drivers/scsi/scsi_transport_spi.c
> +++ b/drivers/scsi/scsi_transport_spi.c
> @@ -997,59 +997,75 @@ void
>  spi_dv_device(struct scsi_device *sdev)
>  {
>  	struct scsi_target *starget = sdev->sdev_target;
> +	struct request_queue *q1, *q2;
>  	u8 *buffer;
>  	const int len = SPI_MAX_ECHO_BUFFER_SIZE*2;
>  
>  	/*
> -	 * Because this function and the power management code both call
> -	 * scsi_device_quiesce(), it is not safe to perform domain validation
> -	 * while suspend or resume is in progress. Hence the
> -	 * lock/unlock_system_sleep() calls.
> +	 * Because creates a new request queue that is not visible to the rest
> +	 * of the system, domain validation must be serialized against suspend,
> +	 * resume and runtime power management. Hence the
> +	 * lock/unlock_system_sleep() and scsi_autopm_{get,put}_device() calls.
>  	 */
>  	lock_system_sleep();
>  
> +	if (scsi_autopm_get_device(sdev))
> +		goto unlock_system_sleep;
> +
>  	if (unlikely(spi_dv_in_progress(starget)))
> -		goto unlock;
> +		goto put_autopm;
>  
>  	if (unlikely(scsi_device_get(sdev)))
> -		goto unlock;
> +		goto put_autopm;
>  
>  	spi_dv_in_progress(starget) = 1;
>  
>  	buffer = kzalloc(len, GFP_KERNEL);
>  
>  	if (unlikely(!buffer))
> -		goto out_put;
> -
> -	/* We need to verify that the actual device will quiesce; the
> -	 * later target quiesce is just a nice to have */
> -	if (unlikely(scsi_device_quiesce(sdev)))
> -		goto out_free;
> -
> -	scsi_target_quiesce(starget);
> +		goto put_sdev;
>  
>  	spi_dv_pending(starget) = 1;

I'm not familiar with this code.  What happens if the test and 
assignment of spi_dv_in_progress() race between two threads?

It looks like the first to acquire the spi_dv_mutex will do a domain 
validation, then clear the spi_dv_in_progress and spi_dv_pending flags.
Then the second thread will perform another domain validation with 
those flags set to 0.  Is it supposed to work like that?

>  	mutex_lock(&spi_dv_mutex(starget));
>  
>  	starget_printk(KERN_INFO, starget, "Beginning Domain Validation\n");
>  
> -	spi_dv_device_internal(sdev, sdev->request_queue, buffer);
> +	/*
> +	 * Save the request queue pointer before it is overwritten by
> +	 * scsi_mq_alloc_queue().
> +	 */
> +	q1 = sdev->request_queue;
> +	q2 = scsi_mq_alloc_queue(sdev);
> +
> +	if (q2) {
> +		/*
> +		 * Restore the request queue pointer such that no other
> +		 * subsystem can submit SCSI commands to 'sdev'.
> +		 */

Too bad there's a little window here during which external commands can 
get sent to q2.

> +		sdev->request_queue = q1;
> +		scsi_target_freeze(starget);
> +		spi_dv_device_internal(sdev, q2, buffer);
> +		blk_cleanup_queue(q2);
> +		scsi_target_unfreeze(starget);
> +	}

No error message if the domain validation couldn't be performed?

Also, do you need to restore sdev->request_queue if the allocation 
failed?  It would be a little safer to move the restoration line 
immediately after the scsi_mq_alloc_queue call.

Ideally scsi_mq_alloc_queue would return q2 without assigning it to 
sdev->request_queue.

>  
>  	starget_printk(KERN_INFO, starget, "Ending Domain Validation\n");
>  
>  	mutex_unlock(&spi_dv_mutex(starget));
>  	spi_dv_pending(starget) = 0;
>  
> -	scsi_target_resume(starget);
> -
>  	spi_initial_dv(starget) = 1;
>  
> - out_free:
>  	kfree(buffer);
> - out_put:
> +
> +put_sdev:
>  	spi_dv_in_progress(starget) = 0;
>  	scsi_device_put(sdev);
> -unlock:
> +
> +put_autopm:
> +	scsi_autopm_put_device(sdev);
> +
> +unlock_system_sleep:
>  	unlock_system_sleep();
>  }
>  EXPORT_SYMBOL(spi_dv_device);

Alan Stern