Re: [PATCH 04/12] IB/srp: Fix connection state tracking

[Doug Ledford <dledford@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4093 bytes --]

On Thu, 2015-04-30 at 10:58 +0200, Bart Van Assche wrote:
> Reception of a DREQ message only causes the state of a single
> channel to change. Modify the SRP initiator such that channel
> and target connection state are tracked separately. This patch
> avoids that following false positive warning can be reported
> by srp_destroy_qp():
> 
> WARNING: at drivers/infiniband/ulp/srp/ib_srp.c:617 srp_destroy_qp+0xa6/0x120 [ib_srp]()
> Call Trace:
> [<ffffffff8106e10f>] warn_slowpath_common+0x7f/0xc0
> [<ffffffff8106e16a>] warn_slowpath_null+0x1a/0x20
> [<ffffffffa0440226>] srp_destroy_qp+0xa6/0x120 [ib_srp]
> [<ffffffffa0440322>] srp_free_ch_ib+0x82/0x1e0 [ib_srp]
> [<ffffffffa044408b>] srp_create_target+0x7ab/0x998 [ib_srp]
> [<ffffffff81346f60>] dev_attr_store+0x20/0x30
> [<ffffffff811dd90f>] sysfs_write_file+0xef/0x170
> [<ffffffff8116d248>] vfs_write+0xc8/0x190
> [<ffffffff8116d411>] sys_write+0x51/0x90
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Sebastian Parschauer <sebastian.riemer@profitbricks.com>
> Cc: <stable@vger.kernel.org> #v3.19
> ---
>  drivers/infiniband/ulp/srp/ib_srp.c | 7 ++++---
>  drivers/infiniband/ulp/srp/ib_srp.h | 1 +
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 5ce6cfd..0eb07d3 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -465,14 +465,13 @@ static struct srp_fr_pool *srp_alloc_fr_pool(struct srp_target_port *target)
>   */
>  static void srp_destroy_qp(struct srp_rdma_ch *ch)
>  {
> -	struct srp_target_port *target = ch->target;
>  	static struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
>  	static struct ib_recv_wr wr = { .wr_id = SRP_LAST_WR_ID };
>  	struct ib_recv_wr *bad_wr;
>  	int ret;
>  
>  	/* Destroying a QP and reusing ch->done is only safe if not connected */
> -	WARN_ON_ONCE(target->connected);
> +	WARN_ON_ONCE(ch->connected);
>  
>  	ret = ib_modify_qp(ch->qp, &attr, IB_QP_STATE);
>  	WARN_ONCE(ret, "ib_cm_init_qp_attr() returned %d\n", ret);
> @@ -836,6 +835,7 @@ static void srp_disconnect_target(struct srp_target_port *target)
>  
>  		for (i = 0; i < target->ch_count; i++) {
>  			ch = &target->ch[i];
> +			ch->connected = false;
>  			if (ch->cm_id && ib_send_cm_dreq(ch->cm_id, NULL, 0)) {
>  				shost_printk(KERN_DEBUG, target->scsi_host,
>  					     PFX "Sending CM DREQ failed\n");
> @@ -1017,6 +1017,7 @@ static int srp_connect_ch(struct srp_rdma_ch *ch, bool multich)
>  		switch (ch->status) {
>  		case 0:
>  			srp_change_conn_state(target, true);
> +			ch->connected = true;
>  			return 0;
>  
>  		case SRP_PORT_REDIRECT:
> @@ -2367,7 +2368,7 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
>  	case IB_CM_DREQ_RECEIVED:
>  		shost_printk(KERN_WARNING, target->scsi_host,
>  			     PFX "DREQ received - connection closed\n");
> -		srp_change_conn_state(target, false);
> +		ch->connected = false;

So, in this patch, you modify disconnect to set srp_change_conn_state()
to false for the target, then loop through all the channels sending
cm_dreq's, and on the receiving side, you modify the cm_dreq handler to
set each channel to false.  However, once you get to 0 channels open,
shouldn't you then set the target state to false too just to keep things
consistent?

>  		if (ib_send_cm_drep(cm_id, NULL, 0))
>  			shost_printk(KERN_ERR, target->scsi_host,
>  				     PFX "Sending CM DREP failed\n");
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
> index a611556..95a4471 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.h
> +++ b/drivers/infiniband/ulp/srp/ib_srp.h
> @@ -170,6 +170,7 @@ struct srp_rdma_ch {
>  
>  	struct completion	tsk_mgmt_done;
>  	u8			tsk_mgmt_status;
> +	bool			connected;
>  };
>  
>  /**


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]