On 08/20/2019 09:02 AM, Bart Van Assche wrote:
> On 8/20/19 2:04 AM, Naohiro Aota wrote:
>> If there is no corresponding scsi_device for a LUN,
>> tcm_loop_port_unlink() complains that it "Unable to locate struct
>> scsi_device for " the device and keep %tl_tpg_port_count as is. However,
>> such situation is legal when we delete a SCSI device using
>> /sys/class/scsi_device/${lun}/device/delete. We can safely ignore the
>> missing SCSI device case here.
>>
>> Signed-off-by: Naohiro Aota <naohiro.aota@xxxxxxx>
>> ---
>> drivers/target/loopback/tcm_loop.c | 18 +++++++++---------
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/target/loopback/tcm_loop.c
>> b/drivers/target/loopback/tcm_loop.c
>> index 3305b47fdf53..0942f3bd7eec 100644
>> --- a/drivers/target/loopback/tcm_loop.c
>> +++ b/drivers/target/loopback/tcm_loop.c
>> @@ -654,16 +654,16 @@ static void tcm_loop_port_unlink(
>> sd = scsi_device_lookup(tl_hba->sh, 0, tl_tpg->tl_tpgt,
>> se_lun->unpacked_lun);
>> - if (!sd) {
>> - pr_err("Unable to locate struct scsi_device for %d:%d:%llu\n",
>> - 0, tl_tpg->tl_tpgt, se_lun->unpacked_lun);
>> - return;
>> + if (sd) {
>> + /*
>> + * Remove Linux/SCSI struct scsi_device by HCTL
>> + */
>> + scsi_remove_device(sd);
>> + scsi_device_put(sd);
>> + } else {
>> + pr_debug("Unable to locate struct scsi_device for %d:%d:%llu\n",
>> + 0, tl_tpg->tl_tpgt, se_lun->unpacked_lun);
>> }
>> - /*
>> - * Remove Linux/SCSI struct scsi_device by HCTL
>> - */
>> - scsi_remove_device(sd);
>> - scsi_device_put(sd);
>> atomic_dec_mb(&tl_tpg->tl_tpg_port_count);
>
> The above patch looks wrong to me. I think this patch does not fix the
> reference leak present in the current code. Have you considered to
> modify tcm_loop_port_link() such that it saves the pointer returned by
> scsi_add_device() and to use that pointer in tcm_loop_port_unlink()?
>
Are you guys talking about different issues?
tcm loop does not take a reference to the scsi_device at creation/link
time then need to release at removal/unlink time. The above
scsi_device_put is for the successful scsi_device_lookup call. tcm loop
works like a scsi host driver that does its own scanning via
scsi_add_device (maybe similar to scsi drivers that are raid cards).
Like other host drivers it does not take a reference to the device when
it is added and relies on scsi-ml to handle all that for it before doing
operations like queuecommand.
The leak is if you removed the scsi_device via the scsi ml sysfs
interface then there is no way to completely unlink the lio port because
if scsi_device_lookup fails we return from the function and do not do
not release our refcount on the tl_tpg_port_count.
