[PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[lanevdenoche]

Calling isert_setup_id() from isert_np_cma_handler() is wrong
since at that time the socket address was still bound to the old cma_id
which will be destroyed via rdma_destroy_id() only after processing
the RDMA_CM_EVENT_ADDR_CHANGE event.

 - isert_np_cma_handler() calls isert_setup_id()

 - isert_setup_id() calls rdma_bind_addr()

 - rdma_bind_addr() returns -EADDRINUSE

Move the creation of the cma_id in workqueue context and delete old
cma_id directly, not through returning the error code to the upper
level.

Fixes: ca6c1d82d12d ("iser-target: Handle ADDR_CHANGE event for listener cm_id")
Signed-off-by: Chesnokov Gleb <Chesnokov.G@raidix.com>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 41 ++++++++++++++++++++-----
 drivers/infiniband/ulp/isert/ib_isert.h |  2 ++
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 636d590765f9..de5ab2ae8e17 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -597,10 +597,26 @@ isert_conn_terminate(struct isert_conn *isert_conn)
 			   isert_conn);
 }
 
+static void isert_np_reinit_id_work(struct work_struct *w)
+{
+	struct isert_np *isert_np = container_of(w, struct isert_np, work);
+
+	rdma_destroy_id(isert_np->cm_id);
+
+	isert_np->cm_id = isert_setup_id(isert_np);
+	if (IS_ERR(isert_np->cm_id)) {
+		isert_err("isert np %p setup id failed: %ld\n",
+			  isert_np, PTR_ERR(isert_np->cm_id));
+		isert_np->cm_id = NULL;
+	}
+}
+
 static int
 isert_np_cma_handler(struct isert_np *isert_np,
 		     enum rdma_cm_event_type event)
 {
+	int ret = -1;
+
 	isert_dbg("%s (%d): isert np %p\n",
 		  rdma_event_msg(event), event, isert_np);
 
@@ -609,19 +625,15 @@ isert_np_cma_handler(struct isert_np *isert_np,
 		isert_np->cm_id = NULL;
 		break;
 	case RDMA_CM_EVENT_ADDR_CHANGE:
-		isert_np->cm_id = isert_setup_id(isert_np);
-		if (IS_ERR(isert_np->cm_id)) {
-			isert_err("isert np %p setup id failed: %ld\n",
-				  isert_np, PTR_ERR(isert_np->cm_id));
-			isert_np->cm_id = NULL;
-		}
+		queue_work(isert_np->reinit_id_wq, &isert_np->work);
+		ret = 0;
 		break;
 	default:
 		isert_err("isert np %p Unexpected event %d\n",
 			  isert_np, event);
 	}
 
-	return -1;
+	return ret;
 }
 
 static int
@@ -2272,6 +2284,15 @@ isert_setup_np(struct iscsi_np *np,
 	if (!isert_np)
 		return -ENOMEM;
 
+	isert_np->reinit_id_wq = alloc_ordered_workqueue("isert_reinit_id_wq", WQ_MEM_RECLAIM);
+	if (unlikely(!isert_np->reinit_id_wq)) {
+		isert_err("Unable to allocate reinit workqueue\n");
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	INIT_WORK(&isert_np->work, isert_np_reinit_id_work);
+
 	sema_init(&isert_np->sem, 0);
 	mutex_init(&isert_np->mutex);
 	INIT_LIST_HEAD(&isert_np->accepted);
@@ -2288,7 +2309,7 @@ isert_setup_np(struct iscsi_np *np,
 	isert_lid = isert_setup_id(isert_np);
 	if (IS_ERR(isert_lid)) {
 		ret = PTR_ERR(isert_lid);
-		goto out;
+		goto free_wq;
 	}
 
 	isert_np->cm_id = isert_lid;
@@ -2296,6 +2317,8 @@ isert_setup_np(struct iscsi_np *np,
 
 	return 0;
 
+free_wq:
+	destroy_workqueue(isert_np->reinit_id_wq);
 out:
 	kfree(isert_np);
 
@@ -2466,6 +2489,8 @@ isert_free_np(struct iscsi_np *np)
 	}
 	mutex_unlock(&isert_np->mutex);
 
+	destroy_workqueue(isert_np->reinit_id_wq);
+
 	np->np_context = NULL;
 	kfree(isert_np);
 }
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
index ca8cfebe26ca..5fdc799f3ca8 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -209,4 +209,6 @@ struct isert_np {
 	struct mutex		mutex;
 	struct list_head	accepted;
 	struct list_head	pending;
+	struct work_struct      work;
+	struct workqueue_struct *reinit_id_wq;
 };
-- 
2.32.0

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Leon Romanovsky]

On Wed, Jul 14, 2021 at 09:26:46PM +0300, lanevdenoche@gmail.com wrote:
> Calling isert_setup_id() from isert_np_cma_handler() is wrong
> since at that time the socket address was still bound to the old cma_id
> which will be destroyed via rdma_destroy_id() only after processing
> the RDMA_CM_EVENT_ADDR_CHANGE event.
> 
>  - isert_np_cma_handler() calls isert_setup_id()
> 
>  - isert_setup_id() calls rdma_bind_addr()
> 
>  - rdma_bind_addr() returns -EADDRINUSE
> 
> Move the creation of the cma_id in workqueue context and delete old
> cma_id directly, not through returning the error code to the upper
> level.

Why do you need workqueue for that?

Is it possible to introduce new function isert_np_reinit_id() and call
it directly?

Thanks

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Leon Romanovsky]

On Sun, Jul 18, 2021 at 06:24:26PM +0000, Chesnokov Gleb wrote:
> Hi Leon,

Please don't use top-posting format. Thanks

> 
> 
> isert_cma_handler() is called from cma_work_handler(), which holds id_priv->handler_mutex.
> The call of rdma_destroy_id() from isert_cma_handler() will cause recursive acquisition of the mutex, leading to a deadlock.
> 
> -- cma_work_handler()
> ---- mutex_lock(&id_priv->handler_mutex)
> ---- isert_cma_handler()
> ------ rmda_destroy_id()
> -------- mutex_lock(&id_priv->handler_mutex)

This answers why you needed workqueue.

> 
> Therefore, you cannot directly call isert_np_reinit_id().

I would expect rdma-cm to be changed to support such flow.
However it is probably overkill.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Jason Gunthorpe]

On Wed, Jul 14, 2021 at 09:26:46PM +0300, lanevdenoche@gmail.com wrote:
> @@ -2466,6 +2489,8 @@ isert_free_np(struct iscsi_np *np)
>  	}
>  	mutex_unlock(&isert_np->mutex);
>  
> +	destroy_workqueue(isert_np->reinit_id_wq);
> +

This is racy, the lines above have:

	if (isert_np->cm_id)
		rdma_destroy_id(isert_np->cm_id);

And I don't see anything preventing that from running concurrently
with the WQ

What is this trying to do anyhow? If the addr has truely changed why
does the bind fail?

Jason

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Chesnokov Gleb]

> And I don't see anything preventing that from running concurrently
> with the WQ

Thanks, i made a fix.

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index de5ab2ae8e1738455bc36eca5487f1c9136e060e..6b0fc8d54f49102ad8c9fe67b1888dc6883f2164 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -601,14 +601,18 @@ static void isert_np_reinit_id_work(struct work_struct *w)
 {
 	struct isert_np *isert_np = container_of(w, struct isert_np, work);
 
-	rdma_destroy_id(isert_np->cm_id);
+	mutex_lock(&isert_np->id_mutex);
+	if (isert_np->cm_id) {
+		rdma_destroy_id(isert_np->cm_id);
 
-	isert_np->cm_id = isert_setup_id(isert_np);
-	if (IS_ERR(isert_np->cm_id)) {
-		isert_err("isert np %p setup id failed: %ld\n",
-			  isert_np, PTR_ERR(isert_np->cm_id));
-		isert_np->cm_id = NULL;
+		isert_np->cm_id = isert_setup_id(isert_np);
+		if (IS_ERR(isert_np->cm_id)) {
+			isert_err("isert np %p setup id failed: %ld\n",
+				  isert_np, PTR_ERR(isert_np->cm_id));
+			isert_np->cm_id = NULL;
+		}
 	}
+	mutex_unlock(&isert_np->id_mutex);
 }
 
 static int
@@ -2292,6 +2296,7 @@ isert_setup_np(struct iscsi_np *np,
 	}
 
 	INIT_WORK(&isert_np->work, isert_np_reinit_id_work);
+	mutex_init(&isert_np->id_mutex);
 
 	sema_init(&isert_np->sem, 0);
 	mutex_init(&isert_np->mutex);
@@ -2455,8 +2460,12 @@ isert_free_np(struct iscsi_np *np)
 	struct isert_np *isert_np = np->np_context;
 	struct isert_conn *isert_conn, *n;
 
-	if (isert_np->cm_id)
+	mutex_lock(&isert_np->id_mutex);
+	if (isert_np->cm_id) {
 		rdma_destroy_id(isert_np->cm_id);
+		isert_np->cm_id = NULL;
+	}
+	mutex_unlock(&isert_np->id_mutex);
 
 	/*
 	 * FIXME: At this point we don't have a good way to insure
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
index 5fdc799f3ca864667a454374f8548e7f031e9925..dafe7b44494d1d5687f0d87e18855d78e8729707 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -211,4 +211,5 @@ struct isert_np {
 	struct list_head	pending;
 	struct work_struct      work;
 	struct workqueue_struct *reinit_id_wq;
+	struct mutex		id_mutex;
 };

> What is this trying to do anyhow? If the addr has truely changed why
> does the bind fail?

When the active physical link member of bonding interface in active-standby 
mode gets faulty, the standby link will represent the assigned addresses on 
behalf of the active link.
Therefore, RDMA communication manager will notify iSER target with 
RDMA_CM_EVENT_ADDR_CHANGE.

The iSCSI socket address does not change.
But the cma_id at the IB layer, which is bound to the iSCSI socket, will change.
The problem is that the new cma_id is trying to bind to a socket address that is still bound to the old cma_id.
That is, before you bind a new cma_id to a socket, you must first delete the old one.

Best regards,
Chesnokov Gleb

From: Jason Gunthorpe <jgg@nvidia.com>
Sent: Monday, July 19, 2021 3:13:02 PM
To: lanevdenoche@gmail.com
Cc: linux-rdma@vger.kernel.org; sagi@grimberg.me; dledford@redhat.com; Chesnokov Gleb
Subject: Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE
    
On Wed, Jul 14, 2021 at 09:26:46PM +0300, lanevdenoche@gmail.com wrote:
> @@ -2466,6 +2489,8 @@ isert_free_np(struct iscsi_np *np)
>        }
>        mutex_unlock(&isert_np->mutex);
>  
> +     destroy_workqueue(isert_np->reinit_id_wq);
> +

This is racy, the lines above have:

        if (isert_np->cm_id)
                rdma_destroy_id(isert_np->cm_id);

And I don't see anything preventing that from running concurrently
with the WQ

What is this trying to do anyhow? If the addr has truely changed why
does the bind fail?

Jason
    

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Jason Gunthorpe]

On Mon, Jul 19, 2021 at 04:07:44PM +0000, Chesnokov Gleb wrote:

> The iSCSI socket address does not change.  But the cma_id at the IB
> layer, which is bound to the iSCSI socket, will change.  The problem
> is that the new cma_id is trying to bind to a socket address that is
> still bound to the old cma_id.  That is, before you bind a new
> cma_id to a socket, you must first delete the old one.

So why is iser trying to rebind a listening socket to the same
address? Isn't that the bug here? Just don't do that.

Jason

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Sagi Grimberg]

>> The iSCSI socket address does not change.  But the cma_id at the IB
>> layer, which is bound to the iSCSI socket, will change.  The problem
>> is that the new cma_id is trying to bind to a socket address that is
>> still bound to the old cma_id.  That is, before you bind a new
>> cma_id to a socket, you must first delete the old one.
> 
> So why is iser trying to rebind a listening socket to the same
> address? Isn't that the bug here? Just don't do that.

Hey Json,

Yes it is broken, IIRC it was meant to try and handle cases
where the address was restroed, but that needs to do is a periodic
attempt to repair (as bind is expected to fail). Similar to how
nvmet-rdma does this.

(I also have some vague memory that some bonding events mapped to this
cma event although the address was still available but that was a long
time ago so I'm not sure, and the change log says nothing about it...)

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Sagi Grimberg]

>> What is this trying to do anyhow? If the addr has truely changed why
>> does the bind fail?
> 
> When the active physical link member of bonding interface in active-standby
> mode gets faulty, the standby link will represent the assigned addresses on
> behalf of the active link.
> Therefore, RDMA communication manager will notify iSER target with
> RDMA_CM_EVENT_ADDR_CHANGE.

Ah, here is my recollection...

However I think that if we move that into a work, we should do it
periodically until we succeed or until the endpoint is torn down so
we can handle address removed and restore use-cases.

See nvmet-rdma for an example of what I'm talking about.

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Chesnokov Gleb]

> (I also have some vague memory that some bonding events mapped to this
> cma event although the address was still available but that was a long
> time ago so I'm not sure, and the change log says nothing about it...)

The test case where I reproduced the problem with processing RDMA_CM_EVENT_ADDR_CHANGE 
is the disconnection of the active link from bonding under load (through pull out the cable).

With my patch, the bug is not reproduced, the load is not interrupted.

Best regards,
Chesnokov Gleb

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Jason Gunthorpe]

On Mon, Jul 19, 2021 at 11:29:40AM -0700, Sagi Grimberg wrote:
> 
> > > What is this trying to do anyhow? If the addr has truely changed why
> > > does the bind fail?
> > 
> > When the active physical link member of bonding interface in active-standby
> > mode gets faulty, the standby link will represent the assigned addresses on
> > behalf of the active link.
> > Therefore, RDMA communication manager will notify iSER target with
> > RDMA_CM_EVENT_ADDR_CHANGE.
> 
> Ah, here is my recollection...
> 
> However I think that if we move that into a work, we should do it
> periodically until we succeed or until the endpoint is torn down so
> we can handle address removed and restore use-cases.

That soudns better, but still I would say this shouldn't even happen
in the first place, check the address and don't initiate rebinding if
it hasn't changed.

Jason

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Sagi Grimberg]

>>>> What is this trying to do anyhow? If the addr has truely changed why
>>>> does the bind fail?
>>>
>>> When the active physical link member of bonding interface in active-standby
>>> mode gets faulty, the standby link will represent the assigned addresses on
>>> behalf of the active link.
>>> Therefore, RDMA communication manager will notify iSER target with
>>> RDMA_CM_EVENT_ADDR_CHANGE.
>>
>> Ah, here is my recollection...
>>
>> However I think that if we move that into a work, we should do it
>> periodically until we succeed or until the endpoint is torn down so
>> we can handle address removed and restore use-cases.
> 
> That soudns better, but still I would say this shouldn't even happen
> in the first place, check the address and don't initiate rebinding if
> it hasn't changed.

But don't you need to setup a new cm_id for the this notification? It
will remain active?

Also it's a bit cumbersome to match addresses in some cases like multi
address interfaces. Almost seems easier to setup a new one...

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Jason Gunthorpe]

On Thu, Jul 22, 2021 at 12:54:26PM -0700, Sagi Grimberg wrote:
> 
> > > > > What is this trying to do anyhow? If the addr has truely changed why
> > > > > does the bind fail?
> > > > 
> > > > When the active physical link member of bonding interface in active-standby
> > > > mode gets faulty, the standby link will represent the assigned addresses on
> > > > behalf of the active link.
> > > > Therefore, RDMA communication manager will notify iSER target with
> > > > RDMA_CM_EVENT_ADDR_CHANGE.
> > > 
> > > Ah, here is my recollection...
> > > 
> > > However I think that if we move that into a work, we should do it
> > > periodically until we succeed or until the endpoint is torn down so
> > > we can handle address removed and restore use-cases.
> > 
> > That soudns better, but still I would say this shouldn't even happen
> > in the first place, check the address and don't initiate rebinding if
> > it hasn't changed.
> 
> But don't you need to setup a new cm_id for the this notification? It
> will remain active?

AFAIK the existing listening ID remains, the notification is
informative, it doesn't indicate any CM state has changed.
 
> Also it's a bit cumbersome to match addresses in some cases like multi
> address interfaces. Almost seems easier to setup a new one...

How so? There is only one address passed to bind. If you create
multiple CM ids to cover all addresses then you need to run a set
algorithm to figure out what cm_ids to destroy and which to
create.

Jason

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Sagi Grimberg]

>>>>>> What is this trying to do anyhow? If the addr has truely changed why
>>>>>> does the bind fail?
>>>>>
>>>>> When the active physical link member of bonding interface in active-standby
>>>>> mode gets faulty, the standby link will represent the assigned addresses on
>>>>> behalf of the active link.
>>>>> Therefore, RDMA communication manager will notify iSER target with
>>>>> RDMA_CM_EVENT_ADDR_CHANGE.
>>>>
>>>> Ah, here is my recollection...
>>>>
>>>> However I think that if we move that into a work, we should do it
>>>> periodically until we succeed or until the endpoint is torn down so
>>>> we can handle address removed and restore use-cases.
>>>
>>> That soudns better, but still I would say this shouldn't even happen
>>> in the first place, check the address and don't initiate rebinding if
>>> it hasn't changed.
>>
>> But don't you need to setup a new cm_id for the this notification? It
>> will remain active?
> 
> AFAIK the existing listening ID remains, the notification is
> informative, it doesn't indicate any CM state has changed.

Gleb, can you confirm that?

>> Also it's a bit cumbersome to match addresses in some cases like multi
>> address interfaces. Almost seems easier to setup a new one...
> 
> How so? There is only one address passed to bind. If you create
> multiple CM ids to cover all addresses then you need to run a set
> algorithm to figure out what cm_ids to destroy and which to
> create.

There is one address passed to bind, but I need to check that this
address belongs to the interface, which is what bind does anyways..

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Chesnokov Gleb]

>> AFAIK the existing listening ID remains, the notification is
>> informative, it doesn't indicate any CM state has changed.
>
> Gleb, can you confirm that?

In my case, when 2 physical interfaces are bonded and the cable is pulled out for one of them,
the RDMA_CM_EVENT_ADDR_CHANGE event is processed 2 times.

1 for condition isert_np->cm_id == cma_id
        isert_np_cma_handler() is called
        The old cma_id is destroyed and a new one is created
2 for condition isert_np->cm_id != cma_id
        isert_disconnected_handler() is called

As I understand it in this case, RDMA_CM_EVENT_ADDR_CHANGE is not just an informative event.
It is needed to recreate the isert connection (struct isert_conn) for the standby path.
I may be wrong, but the creation of a 'struct isert_conn' is initiated in isert_rdma_accept(),
that is, when the cma_id is created.

Therefore, it seems to me that you need to recreate cma_id,
otherwise who will recreate isert_conn?

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Sagi Grimberg]

>>> AFAIK the existing listening ID remains, the notification is
>>> informative, it doesn't indicate any CM state has changed.
>>
>> Gleb, can you confirm that?
> 
> In my case, when 2 physical interfaces are bonded and the cable is pulled out for one of them,
> the RDMA_CM_EVENT_ADDR_CHANGE event is processed 2 times.
> 
> 1 for condition isert_np->cm_id == cma_id
>          isert_np_cma_handler() is called
>          The old cma_id is destroyed and a new one is created
> 2 for condition isert_np->cm_id != cma_id
>          isert_disconnected_handler() is called
> 
> As I understand it in this case, RDMA_CM_EVENT_ADDR_CHANGE is not just an informative event.
> It is needed to recreate the isert connection (struct isert_conn) for the standby path.
> I may be wrong, but the creation of a 'struct isert_conn' is initiated in isert_rdma_accept(),
> that is, when the cma_id is created.
> 
> Therefore, it seems to me that you need to recreate cma_id,
> otherwise who will recreate isert_conn?

There are two handlers in question here, the listener cm_id and
the connection cm_id. The connection cma_id should definitely trigger
disconnect and resource cleanup. The question is should the listener
cma_id (which maps to the isert network portal - np) recreate the
cma_id in this event.

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Chesnokov Gleb]

> There are two handlers in question here, the listener cm_id and
> the connection cm_id. The connection cma_id should definitely trigger
> disconnect and resource cleanup. The question is should the listener
> cma_id (which maps to the isert network portal - np) recreate the
> cma_id in this event.

1) How connection cm_id and isert_conn are created:
[cma.c]     - cma_ib_req_handler()
                 - * Create conn_id via cma_ib_new_conn_id(listen_id)
                 - * Calls isert_connect_request(conn_id)
[ib_isert.c] - - isert_connect_request(cma_id)
                 - - * isert_conn = kzalloc()
                 - - * isert_conn->cm_id = cma_id

2) Acceptance of the connection request starts after creating the listener cm_id:
[ib_isert.c] - isert_setup_id()
[ib_isert.c] - - rdma_listen()
[cma.c]      - - - cma_ib_listen()
[cma.c]      - - - - ib_cm_insert_listen(cma_ib_req_handler)


3) Processing RDMA_CM_EVENT_ADDR_CHANGE for connection cm_id:
[ib_isert.c] - rdma_disconnect(cm_id)
[ib_isert.c] - rdma_destroy_id(cm_id)
[ib_isert.c] - kfree(iser_conn)

4) Processing RDMA_CM_EVENT_ADDR_CHANGE for listener cm_id:

Since iser_conn has been freed, it needs to be recreated.

There are 2 options here:
a) listener cm_id needs to be recreated, it calls rdma_listen(),
which in turn initiates the acceptance of the connection request,
after which iser_conn will be created.

b) listener cm_id does not need to be recreated,
that is, RDMA_CM_EVENT_ADDR_CHANGE is informative for it.

I have tested my test case with the following patch that matches point b:

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 636d590765f9578c0ff595cdf74b79400bfa66ed..54f615828961576ffa1f74c8b9781a5cf48512a3 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -601,6 +601,8 @@ static int
 isert_np_cma_handler(struct isert_np *isert_np,
 		     enum rdma_cm_event_type event)
 {
+	int ret = -1;
+
 	isert_dbg("%s (%d): isert np %p\n",
 		  rdma_event_msg(event), event, isert_np);
 
@@ -609,19 +611,14 @@ isert_np_cma_handler(struct isert_np *isert_np,
 		isert_np->cm_id = NULL;
 		break;
 	case RDMA_CM_EVENT_ADDR_CHANGE:
-		isert_np->cm_id = isert_setup_id(isert_np);
-		if (IS_ERR(isert_np->cm_id)) {
-			isert_err("isert np %p setup id failed: %ld\n",
-				  isert_np, PTR_ERR(isert_np->cm_id));
-			isert_np->cm_id = NULL;
-		}
+		ret = 0;
 		break;
 	default:
 		isert_err("isert np %p Unexpected event %d\n",
 			  isert_np, event);
 	}
 
-	return -1;
+	return ret;
 }
 
 static int

As a result, the listener cm_id remained, the connection request did not come, isert_conn was not recreated.
On the initiator, the load dropped to 0 and then ended.


With my patch that matches point a, the listener cm_id was recreated -> connection request was received -> isert_conn was created.

Based on the above, I can conclude that the RDMA_CM_EVENT_ADDR_CHANGE event is not informative for the listener cm_id.

Best regards,
Chesnokov Gleb

Re: [PATCH 1/1] iser-target: Fix handling of RDMA_CV_EVENT_ADDR_CHANGE

[Jason Gunthorpe]

On Wed, Sep 01, 2021 at 11:43:34AM +0000, Chesnokov Gleb wrote:

> b) listener cm_id does not need to be recreated,
> that is, RDMA_CM_EVENT_ADDR_CHANGE is informative for it.

I depends on what is in sa = (struct sockaddr *)&np->np_sockaddr;

If that is not a wildcard then the listen is only good so long as that
address the one assigned to the interface.

I'm not sure why isert would use a non-wildcard listen?

In any event if the isert_setup_np() was called with a specific
address:

static int
isert_setup_np(struct iscsi_np *np,
	       struct sockaddr_storage *ksockaddr)

Then the np should operate on that address, and only that address, and
not quitely change to some other address during operation. So again I
think the original proposal is not right - though I don't know enough
about iscsi insides to suggest what is wrong here.

Jason