On 11/22/20 11:02 PM, Hannes Reinecke wrote:
> On 11/23/20 4:17 AM, 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}().
>>
> The description is now partially wrong, as scsi_mq_alloc_queue() is
> certainly not exported (why would it?).
> 
> And it also glosses over the main idea of this patch, namely that not
> only sdev->request_queue is frozen, but also a completely _new_
> request queue is allocated to send SPI DV requests over.
> 
> Please modify the description.

Hi Hannes,

Thanks for the feedback. Please take a look at the attached patch.

>> +    q2 = scsi_mq_alloc_queue(sdev);
>> +
>> +    if (q2) {
>> +        /*
>> +         * Freeze the target such that no other subsystem can submit
>> +         * SCSI commands to 'sdev'. Submitting SCSI commands through
>> +         * q2 may trigger the SCSI error handler. The SCSI error
>> +         * handler must be able to handle a frozen sdev->request_queue
>> +         * and must also use blk_mq_rq_from_pdu(q2)->q instead of
>> +         * sdev->request_queue if it would be necessary to access q2
>> +         * directly.
>> +         */
> 
> ... 'it would be necessary' indicates that it doesn't do so, currently.
> And the SPI DV code most certainly _is_ submitting SCSI commands.
> So doesn't the above imply that SCSI EH will fail to work correctly?
> And if so, why isn't it fixed with some later patch in this series?
> Or how do you plan to address it?
> Hmm?

Before SPI domain validation starts, sdev->request_queue is frozen which
means that DV will only start after all pending SCSI commands have
finished, including potential error handling for these commands.

As far as I know there is only one sdev->request_queue access in the
SCSI error handler, namely in scsi_eh_lock_door(). Patch 5/9 from this
series makes the blk_get_request() call in that function use
BLK_MQ_REQ_NOWAIT so the SCSI error handler should still work fine if a
DV command fails. Does this answer your question?

Thanks,

Bart.