Re: [RFC] scsi: reduce protection of scan_mutex in scsi_remove_device

[Song Liu <songliubraving@fb.com>]

> On Apr 25, 2017, at 3:17 PM, Bart Van Assche <Bart.VanAssche@sandisk.com> wrote:
> 
> On Tue, 2017-04-25 at 21:17 +0000, Song Liu wrote:
>> I am not sure I fully understand the problem here. If I understand the logic 
>> correctly, when a device is being removed, it will stay in scsi_host->__devices
>> until fully the remove routine is finished. And LUN scanning in parallel will
>> find the device with scsi_device_lookup_by_target(), and thus it would not 
>> rescan the device until the device is fully removed? Did I miss anything here?
> 
> Hello Song,
> 
> The SCSI core is already complicated enough. Please don't complicate it further
> by making subtle changes to the semantics of scan_mutex. Please also note that
> I have proposed an alternative, namely to make the START STOP UNIT command
> asynchronous.
> 

Hello Bart, 

I total agree with your concern in making SCSI core more complicated. However, I am
not sure whether how asynchronous START STOP UNIT command makes multiple deletes run
in parallel. If I understand correctly, each device delete has the following steps:
  1. lock scan_mutex
  2. issue async START STOP UNIT 
  3. wait START STOP UNIT to complete
  4. unlock scan_mutex

scan_mutex will prevent multiple device deletes to run in parallel. 

On the other hand, maybe the problem is simpler than I thought? Once we ensure all
slave_destroy() holds proper lock to access host level data, something as follows
might just work. In this way, echoing into delete handle is not protected by 
scan_mutex. However, when the sysfs entry exists, the device should be fully added 
to the system. And before delete operation finishes, future scan should not re-add 
the device (as the device can be found by scsi_device_lookup_by_target). 

Am I too optimistic here?

Thanks,
Song


diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 82dfe07..d1c3338 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -710,7 +710,7 @@ sdev_store_delete(struct device *dev, struct device_attribute *attr,
                  const char *buf, size_t count)
 {
        if (device_remove_file_self(dev, attr))
-               scsi_remove_device(to_scsi_device(dev));
+               __scsi_remove_device(to_scsi_device(dev));
        return count;
 };