Re: [Patch] plug async scan race at 1st node scan

[Matthew Wilcox <matthew@wil.cx>]

On Mon, Aug 20, 2007 at 10:02:45AM -0400, James Smart wrote:
> Well - depends on what the semantics of scan_start are.... to date, there
> really are none....  and what does requiring it mean ?

OK, and to make this discussion exceptionally useful, all comments are
to be made in the context of modifications to the documentation:

        /*
         * If the host wants to be called before the scan starts, but
         * after the midlayer has set up ready for the scan, it can fill
         * in this function.
         */

When I say "require it", I mean that currently we have the code:

        if (shost->hostt->scan_finished) {
                unsigned long start = jiffies;
                if (shost->hostt->scan_start)
                        shost->hostt->scan_start(shost);

I propose removing that second 'if':

        if (shost->hostt->scan_finished) {
                unsigned long start = jiffies;
		shost->hostt->scan_start(shost);

And here's my proposal for new documentation for scan_start():

	/*
	 * If the host fills in scan_finished, above, it must also fill in
	 * scan_start.  scan_start will be called by the midlayer to inform
	 * the host that it is now ready to receive requests to scan targets.
	 */

> What's implied is that you want "bring up link" to be enabled in 
> scan_start().

That's certainly one obvious way to do it.  Another way would be to have
a flag in the driver blocking calls to the midlayer ... I think you have
that already?

> Doable, but the code paths weren't put together expecting this, so it may be
> a bit of work. I'll have to look at it.   Also, you're asking me to fix one
> driver, without thinking about the structure in others.... I'd rather the
> api itself locked down state/behavior, not simply the LLD coding.

I think other drivers already do this.  We only have three drivers
currently using this API -- lpfc, qla2xxx and aic94xx.  asd_scan_start()
enables the PHYs.  qla2xxx_scan_start sets some bits, but I'm not quite
sure of their effect.

> Before starting, I'd rather we setting on what the semantics of the api is.
> To the uninitiated, requiring a driver to call scsi_scan_host(), when the
> transport drives all discovery, and where the scan_host really has nothing
> to do with scanning, but rather creates a firewall delay to hold off 
> serializing
> of device enumerations - is all very confusing.  I'd rather we had a 
> different
> entry point for those things that supply start/finish routines and it was
> named more in line with "start discovery delay".

The name certainly isn't great, but I thought we agreed that having a
common entry point for all SCSI hosts was a good idea.

> Adding the notion of scan_start bringing up the link sounds reasonable. 
> However,
> how does this translate to other bus types ?

It works for SAS and FC ... I don't see why it wouldn't work for other
bus types.  Obviously, we don't call it for SPI.

> -- james s
> 
> Matthew Wilcox wrote:
> >On Mon, Aug 20, 2007 at 09:10:35AM -0400, James Smart wrote:
> >>In testing 2.6.23-rc3, there is a small window where the async-per-target
> >>scan of the transport can beat the call from the LLDD to scsi_scan_host().
> >
> >I'd assumed that events wouldn't come in until ->scan_start was called.
> >I see lpfc doesn't have one; is it possible to restructure it to have one?
> >
> >(In any case, good job tracking this down; it was really annoying me.)
> >
> >Possibly we should be less forgiving, and require drivers to have a
> >scan_start, otherwise they can't avoid this race.
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."