bug fix: SCSI async scan sysfs -EEXIST problem
PATCH 1/1 bug fix: SCSI async scan sysfs -EEXIST problem
This corrects the kobject_add -EEXIST failure reported in
http://marc.info/?l=linux-scsi&m=117699334422336&w=2
Basically, there was a collision between async scsi scan's scsi_sysfs_add_devices() and asynchronous scanning kicked off by the fc transport's rport code. Both called scsi_sysfs_add_sdev() for the same sdev.
The problem was that async scsi scan's call did not hold the scan_mutex when making the call to scsi_sysfs_add_sdev(). Additionally, looking at the shost->async_scan flag, which is critical for whether the sdev gets enumerated or not, many uses were fuzzy. As a bit flag, it should have been under the scsi_host lock, but via the way it's used within scsi_scan.c, it should have been under the scan_mutex. It was positioned within the async scan semaphore, which didn't help anything. Key to its change, is that is done prior to releasing the scan_mutex after around the scsi_sysfs_add_sdev() calls. To avoid all the lock repositioning, I simply converted it to an atomic. A little overkill, but has the coherency it needs.
Confirmed by Josef that it corrected his problems.
-- james s
drivers/scsi/scsi_scan.c +12 -8
@@ -1081,7 +1081,8 @@ static int scsi_probe_and_add_lun(struct goto out_free_result; } - res =scsi_add_lun (sdev, result, &bflags, shost->async_scan); + res =scsi_add_lun (sdev, result, &bflags, +atomic_read (&shost->async_scan));if (res ==SCSI_SCAN_LUN_PRESENT ) {if (bflags &BLIST_KEY ) { sdev->lockable =0 ;
@@ -1470,7 +1471,7 @@ struct scsi_device *__scsi_add_device(stif (strncmp (scsi_scan_type,"none" ,4 ) ==0 )return ERR_PTR (-ENODEV ); -if (!shost->async_scan) +if (!atomic_read (&shost->async_scan))scsi_complete_async_scans (); starget =scsi_alloc_target (parent, channel, id);
@@ -1590,7 +1591,7 @@ void scsi_scan_target(struct device *parif (strncmp (scsi_scan_type,"none" ,4 ) ==0 )return ; -if (!shost->async_scan) +if (!atomic_read (&shost->async_scan))scsi_complete_async_scans ();mutex_lock (&shost->scan_mutex);
@@ -1638,7 +1639,7 @@ int scsi_scan_host_selected(struct Scsi_"%s: <%u:%u:%u>\n" , __FUNCTION__, channel, id, lun)); -if (!shost->async_scan) +if (!atomic_read (&shost->async_scan))scsi_complete_async_scans (); if (((channel != SCAN_WILD_CARD) && (channel > shost->max_channel)) ||
@@ -1687,7 +1688,7 @@ static struct async_scan_data *scsi_prepif (strncmp (scsi_scan_type,"sync" ,4 ) ==0 )return NULL ; -if (shost->async_scan) { +if (atomic_read (&shost->async_scan)) {printk ("%s called twice for host %d" , __FUNCTION__, shost->host_no);dump_stack ();
@@ -1702,8 +1703,9 @@ static struct async_scan_data *scsi_prep goto err;init_completion (&data->prev_finished); +atomic_set (&shost->async_scan,1 ); +spin_lock (&async_scan_lock); - shost->async_scan =1 ;if (list_empty (&scanning_hosts))complete (&data->prev_finished);list_add_tail (&data->list, &scanning_hosts);
@@ -1732,7 +1734,7 @@ static void scsi_finish_async_scan(strucreturn ; shost = data->shost; -if (!shost->async_scan) { +if (!atomic_read (&shost->async_scan)) {printk ("%s called twice for host %d" , __FUNCTION__, shost->host_no);dump_stack ();
@@ -1741,10 +1743,12 @@ static void scsi_finish_async_scan(strucwait_for_completion (&data->prev_finished); +mutex_lock (&shost->scan_mutex);scsi_sysfs_add_devices (shost); +atomic_set (&shost->async_scan,0 ); +mutex_unlock (&shost->scan_mutex);spin_lock (&async_scan_lock); - shost->async_scan =0 ;list_del (&data->list);if (!list_empty (&scanning_hosts)) { struct async_scan_data *next = list_entry(scanning_hosts.next,
include/scsi/scsi_host.h +2 -1
@@ -6,6 +6,7 @@#include <linux/types.h> #include <linux/workqueue.h> #include <linux/mutex.h> +#include <asm/atomic.h> struct request_queue ;struct block_device ;
@@ -605,7 +606,7 @@ struct Scsi_Host { unsigned tmf_in_progress:1;/* Asynchronous scan in progress */ - unsigned async_scan:1; +atomic_t async_scan;/* * Optional work queue to be utilized by the transport
drivers/scsi/scsi_scan.c static void scsi_finish_async_scan(struc · line 1749
Quoted 4 lines · James Smart
drivers/scsi/scsi_scan.c +3
-0+mutex_lock (&shost->scan_mutex);scsi_sysfs_add_devices (shost); +atomic_set (&shost->async_scan,0 ); +mutex_unlock (&shost->scan_mutex);
It really seems that the only safety here is expanding the scan mutex to cover more of the code. I don't really see any value to using the atomic types ... it's not a simultaneous variable update problem, it's a scan race which the mutex can be used to mediate.
James
Quoted 6 lines · James Bottomley
drivers/scsi/scsi_scan.c static void scsi_finish_async_scan(struc · line 1749
Quoted 4 lines · James Smart
drivers/scsi/scsi_scan.c +3
-0+mutex_lock (&shost->scan_mutex);scsi_sysfs_add_devices (shost); +atomic_set (&shost->async_scan,0 ); +mutex_unlock (&shost->scan_mutex);
Quoted 6 lines
It really seems that the only safety here is expanding the scan mutex to cover more of the code. I don't really see any value to using the atomic types ... it's not a simultaneous variable update problem, it's a scan race which the mutex can be used to mediate.
James
True. First requirement is that scsi_sysfs_add_devices() must be under the scan_mutex. Additionally, (ignoring the atomic) shost->async_scan, based on its use in scsi_add_lun(), should be changed under the mutex as well, thus the "proper" placement in the snippet above is while under the scan mutex.
The real headache was the other code paths that read the value of async_scan and determine whether to enumerate or ignore the sdev. Reorganizing that code to place it under scan_mutex was very ugly. Converting to an atomic simply addressed the synchronicity on sampling, although it essentially makes it independent of the scan_mutex.
Are you suggesting you would rather rework all the other code paths for scan_mutex encapsulating async_scan ?
-- james