On Wed, 2023-06-07 at 09:39 -0700, Bart Van Assche wrote:
> On 6/7/23 08:38, Martin Wilck wrote:
> > On Wed, 2023-06-07 at 07:05 -0700, Bart Van Assche wrote:
> > > On 6/7/23 02:26, Martin Wilck wrote:
> > > > 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:
> > > > > > 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(de
> > > > > > v),
> > > > > > 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.
> > >
> > > I propose to pass shost as the first argument to
> > > scsi_target_block()
> > > instead of using dev_to_shost() inside scsi_target_block().
> > > Except in
> > > __iscsi_block_session(), shost is already available as a local
> > > variable.
> >
> > If we do this, it might actually be cleaner to just pass the tag
> > set to
> > wait for.
>
> Wouldn't that be close to a layering violation? Shouldn't SCSI APIs
> accept
> pointers to SCSI objects instead of pointers to block layer
> abstractions?
My thought was that quiescing is based on tag sets in current kernels,
and passing in the tag set to scsi_target_block() would make that
explicit.
But you've got a point. I'll resubmit the with a Scsi_Host argument and
see how it goes.
Thanks,
Martin
