Re: [PATCH] async scsi scanning, version 12

[Matthew Wilcox <matthew@wil.cx>]

On Thu, Sep 28, 2006 at 01:32:46PM -0700, Andrew Morton wrote:
> > +obj-$(CONFIG_SCSI)		+= scsi_wait_scan.o
> > +
> >  scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o constants.o \
> >  				   scsicam.o scsi_error.o scsi_lib.o \
> >  				   scsi_scan.o scsi_sysfs.o \
> 
> I think that's supposed to go into scsi_mod-y, at line 146.

Nope.  If CONFIG_SCSI is a module, we want this module around so people
can insmod it in order to ensure that scsi discovery is complete.  If
CONFIG_SCSI is built-in, we want this built-in too.

> > +static char scsi_scan_type[6] = "sync";
> 
> That wasted a byte.

If you specify "async" then the buffer needs to be 6 bytes long.

> >  
> > +static spinlock_t async_scan_lock = SPIN_LOCK_UNLOCKED;
> 
> DEFINE_SPINLOCK()

OK.

> > +int scsi_complete_async_scans(void)
> > +{
> > +	struct async_scan_data *data;
> > +
> > +	do {
> > +		if (list_empty(&scanning_hosts))
> > +			return 0;
> > +		data = kmalloc(sizeof(*data), GFP_KERNEL);
> > +		if (!data)
> > +			msleep(1);
> > +	} while (!data);
> 
> ick.  Immortal allocation loops are poor form.  If there's really no
> alternative, please use __GFP_NOFAIL, so people can easily grep for your
> sins.

It's not immortal.  The scanning threads will eventually terminate, and
so will this loop.

> > +	data->shost = NULL;
> > +	init_completion(&data->prev_finished);
> > +
> > +	spin_lock(&async_scan_lock);
> > +	if (list_empty(&scanning_hosts))
> > +		goto done;
> > +	list_add_tail(&data->list, &scanning_hosts);
> > +	spin_unlock(&async_scan_lock);
> 
> "If the list is not empty, stick something on it".
> 
> What an unusual thing to do.  I'm sure it makes sense, but some comments
> explaining what's going on would be nice.

OK.

> > +#ifdef MODULE
> > +/* Only exported for the benefit of scsi_wait_scan */
> > +EXPORT_SYMBOL_GPL(scsi_complete_async_scans);
> > +#endif
> 
> Is that actually needed?  AFACIT this .o file will just get linked against
> the one which contains scsi_wait_scan() anyway.

See above.