| Message ID | 20230117030114.2131734-1-zhongjinghua@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: fix iscsi rescan fails to create block | expand |
On 1/16/23 21:01, Zhong Jinghua wrote: > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c > index 42db9c52208e..e7893835b99a 100644 > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -1503,6 +1503,13 @@ void scsi_remove_device(struct scsi_device *sdev) > } > EXPORT_SYMBOL(scsi_remove_device); > > +static int scsi_device_try_get(struct scsi_device *sdev) > +{ > + if (!kobject_get_unless_zero(&sdev->sdev_gendev.kobj)) > + return -ENXIO; > + return 0; > +} > + > static void __scsi_remove_target(struct scsi_target *starget) > { > struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); > @@ -1521,9 +1528,7 @@ static void __scsi_remove_target(struct scsi_target *starget) > if (sdev->channel != starget->channel || > sdev->id != starget->id) > continue; > - if (sdev->sdev_state == SDEV_DEL || > - sdev->sdev_state == SDEV_CANCEL || > - !get_device(&sdev->sdev_gendev)) > + if (scsi_device_try_get(sdev)) > continue; > spin_unlock_irqrestore(shost->host_lock, flags); > scsi_remove_device(sdev); I think the patch will work ok. I don't think we want to mix in our own reference getting function that works on kobjects directly with the put_device use a little below that line above. Since this is the second time (looks like Hannes was wanting one when he originally fixed this) we've wanted a get_unless_zero type function did you send Greg a get_device_unless_zero type of patch already and was that rejected? Why doesn't scsi_forget_host have the same issue with other drivers and similar scan/delete/host-removal type of tests? Is there something that flushes those async scans? I'm just wondering if we can do something similar for the target removal or if the host removal needs a similar fix.
Hello, We also want to write get_device_unless_zero, I will try to make a patch. I'm not sure if this problem exists in other places and can be fixed in this way, because we only tested this problem. I want to fix this problem that is now tested first. Thanks. -----邮件原件----- 发件人: Mike Christie <michael.christie@oracle.com> 发送时间: 2023年1月19日 7:07 收件人: zhongjinghua <zhongjinghua@huawei.com>; jejb@linux.ibm.com; martin.petersen@oracle.com; bvanassche@acm.org; emilne@redhat.com; hare@suse.de 抄送: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; zhangyi (F) <yi.zhang@huawei.com>; yukuai (C) <yukuai3@huawei.com>; houtao (A) <houtao1@huawei.com> 主题: Re: [PATCH] scsi: fix iscsi rescan fails to create block On 1/16/23 21:01, Zhong Jinghua wrote: > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c > index 42db9c52208e..e7893835b99a 100644 > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -1503,6 +1503,13 @@ void scsi_remove_device(struct scsi_device > *sdev) } EXPORT_SYMBOL(scsi_remove_device); > > +static int scsi_device_try_get(struct scsi_device *sdev) { > + if (!kobject_get_unless_zero(&sdev->sdev_gendev.kobj)) > + return -ENXIO; > + return 0; > +} > + > static void __scsi_remove_target(struct scsi_target *starget) { > struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); > @@ -1521,9 +1528,7 @@ static void __scsi_remove_target(struct scsi_target *starget) > if (sdev->channel != starget->channel || > sdev->id != starget->id) > continue; > - if (sdev->sdev_state == SDEV_DEL || > - sdev->sdev_state == SDEV_CANCEL || > - !get_device(&sdev->sdev_gendev)) > + if (scsi_device_try_get(sdev)) > continue; > spin_unlock_irqrestore(shost->host_lock, flags); > scsi_remove_device(sdev); I think the patch will work ok. I don't think we want to mix in our own reference getting function that works on kobjects directly with the put_device use a little below that line above. Since this is the second time (looks like Hannes was wanting one when he originally fixed this) we've wanted a get_unless_zero type function did you send Greg a get_device_unless_zero type of patch already and was that rejected? Why doesn't scsi_forget_host have the same issue with other drivers and similar scan/delete/host-removal type of tests? Is there something that flushes those async scans? I'm just wondering if we can do something similar for the target removal or if the host removal needs a similar fix.
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 42db9c52208e..e7893835b99a 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -1503,6 +1503,13 @@ void scsi_remove_device(struct scsi_device *sdev) } EXPORT_SYMBOL(scsi_remove_device); +static int scsi_device_try_get(struct scsi_device *sdev) +{ + if (!kobject_get_unless_zero(&sdev->sdev_gendev.kobj)) + return -ENXIO; + return 0; +} + static void __scsi_remove_target(struct scsi_target *starget) { struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); @@ -1521,9 +1528,7 @@ static void __scsi_remove_target(struct scsi_target *starget) if (sdev->channel != starget->channel || sdev->id != starget->id) continue; - if (sdev->sdev_state == SDEV_DEL || - sdev->sdev_state == SDEV_CANCEL || - !get_device(&sdev->sdev_gendev)) + if (scsi_device_try_get(sdev)) continue; spin_unlock_irqrestore(shost->host_lock, flags); scsi_remove_device(sdev);
When the three iscsi operations delete, logout, and rescan are concurrent at the same time, there is a probability of failure to add disk through device_add_disk(). The concurrent process is as follows: T0: scan host // echo 1 > /sys/devices/platform/host1/scsi_host/host1/scan T1: delete target // echo 1 > /sys/devices/platform/host1/session1/target1:0:0/1:0:0:1/delete T2: logout // iscsiadm -m node --login T3: T2 scsi_queue_work T4: T0 bus_probe_device T0 T1 T2 T3 scsi_scan_target mutex_lock(&shost->scan_mutex); __scsi_scan_target scsi_report_lun_scan scsi_add_lun scsi_sysfs_add_sdev device_add kobject_add //create session1/target1:0:0/1:0:0:1/ ... bus_probe_device // Create block asynchronously mutex_unlock(&shost->scan_mutex); sdev_store_delete scsi_remove_device device_remove_file mutex_lock(scan_mutex) __scsi_remove_device res = scsi_device_set_state(sdev, SDEV_CANCEL) iscsi_if_recv_msg scsi_queue_work __iscsi_unbind_session session->target_id = ISCSI_MAX_TARGET __scsi_remove_target sdev->sdev_state == SDEV_CANCEL continue; // end, No delete kobject 1:0:0:1 iscsi_if_recv_msg transport->destroy_session(session) __iscsi_destroy_session iscsi_session_teardown iscsi_remove_session __iscsi_unbind_session iscsi_session_event device_del // delete session T4: // create the block, its parent is 1:0:0:1 // If kobject 1:0:0:1 does not exist, it won't go down __device_attach_async_helper device_lock ... __device_attach_driver driver_probe_device really_probe sd_probe device_add_disk register_disk device_add // error The block is created after the seesion is deleted. When T2 deletes the session, it will mark block'parent 1:0:01 as unusable: T2 device_del kobject_del sysfs_remove_dir __kernfs_remove // Mark the children under the session as unusable while ((pos = kernfs_next_descendant_post(pos, kn))) if (kernfs_active(pos)) atomic_add(KN_DEACTIVATED_BIAS, &pos->active); Then, create the block: T4 device_add kobject_add kobject_add_varg kobject_add_internal create_dir sysfs_create_dir_ns kernfs_create_dir_ns kernfs_add_one if ((parent->flags & KERNFS_ACTIVATED) && !kernfs_active(parent)) goto out_unlock; // return error This error will cause a warning: kobject_add_internal failed for block (error: -2 parent: 1:0:0:1). In the lower version (such as 5.10), there is no corresponding error handling, continuing to go down will trigger a kernel panic, so cc stable. Therefore, creating the block should not be done after deleting the session. More practically, we should ensure that the target under the session is deleted first, and then the session is deleted. In this way, there are two possibilities: 1) if the process(T1) of deleting the target execute first, it will grab the device_lock(), and the process(T4) of creating the block will wait for the deletion to complete. Then, block's parent 1:0:0:1 has been deleted, it won't go down. 2) if the process(T4) of creating block execute first, it will grab the device_lock(), and the process(T1) of deleting the target will wait for the creation block to complete. Then, the process(T2) of deleting the session should need wait for the deletion to complete. Fix it by removing the judgment of state equal to SDEV_CANCEL in __scsi_remove_target() to ensure the order of deletion. Then, it will wait for T1's mutex_lock(scan_mutex) and device_del() in __scsi_remove_device() will wait for T4's device_lock(dev). But we found that such a fix would cause the previous problem: commit 81b6c9998979 ("scsi: core: check for device state in __scsi_remove_target()"). So we use scsi_device_try_get() instead of get_devcie() to fix the previous problem. Fixes: 81b6c9998979 ("scsi: core: check for device state in __scsi_remove_target()") Cc: <stable@vger.kernel.org> Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com> --- drivers/scsi/scsi_sysfs.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)