| Message ID | 1497262209-5635-4-git-send-email-mchristi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2017-06-12 at 05:10 -0500, Mike Christie wrote: > +struct se_device *target_find_device(int id) > +{ > + struct se_device *dev; > + > + mutex_lock(&g_device_mutex); > + dev = idr_find(&devices_idr, id); > + mutex_unlock(&g_device_mutex); > + return dev; > +} > +EXPORT_SYMBOL(target_find_device); Hello Mike, Since target_find_device() does not increase any reference count, how is code that uses the return value of target_find_device() protected against concurrent removal of the returned device? Thanks, Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/12/2017 10:46 AM, Bart Van Assche wrote: > On Mon, 2017-06-12 at 05:10 -0500, Mike Christie wrote: >> +struct se_device *target_find_device(int id) >> +{ >> + struct se_device *dev; >> + >> + mutex_lock(&g_device_mutex); >> + dev = idr_find(&devices_idr, id); >> + mutex_unlock(&g_device_mutex); >> + return dev; >> +} >> +EXPORT_SYMBOL(target_find_device); > > Hello Mike, Hey, > > Since target_find_device() does not increase any reference count, how is code > that uses the return value of target_find_device() protected against concurrent > removal of the returned device? > I am still working on that. For the target_core_user case, we control the lifetime (we are blocking in the addition/deletion operations and using in there) or we are using it in a place where we have a refcount (configfs command route) to it so it will not be deleted under us. For the xcopy use, I was still trying to figure out how that worked before my patches. target_xcopy_locate_se_dev_e4 would return a device under the g_device_mutex but I could not figure out how it was protected from removal after that mutex was dropped. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-06-12 at 13:40 -0500, Mike Christie wrote: > For the xcopy use, I was still trying to figure out how that worked > before my patches. target_xcopy_locate_se_dev_e4 would return a device > under the g_device_mutex but I could not figure out how it was protected > from removal after that mutex was dropped. Hello Mike, I don't think that protection did already exist. Had you noticed the following patch: "[PATCH v6 12/33] target: Introduce target_get_device() and target_put_device()" (https://www.spinics.net/lists/target-devel/msg14535.html)? Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/12/2017 03:03 PM, Bart Van Assche wrote: > On Mon, 2017-06-12 at 13:40 -0500, Mike Christie wrote: >> For the xcopy use, I was still trying to figure out how that worked >> before my patches. target_xcopy_locate_se_dev_e4 would return a device >> under the g_device_mutex but I could not figure out how it was protected >> from removal after that mutex was dropped. > > Hello Mike, > > I don't think that protection did already exist. Had you noticed the > following patch: "[PATCH v6 12/33] target: Introduce target_get_device() > and target_put_device()" > (https://www.spinics.net/lists/target-devel/msg14535.html)? > Thanks. What about target_depend_item/target_undepend_item calls? That it would prevent removals while the device is in use. You would need get/put calls if you used my lookup/iter helpers while removal was already in progress and could race. Do we want to do both for my next posting? For the current uses (tcmu and xcopy), both are not needed I do not think, but do we want to make the interface for generic use? For the latter, I will build off of your patches then. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/12/2017 05:51 PM, Mike Christie wrote: > On 06/12/2017 03:03 PM, Bart Van Assche wrote: >> On Mon, 2017-06-12 at 13:40 -0500, Mike Christie wrote: >>> For the xcopy use, I was still trying to figure out how that worked >>> before my patches. target_xcopy_locate_se_dev_e4 would return a device >>> under the g_device_mutex but I could not figure out how it was protected >>> from removal after that mutex was dropped. >> >> Hello Mike, >> >> I don't think that protection did already exist. Had you noticed the >> following patch: "[PATCH v6 12/33] target: Introduce target_get_device() >> and target_put_device()" >> (https://www.spinics.net/lists/target-devel/msg14535.html)? >> > > Thanks. > > What about target_depend_item/target_undepend_item calls? That it would > prevent removals while the device is in use. > > You would need get/put calls if you used my lookup/iter helpers while > removal was already in progress and could race. > > Do we want to do both for my next posting? For the current uses (tcmu > and xcopy), both are not needed I do not think, but do we want to make > the interface for generic use? For the latter, I will build off of your > patches then. Oops. I actually need it in the removal path, so I cannot use target_depend_item, so I guess we need to use refcounts. I will do some testing and code review to see how xcopy works when we allow removals and only get a refcount to the device. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index d4090f7..9f2a527 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -880,6 +880,17 @@ sector_t target_to_linux_sector(struct se_device *dev, sector_t lb) } EXPORT_SYMBOL(target_to_linux_sector); +struct se_device *target_find_device(int id) +{ + struct se_device *dev; + + mutex_lock(&g_device_mutex); + dev = idr_find(&devices_idr, id); + mutex_unlock(&g_device_mutex); + return dev; +} +EXPORT_SYMBOL(target_find_device); + void target_init_device_idr(void) { idr_init(&devices_idr); diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h index b760711..3f015ab 100644 --- a/include/target/target_core_backend.h +++ b/include/target/target_core_backend.h @@ -105,6 +105,8 @@ sense_reason_t transport_generic_map_mem_to_cmd(struct se_cmd *, sense_reason_t passthrough_parse_cdb(struct se_cmd *cmd, sense_reason_t (*exec_cmd)(struct se_cmd *cmd)); +struct se_device *target_find_device(int id); + bool target_sense_desc_format(struct se_device *dev); sector_t target_to_linux_sector(struct se_device *dev, sector_t lb); bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
This adds a helper to find a se_device by dev_index. It will be used in the next patches so tcmu's netlink interface can execute commands on specific devices. Signed-off-by: Mike Christie <mchristi@redhat.com> --- drivers/target/target_core_device.c | 11 +++++++++++ include/target/target_core_backend.h | 2 ++ 2 files changed, 13 insertions(+)