On 05/20/20 17:50, Pavel Zakharov wrote:
Hopefully I didn’t mess-up the recipients/Subject list as I just subscribed to target-devel and so following-up to an email I found in the archive.
Bodo, I happened to have run into the same issue and was just about to submit a similar patch when I randomly stumbled on yours.
I think we can improve on that patch by moving the call to transport_lun_remove_cmd() into transport_cmd_check_stop_to_fabric().
Yeah, I also thought about doing this. But then decided to better
add the one lost line for fixing only.
Here’s the patch I was about to propose. Let me know what you think.
Note that we could also do this simplification as a separate patch later if you think it’s better.
I think it is a good idea to do this simplification in a separate patch.
---
When ABORT and LUN RESET were made synchronous, a call to
transport_lun_remove_cmd() was acidentally removed. This caused a lun_ref
to be leaked every time an ABORT or LUN RESET was processed in
target_tmr_work().
Given that transport_lun_remove_cmd() should always be called before
transport_cmd_check_stop_to_fabric(), move that call into this function.
This ensures that the lun_ref is released before setting .se_lun to NULL.
Signed-off-by: Pavel Zakharov <pavel.zakharov@xxxxxxxxxxx>
---
drivers/target/target_core_transport.c | 36 ++++++++++++--------------
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 594b724bbf79..ec578822709a 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -653,6 +653,17 @@ static void target_remove_from_state_list(struct se_cmd *cmd)
spin_unlock_irqrestore(&dev->execute_task_lock, flags);
}
+static void transport_lun_remove_cmd(struct se_cmd *cmd)
+{
+ struct se_lun *lun = cmd->se_lun;
+
+ if (!lun)
+ return;
+
+ if (cmpxchg(&cmd->lun_ref_active, true, false))
+ percpu_ref_put(&lun->lun_ref);
+}
+
/*
* This function is called by the target core after the target core has
* finished processing a SCSI command or SCSI TMF. Both the regular command
@@ -664,6 +675,11 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd)
{
unsigned long flags;
+ /*
+ * Remove cmd ref from lun if needed before clearing the se_lun.
+ */
+ transport_lun_remove_cmd(cmd);
+
target_remove_from_state_list(cmd);
/*
@@ -698,17 +714,6 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd)
return cmd->se_tfo->check_stop_free(cmd);
}
-static void transport_lun_remove_cmd(struct se_cmd *cmd)
-{
- struct se_lun *lun = cmd->se_lun;
-
- if (!lun)
- return;
-
- if (cmpxchg(&cmd->lun_ref_active, true, false))
- percpu_ref_put(&lun->lun_ref);
-}
-
static void target_complete_failure_work(struct work_struct *work)
{
struct se_cmd *cmd = container_of(work, struct se_cmd, work);
@@ -799,8 +804,6 @@ static void target_handle_abort(struct se_cmd *cmd)
WARN_ON_ONCE(kref_read(&cmd->cmd_kref) == 0);
- transport_lun_remove_cmd(cmd);
-
transport_cmd_check_stop_to_fabric(cmd);
}
@@ -1726,7 +1729,6 @@ static void target_complete_tmr_failure(struct work_struct *work)
se_cmd->se_tmr_req->response = TMR_LUN_DOES_NOT_EXIST;
se_cmd->se_tfo->queue_tm_rsp(se_cmd);
- transport_lun_remove_cmd(se_cmd);
transport_cmd_check_stop_to_fabric(se_cmd);
}
@@ -1918,7 +1920,6 @@ void transport_generic_request_failure(struct se_cmd *cmd,
goto queue_full;
check_stop:
- transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
return;
@@ -2216,7 +2217,6 @@ static void transport_complete_qf(struct se_cmd *cmd)
transport_handle_queue_full(cmd, cmd->se_dev, ret, false);
return;
}
- transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
}
@@ -2311,7 +2311,6 @@ static void target_complete_ok_work(struct work_struct *work)
if (ret)
goto queue_full;
- transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
return;
}
@@ -2337,7 +2336,6 @@ static void target_complete_ok_work(struct work_struct *work)
if (ret)
goto queue_full;
- transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
return;
}
@@ -2373,7 +2371,6 @@ static void target_complete_ok_work(struct work_struct *work)
if (ret)
goto queue_full;
- transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
return;
}
@@ -2409,7 +2406,6 @@ static void target_complete_ok_work(struct work_struct *work)
break;
}
- transport_lun_remove_cmd(cmd);
transport_cmd_check_stop_to_fabric(cmd);
return;
--
2.21.1 (Apple Git-122.3)
