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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux