On 11/24/20 6:09 AM, Bart Van Assche wrote:
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?
My argument wasn't so much about scsi_eh_lock_door() (which will be sent
only in rare cases), but rather scsi_eh_try_stu() and scsi_eh_tur() as
invoked from scsi_eh_test_devices() at the end of each SCSI EH step.
Both of which would need similar treatment.
Plus one maybe should revisit scsi_end_request(); that does refer to
struct request_queue *q = sdev->request_queue;
despite having a 'struct request' as argument; maybe one should modify
that to
struct request_queue *q = req->q;
to not start accounting on the wrong queue.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
