On Sun, 2020-05-10 at 17:50 -0400, Douglas Gilbert wrote:
> On 2020-05-10 2:52 p.m., James Bottomley wrote:
> > On Sun, 2020-05-10 at 14:32 -0400, Douglas Gilbert wrote:
> > > This gem is in scsi_scan.c in the documentation of that
> > > function's first argument. "need not be a scsi host" should read
> > > "it damn well better be a scsi host" otherwise that function will
> > > crash and burn!
> >
> > It shouldn't: several transport classes, like SAS and FC have
> > intermediate devices between the host and the target and they all
> > work just fine using the non-host parent. Since you don't give the
> > error this is just guesswork, but the host has to be somewhere in
> > the parent chain otherwise dev_to_shost(parent) will return NULL
> > ... is that your problem?
>
> May be that "need not be a scsi host" should be expanded to something
> that suggests dev_to_shost(first_arg) can resolve to a non-NULL
> pointer.
>
> Are there restrictions on those intermediate object(s)? Can there be
> more than one? If so, can those intermediate objects only form a
> linear chain or could it be more complex, an object subtree for
> example?
I think you can read the current code as well as I can:
static inline struct Scsi_Host *dev_to_shost(struct device *dev)
{
while
(!scsi_is_host_device(dev)) {
if (!dev->parent)
return NULL;
dev =
dev->parent;
}
return container_of(dev, struct Scsi_Host,
shost_gendev);
}
The broad point is that this is open source, not some proprietary OS.
I'm not giving you an API set in stone and you have to figure out the
use cases, I'm telling you how the current API behaves and the reason
why it behaves this way. If there's an expansion or change you need,
provided it supports the current uses and is maintainable, it can be
done. What you have to tell me is what you're trying to do.
> > > I'm trying to work out why the function:
> > > starget_for_each_device() in scsi.c does _not_ use that
> > > collection right in front of it (i.e. scsi_target::devices).
> > > Instead, it step up to the host level, and iterates over all
> > > devices (LUs) on that host and only calls the given function for
> > > those devices that match the channel and target numbers.
> > > That is bizarrely wasteful if scsi_target::devices could be
> > > iterated over instead.
> > >
> > > Anyone know why this is?
> >
> > Best guess would be it wasn't converted over when the target list
> > was introduced.
>
> Okay, I'll change it and see what breaks.
>
>
> If you are not familiar with "mark"s in xarrays, they are binary
> flags hidden inside the pointers that xarray holds to form a
> container. With these flags (two available per xarray) one can make
> iterations much more efficient with xa_for_each_marked() { }. The win
> is that there is no need to dereference the pointers of collection
> members that are _not_ marked. After playing with these in my rework
> of the sg driver, I concluded that the best thing to mark was object
> states. Unfortunately there are only 2 marks *** per xarray but
> scsi_device, for example, has 9 states in scsi_device_state. Would
> you like to hazard a guess of which two (or two groups), if any, are
> iterated over often enough to make marking worthwhile?
>
> Those two marks could be stretched to four, sort of, as scsi_device
> objects are members of two xarrays in my xarray rework: all sdev
> objects in a starget, and all sdev objects in a shost.
I've got to say that sounds like a solution looking for a problem.
The fast path of SCSI is pretty rigorously checked with high iops
devices these days. The fact that this iterator is really lock heavy
but has never been fingered in any of the traces indicates that it's
probably not really a problem that needs solving.
James
