[Stgt-devel] iSER

[FUJITA Tomonori]

On Fri, 12 Oct 2007 12:34:58 -0400
Pete Wyckoff <pw at osc.edu> wrote:

> fujita.tomonori at lab.ntt.co.jp wrote on Fri, 12 Oct 2007 10:48 +0900:
> > 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.
> 
> Will do.

Thanks.


> > 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.
> 
> Hrm.  We've decided to execute the task.  But maybe all the data
> hasn't arrived yet.  If unsolicited data has all shown up, and we
> still need to generate more R2Ts, put task on tx_clist and turn
> on EPOLLOUT.  That's pretty straightforward.
> 
> But if all the write data is in, and we queue the task to run, there
> is no way it will be ready to transmit again.  Until target calls
> iscsi_scsi_cmd_done(), which it always does, both for sync and async
> backing stores.
> 
> So the only way we could need this extra EPOLLOUT line is if there
> were a bug elsewhere, in my opinion.  For a sync target, it will
> just duplicate the call at the end of cmd_done().  For an async
> target, it will force a trip through iscsi_task_tx_start() that
> will turn it off as soon as it sees that tx_clist is empty.

Yeah, I think so.


> I tried it with and without.  Didn't seem to matter.  Extra call
> is very fast, sub-microsecond.  You can do a clean revert or take
> the patch when I send it---your call.

I did a clean revert just because the original code have been tested
for a long time in my lab.

As you said, I think that we don't need to set EPOLLIN|EPOLLOUT there
so I'm happy to change it later on (after lots of testings).