Mike Christie wrote:
>> - the kfifo_put call is safe since there's one consumer (xmit flow under
>> lock for passthrough, and xmit worker for non passthrough) and one
>> producer (response flow has single tasklet instance).
> I do do not know what you mean that there is one consumer. There is a
> kfifo_get in the passthrough path (__iscsi_conn_send_pdu) and in the non
> passthrough path (iscsi_queuecommand). We could have 4 threads/contects
> calling kfifo_get at the same time: [...]
agree to all of 1-4, sorry, I wasn't clear enough, I wanted to say that all the
calls
to kfifo_get are under the session lock and hence I am considering them as one
consumer.
> There is normally one __kfifo_put in the completion path. However, there
> are __kfifo_puts all over the place when you take into consideration the
> error paths. We can do __kfifo_put in iscsi_queuecommand, in the xmit_thread
> in
> iscsi_data_xmit, and in __iscsi_conn_send_pdu.
oh boy, you are right, doing lockless response path isn't simple as I thought...
> If you did not have a get() on the task and are running
> iser_task_rdma_finalize while some other code is doing the final put
> (could be due to some sort of cleanup like from a abort or lun/target
> reset) then the task could be reallocated while iser_task_rdma_finalize
> is still accessing it. This should not happen though, because the only
> case it would happen on is where are sending tmfs and we get success
> responses, and then the target continues to send data/responses for the task.
okay, lets do that one at a time, that is before looking on turning the giant
session
lock to bunch of locks/atomics as you suggest below, I'd like to make the iser
recv
completion path take the session lock once and not twice, e.g as done in the
patch below,
> One question on this. Maybe there is one other place it could happen. If
> after ep_disconnect has completed can iser_rcv_completion be running or get
> called?
no, iser_ep_disconnect does a wait call, and the wakeup logic will not trigger
before
(A && B) take place, where A stands for all the posted recv/send buffers are
completed/flushed
from the QP (see iser_handle_comp_error) and B stands for a disconnect event
was delivered
by the RDMA-CM (see iser_disconnected_handler).
> If so then libiscsi could be cleaning up tasks
> (scsi_fail_tasks) for erl0 cleanup and then that could race with
> iser_rcv_completion. I think we (maybe Erez and I) discussed this on the
> list and we said it should not be possible, so maybe there is just the
> tmf case which should not occur and we can think of something that does
> not affect perf.
I wasn't sure to follow here, taking into account that after ep_disconnect
returns no
invocations of iscsi_complete_pdu through iser_rcv_completion are expected, is
the question
raised here still relevant?
[PATCH RFC] remove some of the locking in iser scsi command response flow
currently iser recv completion flow takes the session lock twice.
optimize it to avoid the first one by letting iser_task_rdma_finalize()
be called only from the cleanup_task callback invoked by iscsi_free_task,
thus reducing the contention on the session lock between the scsi
command submission to the scsi command completion flows.
Signed-off-by: Or Gerlitz <ogerl...@voltaire.com>
-----
Index: linux-2.6/drivers/infiniband/ulp/iser/iser_initiator.c
===================================================================
--- linux-2.6.orig/drivers/infiniband/ulp/iser/iser_initiator.c
+++ linux-2.6/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -571,8 +571,6 @@ void iser_rcv_completion(struct iser_des
{
struct iser_dto *dto = &rx_desc->dto;
struct iscsi_iser_conn *conn = dto->ib_conn->iser_conn;
- struct iscsi_task *task;
- struct iscsi_iser_task *iser_task;
struct iscsi_hdr *hdr;
char *rx_data = NULL;
int rx_data_len = 0;
@@ -590,25 +588,6 @@ void iser_rcv_completion(struct iser_des
opcode = hdr->opcode & ISCSI_OPCODE_MASK;
- if (opcode == ISCSI_OP_SCSI_CMD_RSP) {
- spin_lock(&conn->iscsi_conn->session->lock);
- task = iscsi_itt_to_ctask(conn->iscsi_conn, hdr->itt);
- if (task)
- __iscsi_get_task(task);
- spin_unlock(&conn->iscsi_conn->session->lock);
-
- if (!task)
- iser_err("itt can't be matched to task!!! "
- "conn %p opcode %d itt %d\n",
- conn->iscsi_conn, opcode, hdr->itt);
- else {
- iser_task = task->dd_data;
- iser_dbg("itt %d task %p\n",hdr->itt, task);
- iser_task->status = ISER_TASK_STATUS_COMPLETED;
- iser_task_rdma_finalize(iser_task);
- iscsi_put_task(task);
- }
- }
iser_dto_buffs_release(dto);
iscsi_iser_recv(conn->iscsi_conn, hdr, rx_data, rx_data_len);
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To post to this group, send email to open-is...@googlegroups.com.
To unsubscribe from this group, send email to
open-iscsi+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/open-iscsi?hl=en.
