On Wed, 2023-06-07 at 07:27 +0200, Christoph Hellwig wrote:
> 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
ok
> 2) move the scsi_internal_device_block in what was
You mean scsi_stop_queue() here, right?
> 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.
ok
> 4) move the quiesce wait to scsi_target_block and make it per-
> tagset
ok
>
> > 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.
>
The reason is simple: I wasn't certain if dev_to_shost() could return
NULL, and preferred skipping the wait over an Oops. I hear you say that
dev_to_shost() can't go wrong, so I'll remove the NULL test.
> 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.
Can we do this separately, maybe?
Thanks,
Martin
