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@xxxxxxx>
> ---
> --- 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
