[Stgt-devel] iSER

[FUJITA Tomonori]

On Thu, 11 Oct 2007 14:22:53 -0400
Pete Wyckoff <pw at osc.edu> wrote:

> fujita.tomonori at lab.ntt.co.jp wrote on Thu, 11 Oct 2007 14:57 +0900:
> > Pete, the iSER patchset is ready for re-submission?
> 
> I think so.  There are no major outstanding issues that I know about
> now.  Let me know if you want to see another round of emails on
> this.  Or we can just iterate on your questions as you view the git.

Can you please submit an updated patchset to the mailing list?

It's easier to review and comment on it. And there might be other
people who are interested in them.


> > BTW, can you elaborate on the following commit?
> > 
> > http://git.osc.edu/?p=tgt.git;a=commit;h=8d9eae7acd041fc10a7cfe560c1c280dcc290fa1
> > 
> > What type of commands hit this bug?
> 
> First, the original commit referred to in that log fixes an issue
> where the target would spin with nothing to do.  A request comes in,
> gets handed off to bs_sync, but the conn->fd was still EPOLLIN |
> EPOLLOUT.  tgtd main thread would keep trying to send something,
> looping.  But now, looking at that old tree, I can't convince myself
> how this was ever a problem.  The only bad thing that happens
> without this old patch, now, is one extra trip through
> iscsi_task_tx_start() which always prints "no more data", but that
> doesn't seem like a huge deal.  Maybe I'm missing something today,
> or was missing something back then.  Or fixed some real bug in the
> interim.
> 
> The new commit you ask about essentially reverts the original
> commit.  The problem I was seeing is that with multiple commands
> outstanding, turning off EPOLLOUT on conn->fd would prohibit any
> task responses from being sent on the connection.  That epoll
> setting is for the entire connection, not for the individual task
> that is just beginning execution.  So you could have 10 tasks on
> task->c_list, waiting to send responses back to the initiator.  Then
> one new request is received and is submitted via
> iscsi_scsi_cmd_execute().  This disables EPOLLOUT, and the waiting
> 10 tasks do not get sent until some other task happens to complete
> in bs_sync and re-enables EPOLLOUT polling on the connection fd.

Thanks. I see the point. Surely we should remove the line:

tgt_event_modify(conn->fd, EPOLLIN);


> The net effect of the two patches is zero.  Just some minor code
> path rewrite with no logic change.  Sorry for the churn.

There is one logic change. The original iscsi_scsi_cmd_execute()
always calls:

static int iscsi_scsi_cmd_execute(struct iscsi_task *task)
{
	struct iscsi_connection *conn = task->conn;
	struct iscsi_cmd *req = (struct iscsi_cmd *) &task->req;
	uint8_t rw = req->flags & ISCSI_FLAG_CMD_WRITE;
	int err = 0;

	if (rw && task->r2t_count) {
		if (!task->unsol_count)
			list_add_tail(&task->c_list, &task->conn->tx_clist);
		goto no_queuing;
	}

	task->offset = 0;  /* for use as transmit pointer for data-ins */
	err = iscsi_target_cmd_queue(task);
no_queuing:
	tgt_event_modify(conn->fd, EPOLLIN|EPOLLOUT);

	return err;
}


Do you think that we need this? I guess that we don't need it but it's
a safer option.