ib_srpt: An IB SRP target

Note: this patch is also available in branch srpt of this git
repository: https://github.com/bvanassche/srpt

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Bart,

Thank you for your contibution. I will plan to spend time in the next days reviewing the TCM interaction pieces, but have gone ahead and pushed as-is into a working branch @ lio-core-2.6.git/tcm_ib_srpt (.38-rc4). Please send along any further code updates, and I will make sure they hit this branch.

Also just a FYI, in order to compile ib_srpt against the latest LIO upstream tree, the original srp_tmr_to_tcm() code was modified with the following patch to use the new TMR_* prefixed definitions from include/target/target_core_tmr.h.

Thanks!

--nab

index e63775b..cfcb1bc 100644
@@ -1860,15 +1860,15 @@ static int srp_tmr_to_tcm(int fn)
 {
        switch (fn) {
        case SRP_TSK_ABORT_TASK:
-               return ABORT_TASK;
+               return TMR_ABORT_TASK;
        case SRP_TSK_ABORT_TASK_SET:
-               return ABORT_TASK_SET;
+               return TMR_ABORT_TASK_SET;
        case SRP_TSK_CLEAR_TASK_SET:
-               return CLEAR_TASK_SET;
+               return TMR_CLEAR_TASK_SET;
        case SRP_TSK_LUN_RESET:
-               return LUN_RESET;
+               return TMR_LUN_RESET;
        case SRP_TSK_CLEAR_ACA:
-               return CLEAR_ACA;
+               return TMR_CLEAR_ACA;
        default:
                return -1;
        }

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Since no documentation is available for the target core, the unavoidable
happened: I had misinterpret the meaning of one target core function. I just found out that queue_data_in must operate synchronously. So this follow-up patch is necessary in order to make this IB SRP target implementation work correctly:

b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 7fe9d3b..b92c0e3 100644

@@ -1320,6 +1320,7 @@ static struct srpt_send_ioctx

*srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
ioctx->n_rdma_ius = 0;
ioctx->rdma_ius = NULL;
ioctx->mapped_sg_count = 0;
+ init_completion(&ioctx->tx_done);
/*
* transport_init_se_cmd() does not initialize all fields, so do it
* here.
@@ -1429,6 +1430,7 @@ static int srpt_abort_cmd(struct srpt_send_ioctx *ioctx)
* not been received in time.
*/
srpt_unmap_sg_to_ib_sge(ioctx->ch, ioctx);
+ complete(&ioctx->tx_done);
atomic_set(&ioctx->cmd.t_task->transport_lun_stop, 1); kref_put(&ioctx->kref, srpt_put_send_ioctx_kref); break;
@@ -1492,6 +1494,9 @@ static void srpt_handle_send_comp(struct srpt_rdma_ch *ch,
&& state != SRPT_STATE_DONE))
pr_debug("state = %d\n", state);

+ if (state == SRPT_STATE_CMD_RSP_SENT)
+ complete(&ioctx->tx_done);
+
if (state != SRPT_STATE_DONE)
kref_put(&ioctx->kref, srpt_put_send_ioctx_kref);
else
@@ -3097,7 +3102,9 @@ static int srpt_queue_data_in(struct se_cmd *cmd)

resp_len = srpt_build_cmd_rsp(ch, ioctx, ioctx->tag, cmd->scsi_status); ret = srpt_post_send(ch, ioctx, resp_len);
- if (ret) {
+ if (ret == 0)
+ wait_for_completion(&ioctx->tx_done);
+ else {
printk(KERN_ERR "sending cmd response failed for tag %llu\n",
ioctx->tag);
srpt_unmap_sg_to_ib_sge(ch, ioctx);

b/drivers/infiniband/ulp/srpt/ib_srpt.h
index 321b097..54d2382 100644

@@ -218,6 +218,7 @@ struct srpt_send_ioctx {
 	spinlock_t		spinlock;
 	enum srpt_command_state	state;
 	struct se_cmd		cmd;
+	struct completion	tx_done;
 	u64			tag;
 	int			sg_cnt;
 	int			mapped_sg_count;

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Greetings Bart,

Using a completion queue here with possibility of the extra context switch is IMHO an unnecessary overhead. The main issue here is that target core needs to be able to clear the struct se_cmd descriptor after invoking the struct target_core_fabric_ops->send_data_in() callback, but before the call to srpt_handle_send_comp() -> release of the fabric dependent descriptor containing our struct se_cmd can happen.

I prefer something along the lines of the patch below for lio-core-2.6.git/tcm_ib_srpt to follow what other TCM v4 HW fabric modules are currently doing wrt to TFO->check_stop_free() usage. This patch has been compile tested only at this point, but I will be testing this on IB hardware in the upcoming week. Please review.

Thanks,

--nab

index cfcb1bc..cf42db8 100644
@@ -1326,6 +1326,8 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
         */
        memset(&ioctx->cmd, 0, sizeof(ioctx->cmd));
        memset(&ioctx->sense_data, 0, sizeof(ioctx->sense_data));
+       atomic_set(&ioctx->cmd_done, 0);
+       atomic_set(&ioctx->cmd_stop_free, 0);
 
        return ioctx;
 }

@@ -1486,6 +1488,15 @@ static void srpt_handle_send_comp(struct srpt_rdma_ch *ch,
        atomic_inc(&ch->sq_wr_avail);
 
        state = srpt_set_cmd_state(ioctx, SRPT_STATE_DONE);
+       /*
+        * If srpt_check_stop_free() has already been called, we
+        * are safe to go ahead and call transport_generic_free_cmd()
+        * to release the descriptor.
+        */
+       atomic_set(&ioctx->cmd_done, 1);
+
+       if (atomic_read(&ioctx->cmd_stop_free) != 0)
+               transport_generic_free_cmd(&ioctx->cmd, 0, 1, 0);
 
        if (WARN_ON(state != SRPT_STATE_CMD_RSP_SENT
                    && state != SRPT_STATE_MGMT_RSP_SENT

@@ -1737,13 +1748,26 @@ out_err:
 
 static void srpt_check_stop_free(struct se_cmd *cmd)
 {
-       struct srpt_send_ioctx *ioctx;
+       struct srpt_send_ioctx *ioctx = container_of(cmd,
+                               struct srpt_send_ioctx, cmd);
+
+       if (cmd->se_tmr_req)
+               return;
 
-       ioctx = container_of(cmd, struct srpt_send_ioctx, cmd);
-       transport_generic_free_cmd(cmd, 0, 1, 0);
        if (srpt_get_cmd_state(ioctx) != SRPT_STATE_MGMT_RSP_SENT)
                srpt_unmap_sg_to_ib_sge(ioctx->ch, ioctx);
-       kref_put(&ioctx->kref, srpt_put_send_ioctx_kref);
+
+       /*
+        * If srpt_handle_send_comp() has already been called from the LLD,
+        * it's safe to call transport_generic_free_cmd() to release the
+        * descriptor.  Otherwise set cmd->cmd_stop_free=1 and let
+        * srpt_handle_send_comp() call transport_generic_free_cmd().
+        */
+       if (atomic_read(&ioctx->cmd_done) != 0) {
+               transport_generic_free_cmd(&ioctx->cmd, 0, 1, 0);
+               kref_put(&ioctx->kref, srpt_put_send_ioctx_kref);
+       } else
+               atomic_set(&ioctx->cmd_stop_free, 1);
 }
 
 /**

index 321b097..02be818 100644
@@ -218,6 +218,8 @@ struct srpt_send_ioctx {
        spinlock_t              spinlock;
        enum srpt_command_state state;
        struct se_cmd           cmd;
+       atomic_t                cmd_done;
+       atomic_t                cmd_stop_free;
        u64                     tag;
        int                     sg_cnt;
        int                     mapped_sg_count;

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

What's the point of using atomic_t here? AFAICT, you only use atomic_set and atomic_read with them, which have no special ordering powers (look at what those macros expand to).

- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Ugh, thanks for pointing this out. For atomic_t inc/dec I always use smp_mb__after_atomic_[inc,dec](), but I forgot that atomic_set() is a simple/dumb assignment wrapper that also expects an explict barrier() to enforce CONFIG_SMP ordering..

So adding the extra explict barrier here is currently required to address the potential race between TFO->check_stop_free() usage after TFO->queue_data_in(), and the explict ioctx and ioctl->cmd (struct se_cmd) release from IB completion queue context. However, I am starting to wonder if using TFO->check_stop_free() here with HW target perhaps does not make the most sense..

The ->check_stop_free() target fabric callback was originally used for the TCM_Loop SCSI LLD module in order to immmediate complete and release a struct scsi_cmnd after TFO->queue_data_in() has been called. In the case of using IB completion queues, I think checking for a zero of T_TASK(cmd)->t_transport_active before release the ioctx->cmd might may more sense.. To give more of an idea of how this currently works, the following in target_core_transport.c:transport_cmd_check_stop() is called from TCM backend struct se_device thread context after TFO->queue_data_in() has been called:

   spin_lock_irqsave(&T_TASK(cmd)->t_state_lock, flags);
   .....
        if (transport_off) {
                atomic_set(&T_TASK(cmd)->t_transport_active, 0);
                if (transport_off == 2) {
                        transport_all_task_dev_remove_state(cmd);
                        /*
                         * Clear struct se_cmd->se_lun before the transport_off == 2
                         * handoff to fabric module.
                         */
                        cmd->se_lun = NULL;
                        /*
                         * Some fabric modules like tcm_loop can release
                         * their internally allocated I/O refrence now and
                         * struct se_cmd now.
                         */
                        if (CMD_TFO(cmd)->check_stop_free != NULL) {
                                spin_unlock_irqrestore(
                                        &T_TASK(cmd)->t_state_lock, flags);

                                CMD_TFO(cmd)->check_stop_free(cmd);
                                return 1;
                        }
                }
                spin_unlock_irqrestore(&T_TASK(cmd)->t_state_lock, flags);
		return 0;
        }

.....
spin_unlock_irqrestore(&T_TASK(cmd)->t_state_lock, flags);

So given this logic, I think waiting for T_TASK(cmd)->t_transport_active to return to zero before releasing ioctx->cmd in the IB completion path might be a better way handle this.. However, to do this properly w/o holding T_TASK(cmd)->t_state_lock in the completion path is going to require a small re-org of the above from transport_cmd_check_stop() for the transport_off == 2 case that TFO->queue_data_in() is invoking..

Thoughts Roland..?

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hello Nicholas,

Thanks for the review. Regarding the two issues you mentioned:
- As far as I understood it is safe to send back data to the initiator concurrently with the target core processing new commands ? In that case no new counters are necessary - the existing kref in srpt_send_ioctx is sufficient. The patch that re-enables concurrent SCSI READs can be found here:
https://github.com/bvanassche/srpt/commit/cd8d5e9a6501f5b3a4315c261e945c201c4ce0ae.
Please let me know if anyone wants to see that patch posted on the list. Also, please note that your patch contained the same race as my original patch: directly invoking srpt_unmap_sg_to_ib_sge() from the check_stop_free() callback makes that call race with the ongoing RDMA write. To Roland: I haven't found a way to invoke srpt_put_send_ioctx() without using an atomic counter. If you see an approach to avoid that atomic counter, suggestions are welcome.
- Regarding making the check_stop_free() callback a NOOP for task management commands: are you sure that that's a good idea ? I have tried to do that, and the following appeared in the kernel log during module unload:

CORE_HBA[0] - Detached HBA from Generic Target Core
=============================================================================
BUG se_tmr_cache: Objects remaining on kmem_cache_close()
-----------------------------------------------------------------------------

INFO: Slab 0xffffea0000119c00 objects=16 used=1 fp=0xffff880005080100 flags=0x1000000000000080
Pid: 18117, comm: rmmod Not tainted 2.6.38-rc5-dbg+ #4

Call Trace:
 [<ffffffff81123f76>] ? slab_err+0x96/0xb0
 [<ffffffff81126da4>] ? __slab_alloc+0x124/0x540
 [<ffffffff811277c8>] ? __kmalloc+0xf8/0x1b0
 [<ffffffff81081a0d>] ? trace_hardirqs_off+0xd/0x10
 [<ffffffff811277e8>] ? __kmalloc+0x118/0x1b0
 [<ffffffff81128cf7>] ? kmem_cache_destroy+0x177/0x3f0
 [<ffffffffa02e53c6>] ? release_se_global+0x36/0xb0 [target_core_mod]
 [<ffffffffa02d10ab>] ? target_core_exit_configfs+0x17b/0x190 [target_core_mod]
 [<ffffffff8109340f>] ? sys_delete_module+0x16f/0x260
 [<ffffffff81002fdc>] ? sysret_check+0x27/0x62
 [<ffffffff81087a8d>] ? trace_hardirqs_on_caller+0x14d/0x190
 [<ffffffff81002fab>] ? system_call_fastpath+0x16/0x1b

INFO: Object 0xffff880005080000 @offset=0
INFO: Allocated in core_tmr_alloc_req+0x33/0x80 [target_core_mod] age=2442 cpu=1 pid=18088
SLUB se_tmr_cache: kmem_cache_destroy called for cache that still has objects.
Pid: 18117, comm: rmmod Not tainted 2.6.38-rc5-dbg+ #4

Call Trace:
 [<ffffffff81128ee4>] ? kmem_cache_destroy+0x364/0x3f0
 [<ffffffffa02e53c6>] ? release_se_global+0x36/0xb0 [target_core_mod]
 [<ffffffffa02d10ab>] ? target_core_exit_configfs+0x17b/0x190 [target_core_mod]
 [<ffffffff8109340f>] ? sys_delete_module+0x16f/0x260
 [<ffffffff81002fdc>] ? sysret_check+0x27/0x62
 [<ffffffff81087a8d>] ? trace_hardirqs_on_caller+0x14d/0x190
 [<ffffffff81002fab>] ? system_call_fastpath+0x16/0x1b

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Yes, > 1 TCQ depth is not the issue. This issue at hand is how to clear the struct se_cmd in transport_cmd_check_stop() when using the optional TFO->check_stop_free() call after TFO->queue_data_in() has been invoked.

Ok, so this sounds like srpt_unmap_sg_to_ib_sge() should be getting called from the IB completion queue context then..

Hmmm, I don't recall seeing this with TCM_Loop with sg_reset, but I will double check with the lastest for-39 WIP fabric module code using TFO->check_stop_free().

Thanks!

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html