Re: [PATCH v5] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace
From: Lee Duncan
Date: Fri Oct 28 2022 - 11:05:33 EST
On 10/28/22 02:45, Wenchao Hao wrote:
On 2022/10/28 0:18, Lee Duncan wrote:
On 10/27/22 19:00, Wenchao Hao wrote:
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..2e0d1cd6d4ea 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1676,6 +1676,30 @@ static const char *iscsi_session_state_name(int state)
return name;
}
+static struct {
+ int value;
+ char *name;
+} iscsi_session_target_state_names[] = {
+ { ISCSI_SESSION_TARGET_UNBOUND, "UNBOUND" },
+ { ISCSI_SESSION_TARGET_ALLOCATED, "ALLOCATED" },
+ { ISCSI_SESSION_TARGET_BOUND, "BOUND" },
+ { ISCSI_SESSION_TARGET_UNBINDING, "UNBINDING" },
+};
+
+static const char *iscsi_session_target_state_name(int state)
+{
+ int i;
+ char *name = NULL;
+
+ for (i = 0; i < ARRAY_SIZE(iscsi_session_target_state_names); i++) {
+ if (iscsi_session_target_state_names[i].value == state) {
+ name = iscsi_session_target_state_names[i].name;
+ break;
+ }
+ }
+ return name;
+}
It seems like it might be more efficient to use the target state as the array index, so you don't have to loop to find the name, e.g. something like:
static char* iscsi_session_target_state_names[] = {
.ISCSI_SESSION_TARGET_UNBOUND = "UNBOUND",
.ISCSI_SESSION_TARGET_ALLOCATED = "ALLOCATED",
.ISCSI_SESSION_TARGET_BOUND = "BOUND",
.ISCSI_SESSION_TARGET_UNBINDING = "UNBINDING",
};
I know there are only 4 states, and it's only used for sysfs, so not sure it matters much.
It's a better implement, I would update it.
@@ -1961,6 +1987,15 @@ static void __iscsi_unbind_session(struct work_struct *work)
unsigned long flags;
unsigned int target_id;
+ spin_lock_irqsave(&session->lock, flags);
+ if (session->target_state != ISCSI_SESSION_TARGET_BOUND) {
+ spin_unlock_irqrestore(&session->lock, flags);
+ ISCSI_DBG_TRANS_SESSION(session, "Abort unbind sesison\n");
It'd be nice if this said more, since debugging "Abort unbind sessions" would require finding the sources. How about "Abort unbind session: not bound", for example?
Of course, I would updated.
@@ -264,6 +271,7 @@ struct iscsi_cls_session {
*/
pid_t creator;
int state;
+ int target_state; /* session target bind state */
int sid; /* session id */
void *dd_data; /* LLD private data */
struct device dev; /* sysfs transport/container device */
Thank you for sticking with this. It is very much appreciated.
I should apologize for taking this issue off for a long time because of my slow response.
What's more, should I add an acked-by or reviewed-by in my next patch?
Please add my Reviewed-by tag after you make these changes.
And no problem taking a while. I appreciate your effort!
--
Lee Duncan