Re: [PATCH] scsi: core: don't start concurrent async scan on same host

[Ming Lei <ming.lei@redhat.com>]

On Mon, Oct 19, 2020 at 07:17:52AM -0700, Bart Van Assche wrote:
> On 10/9/20 8:25 PM, Ming Lei wrote:
> > Current scsi host scan mechanism supposes to fallback into sync host
> > scan if async scan is in-progress. However, this rule isn't strictly
> > respected, because scsi_prep_async_scan() doesn't hold scan_mutex when
> > checking shost->async_scan. When scsi_scan_host() is called
> > concurrently, two async scan on same host may be started, and hang in
> > do_scan_async() is observed.
> > 
> > Fixes this issue by checking & setting shost->async_scan atomically
> > with shost->scan_mutex.
> 
> Did you perhaps mean "by serializing shost->async_scan accesses with
> shost->scan_mutex"?

Specifically, the following checking & setting has to be done atomically,
so shost->scan_mutex is required, just as what scsi_finish_async_scan()
does for clearing shost->async_scan:

scsi_prep_async_scan():

	if (!shost->async_scan)
		return NULL;
	...
	shost->async_scan = 1

> 
> It is not clear to me why the shost->async_scan assignment is protected
> with the host lock. Can spin_lock_irqsave(shost->host_lock, flags) and
> spin_unlock_irqrestore(shost->host_lock, flags) be left out from this
> function?

I think it is doable to remove the ->host_lock from both scsi_prep_async_scan()
and scsi_finish_async_scan(), which can be done as one follow-up cleanup.

With this patch, all reading/writing shost->async_scan are protected by
shost->scan_mutex.

> 
> Anyway:
> 
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Thanks!


-- 
Ming