[Stgt-devel] your recent sense checkin

[FUJITA Tomonori]

From: Pete Wyckoff <pw at osc.edu>
Subject: [Stgt-devel] your recent sense checkin
Date: Wed, 28 Feb 2007 15:13:23 -0500

> I took a look at your SVN and rebased my patches on top of it.

Oh, thanks a lot. I can take care of you patches. I need to clean up
something before merging them.


>  One thing to point out: it should be possible for a target to send
> back both data-in bytes as the result of a command, and status bytes
> in the response.  Your use of task->addr to get the status bytes
> appears to make that impossible.  I had added extra task->sense and
> task->senselen to accommodate this usage.

Yeah. We've been fixing sense issues in tgt in-kernel component.

You add:

@@ -94,6 +94,8 @@ struct scsi_cmd {

 	unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];
+	uint8_t *sense;    /* output values from target */

scsi_cmd structure has two sense_buffer. one is for some lld and one
is a pointer for odd llds.

Then, you add super long sense buffer to struct iscsi_connection:

@@ -146,6 +148,7 @@ struct iscsi_connection {
 	struct iscsi_pdu req;
 	void *req_buffer;
 	struct iscsi_pdu rsp;
+	uint8_t sense_buffer[252];  /* architectural limit */


I just want to have one sense_buffer in scsi_cmd even even if it's
large.

The problem is that tgt in-kernel component can't accept unaligned
buffer for sense (and as you pointed out, we wrongly use uaddr and len
fields in cmd_rsp struct for sense). I've been testing the patches.
When I finish it, I can fix tgtd.


> Regarding the "Need to clean up this mess" comment in
> iscsi_scsi_cmd_tx_start:  it gets worse once we mix in the
> bidirectional.  There you cannot do a phase collapse on the final
> data packet, so it becomes even more of a mess.

Nice.


> I have cleaned most of that mess up, perhaps, as part of getting
> bidirectional transfers to work, but the patches are not ready for
> submission to stgt because they depend on AHS structures defined
> only in non-mainline kernel patches.  If you are willing to remove
> the dependency on kernel headers by copying the relevant bits into
> stgt, we can avoid that problem.

I'm ok with using the nice header instead of upstream header files.


> Note the duplication in iscsi_cmd_rsp_build and
> iscsi_sense_rsp_build; wouldn't it be better to have a single
> function with "if (task->result != 0) {...}" to return status?

Probabaly.


> Here they are, fyi.  Let me know if you have suggestions on how I
> can help you get more of this merged.

Sorry for taking so long to merge your patches. Surely, I'll merge
your patches.

I'll try to finish multiple device support sutff this weekend. Yeah, I
did last week and fixed AIO stuff instead. tgtd works with any
upstream kernels. It's as neat as multiple device support, isn't it ;)