| Message ID | 20220712221936.1199196-3-bvanassche@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Call blk_mq_free_tag_set() earlier | expand |
On 7/12/22 5:19 PM, Bart Van Assche wrote: > From: Ming Lei <ming.lei@redhat.com> > > Fix the race conditions between SCSI LLD kernel module unloading and SCSI > device and target removal by making sure that SCSI hosts are destroyed after > all associated target and device objects have been freed. > > Cc: Christoph Hellwig <hch@lst.de> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: Mike Christie <michael.christie@oracle.com> > Cc: Hannes Reinecke <hare@suse.de> > Cc: John Garry <john.garry@huawei.com> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > [ bvanassche: Reworked Ming's patch and split it ] > --- > drivers/scsi/hosts.c | 8 ++++++++ > drivers/scsi/scsi_scan.c | 7 +++++++ > include/scsi/scsi_host.h | 3 +++ > 3 files changed, 18 insertions(+) > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c > index ef6c0e37acce..8fa98c8d0ee0 100644 > --- a/drivers/scsi/hosts.c > +++ b/drivers/scsi/hosts.c > @@ -190,6 +190,13 @@ void scsi_remove_host(struct Scsi_Host *shost) > transport_unregister_device(&shost->shost_gendev); > device_unregister(&shost->shost_dev); > device_del(&shost->shost_gendev); > + > + /* > + * After scsi_remove_host() has returned the scsi LLD module can be > + * unloaded and/or the host resources can be released. Hence wait until > + * the dependent SCSI targets and devices are gone before returning. > + */ > + wait_event(shost->targets_wq, atomic_read(&shost->target_count) == 0); > } If we only wait here we can still hit the race I described right? Is the issue where we might be misunderstanding each other that the target removal is slightly different from the host removal? For host removal we call scsi_forget_host with the scan_mutex already held. So when scsi_forget_host loops over the devices we know that there is no thread doing: sdev_store_delete -> scsi_remove_device -> __scsi_remove_device -> blk_cleanup_queue Since the sdev_store_delete call to scsi_remove_device call also grabs the scan_mutex, we can't call scsi_forget_host until sdev_store_delete -> scsi_remove_device has returned. For target removal,__scsi_remove_target doesn't take the scan_mutex when checking the device state. So, we have this race: 1. syfs deletion runs sdev_store_delete -> scsi_remove_device and that takes the scan_mutex. It then sets the state to SDEV_DEL. 2. fc/iscsi thread does __scsi_remove_target and it sees the device is in the SDEV_DEL state. It skips the device and then we return from __scsi_remove_target without having waited on that device's removal like is done in other cases. If the only issue we are concerned with is blk_cleanup_queue completing when we remove the host or target, then for the target race above I think we can just use the scan_mutex in __scsi_remove_target (that function then would use __scsi_remove_device). If the issue is that there would be other threads holding a ref to the scsi_device and they can call into the driver. and we want to make sure those refs are dropped when scsi_remove_target returns then we need to do what I described in the other thread.
On 7/14/22 11:02 AM, Mike Christie wrote: > On 7/12/22 5:19 PM, Bart Van Assche wrote: >> From: Ming Lei <ming.lei@redhat.com> >> >> Fix the race conditions between SCSI LLD kernel module unloading and SCSI >> device and target removal by making sure that SCSI hosts are destroyed after >> all associated target and device objects have been freed. >> >> Cc: Christoph Hellwig <hch@lst.de> >> Cc: Ming Lei <ming.lei@redhat.com> >> Cc: Mike Christie <michael.christie@oracle.com> >> Cc: Hannes Reinecke <hare@suse.de> >> Cc: John Garry <john.garry@huawei.com> >> Signed-off-by: Ming Lei <ming.lei@redhat.com> >> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >> [ bvanassche: Reworked Ming's patch and split it ] >> --- >> drivers/scsi/hosts.c | 8 ++++++++ >> drivers/scsi/scsi_scan.c | 7 +++++++ >> include/scsi/scsi_host.h | 3 +++ >> 3 files changed, 18 insertions(+) >> >> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c >> index ef6c0e37acce..8fa98c8d0ee0 100644 >> --- a/drivers/scsi/hosts.c >> +++ b/drivers/scsi/hosts.c >> @@ -190,6 +190,13 @@ void scsi_remove_host(struct Scsi_Host *shost) >> transport_unregister_device(&shost->shost_gendev); >> device_unregister(&shost->shost_dev); >> device_del(&shost->shost_gendev); >> + >> + /* >> + * After scsi_remove_host() has returned the scsi LLD module can be >> + * unloaded and/or the host resources can be released. Hence wait until >> + * the dependent SCSI targets and devices are gone before returning. >> + */ >> + wait_event(shost->targets_wq, atomic_read(&shost->target_count) == 0); >> } > > If we only wait here we can still hit the race I described right? > Sorry Bart. Ignore this mail. I missed patch 1/4. I see we do the wait in __scsi_remove_target.
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c index ef6c0e37acce..8fa98c8d0ee0 100644 --- a/drivers/scsi/hosts.c +++ b/drivers/scsi/hosts.c @@ -190,6 +190,13 @@ void scsi_remove_host(struct Scsi_Host *shost) transport_unregister_device(&shost->shost_gendev); device_unregister(&shost->shost_dev); device_del(&shost->shost_gendev); + + /* + * After scsi_remove_host() has returned the scsi LLD module can be + * unloaded and/or the host resources can be released. Hence wait until + * the dependent SCSI targets and devices are gone before returning. + */ + wait_event(shost->targets_wq, atomic_read(&shost->target_count) == 0); } EXPORT_SYMBOL(scsi_remove_host); @@ -394,6 +401,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) INIT_LIST_HEAD(&shost->starved_list); init_waitqueue_head(&shost->host_wait); mutex_init(&shost->scan_mutex); + init_waitqueue_head(&shost->targets_wq); index = ida_alloc(&host_index_ida, GFP_KERNEL); if (index < 0) { diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index 4c1efd6a3b0c..ac6059702d13 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -406,9 +406,14 @@ static void scsi_target_destroy(struct scsi_target *starget) static void scsi_target_dev_release(struct device *dev) { struct device *parent = dev->parent; + struct Scsi_Host *shost = dev_to_shost(parent); struct scsi_target *starget = to_scsi_target(dev); kfree(starget); + + if (atomic_dec_return(&shost->target_count) == 0) + wake_up(&shost->targets_wq); + put_device(parent); } @@ -523,6 +528,8 @@ static struct scsi_target *scsi_alloc_target(struct device *parent, starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED; init_waitqueue_head(&starget->sdev_wq); + atomic_inc(&shost->target_count); + retry: spin_lock_irqsave(shost->host_lock, flags); diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 667d889b92b5..339f975d356e 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -689,6 +689,9 @@ struct Scsi_Host { /* ldm bits */ struct device shost_gendev, shost_dev; + atomic_t target_count; + wait_queue_head_t targets_wq; + /* * Points to the transport data (if any) which is allocated * separately