On Tue, Jun 06, 2023 at 09:38:45PM +0200, mwilck@xxxxxxxx wrote:
> Simplify scsi_stop_queue(), which is only called in this code path, to never
> wait for the quiescing to finish. Rather call blk_mq_wait_quiesce_done()
> from scsi_target_block() after iterating over all devices.
I don't think simplify is the right word here. The code isn't in any
way simpler, it just is more efficient an shifts work from
scsi_stop_queue to scsi_internal_device_block and scsi_target_block.
But the whole transformation is very confusing to me even if it looks
correct in the end, and it took me quite a while to understand it.
I'd suggest to further split this up and include some additional
cleanups:
1) remove scsi_internal_device_block and fold it into device_block
2) move the scsi_internal_device_block in what was
scsi_internal_device_block and now is device_block out
of state_mutex (and document in the commit log why this is safe)
3) remove scsi_stop_queue and open code it in the two callers, one
of which currently wants nowait semantics, and one that doesn't.
4) move the quiesce wait to scsi_target_block and make it per-tagset
> scsi_target_block(struct device *dev)
> {
> + struct Scsi_Host *shost = dev_to_shost(dev);
> +
> if (scsi_is_target_device(dev))
> starget_for_each_device(to_scsi_target(dev), NULL,
> device_block);
> else
> device_for_each_child(dev, NULL, target_block);
> +
> + /* Wait for ongoing scsi_queue_rq() calls to finish. */
> + if (!WARN_ON_ONCE(!shost))
How could host ever be NULL here? I can't see why we'd want this
check.
Btw, as far as I can tell scsi_target_block is never called for
a device that is a target device. It might be worth throwing in
another patch to remove support for that case and simplify things
further.
