Re: [PATCH v2 3/3] scsi: simplify scsi_stop_queue()

[Christoph Hellwig <hch@lst.de>]

On Tue, Jun 06, 2023 at 09:38:45PM +0200, mwilck@suse.com 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.