Re: [PATCH] target: Remove unused se_cmd.cmd_spdtl
On Fri, Aug 17, 2012 at 6:02 PM, Nicholas A. Bellinger n...@linux-iscsi.org wrote: No, or at least that is not what happens anymore with current target core + iscsi-target code.. The se_cmd-data_length re-assignment here is what will be used by iscsi-target fabric code for all iSCSI descriptor related allocations +ransfer, instead of the original fabric 'expected transfer length' that declares the size of the SCSI initiator's available buffer at the other end. Not sure I follow this. Isn't cmd-data_length also what we use to allocate the buffer used to store the data we're going to transfer? I guess my example with READ(10) actually works, because iblock_execute_rw() just uses the buffer allocated, rather than paying attention to the sector count in the command. But what if an initiator sends, say, REPORT LUNS or PR OUT with an allocation length of 8192, but a transport-level length of only 4096? If the REPORT LUNS or PR OUT response is bigger than 4096 bytes, we'll overflow the allocated buffer with your patch, right? - R. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] target: Remove unused se_cmd.cmd_spdtl
We should switch this topic to the scsi mailing list
On 08/17/2012 01:49 PM, Boaz Harrosh wrote:
On 08/17/2012 01:12 AM, Nicholas A. Bellinger wrote:
On Thu, 2012-08-16 at 09:24 -0700, Roland Dreier wrote:
Actually I'm not sure removing cmd_spdtl is the right answer.
If /dev/sda is a device on an iSCSI initiator exported by an LIO
target, try doing:
# sg_raw -r512 /dev/sda 28 0 0 0 0 0 0 0 2 0
This issues a READ (10) for 2 sectors, but only sends a length of 512
at the transfer level.
The target responds by setting the residual to 512 but transmits all
1024 bytes,
Is this the correct behavior from the Target? I would imagine that the
target needs to only send 512 bytes (transfer level size) and set the
OVERFLOW bit and residual to 512.
Not that it really matter because as you stated below the Initiator makes
sure nothing bad happens.
BTW what target are we talking about, on the other side of the initiator
here? (There are two targets in this setup right?)
and the Linux initiator at least rejects it because it
hits:
if (tcp_task-data_offset + tcp_conn-in.datalen total_in_length) {
ISCSI_DBG_TCP(conn, data_offset(%d) + data_len(%d)
total_length_in(%d)\n, tcp_task-data_offset,
tcp_conn-in.datalen, total_in_length);
return ISCSI_ERR_DATA_OFFSET;
}
Ok, this is the 'overflow' case when the fabric -data_length (expected
data transfer length of the initiator's buffer) is smaller than the SCSI
CDB allocation length or sectors*block_size (attempted transfer length)
the target has been asked to process.
The following patch which appears to do the right thing from the
perspective of the target for overflow, but AFAICT open-iscsi still
returns GOOD status w/ no data-in payload (at least via sg_raw) when the
iscsi-target is signaling overflow bit in iSCSI Data IN PDU. Not sure
yet why this is the case, but drivers/scsi/libiscsi.c:iscsi_data_in_rsp
code:
if (rhdr-flags (ISCSI_FLAG_DATA_UNDERFLOW |
ISCSI_FLAG_DATA_OVERFLOW)) {
int res_count = be32_to_cpu(rhdr-residual_count);
if (res_count 0
(rhdr-flags ISCSI_FLAG_CMD_OVERFLOW ||
res_count = scsi_in(sc)-length))
scsi_in(sc)-resid = res_count;
else
sc-result = (DID_BAD_TARGET 16) |
rhdr-cmd_status;
}
OK I admit I kind of rearranged all this code a few years ago. Guilty ;-)
Well now that I look at it again, I think it is totally wrong!!
The scsi and block layer do not know anything about CMD_OVERFLOW
If a scsi_in/out(sc)-resid is set it only ever means UNDERFLOW.
Both scsi and block expects to only do transfer_length - resid.
This is why you get empty buffer back because the transfer_length=512
minus resid=512 means zero bytes.
So the || ISCSI_FLAG_CMD_OVERFLOW case is wrong.
Now the big question is what to do. Fail the all command, or say
nothing?
The correct thing is to teach the middle layers about overflow,
With some kind of warning level system.
I was also thinking that we can make resid signed and signal
an overflow with a negative resid. Now the math of
transfer_length - resid will become correct since it means
more, in the case above 512 - (-512) would be our 1024 CDB len.
For now this code must be fixed. And the command must fail.
Do you need that I prepare a patch? (Please you do it, I'm
swamped, it'll take me time)
There are 3 more places like this.
BTW did you notice how in the code above we have mixed up the
use of: ISCSI_FLAG_DATA_OVERFLOW and ISCSI_FLAG_CMD_OVERFLOW?
That's another bug, here it should be ISCSI_FLAG_DATA_OVERFLOW
only. The other places with the other flags.
appears to be setting resid for non bidi cases correctly, right..? (mnc
+ boaz CC'ed)
How about the following to ensure we restrict overflow to keep the
existing (smaller) cmd-data_length assignment, and only re-assign this
value for the underflow case..? (hch CC'ed)
WDYT..?
diff --git a/drivers/target/target_core_transport.c
b/drivers/target/target_core_transport.c
index 0eaae23..63b3594 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1183,15 +1183,20 @@ int target_cmd_size_check(struct se_cmd *cmd,
unsigned int size)
/* Returns CHECK_CONDITION + INVALID_CDB_FIELD */
goto out_invalid_cdb_field;
}
-
+ /*
+* For the overflow case keep the existing fabric provided
+* -data_length. Otherwise for the underflow case, reset
+* -data_length to the smaller SCSI expected data transfer
+* length.
+*/
if (size cmd-data_length) {
I'm a bit out of context. Is
Re: [PATCH] target: Remove unused se_cmd.cmd_spdtl
(CC'ing linux-scsi as per request by Boaz)
On Fri, 2012-08-17 at 13:49 +0300, Boaz Harrosh wrote:
On 08/17/2012 01:12 AM, Nicholas A. Bellinger wrote:
On Thu, 2012-08-16 at 09:24 -0700, Roland Dreier wrote:
Actually I'm not sure removing cmd_spdtl is the right answer.
If /dev/sda is a device on an iSCSI initiator exported by an LIO
target, try doing:
# sg_raw -r512 /dev/sda 28 0 0 0 0 0 0 0 2 0
This issues a READ (10) for 2 sectors, but only sends a length of 512
at the transfer level.
The target responds by setting the residual to 512 but transmits all
1024 bytes,
Is this the correct behavior from the Target? I would imagine that the
target needs to only send 512 bytes (transfer level size) and set the
OVERFLOW bit and residual to 512.
So my proposed target patch below to keep the smaller cmd-data_length
assignment for the SCSI overflow case in target_cmd_size_check() makes
the target do the right thing here, which is:
Transfer 512 bytes and set the OVERFLOW bit in se_cmd (to signal to the
fabric to also set OVERFLOW bit) with a residual count of 512 bytes
reported during the DATAIN sequence back across the wire..
Not that it really matter because as you stated below the Initiator makes
sure nothing bad happens.
BTW what target are we talking about, on the other side of the initiator
here? (There are two targets in this setup right?)
This is on the final target side running TCM + fabric driver
and the Linux initiator at least rejects it because it
hits:
if (tcp_task-data_offset + tcp_conn-in.datalen total_in_length) {
ISCSI_DBG_TCP(conn, data_offset(%d) + data_len(%d)
total_length_in(%d)\n, tcp_task-data_offset,
tcp_conn-in.datalen, total_in_length);
return ISCSI_ERR_DATA_OFFSET;
}
Ok, this is the 'overflow' case when the fabric -data_length (expected
data transfer length of the initiator's buffer) is smaller than the SCSI
CDB allocation length or sectors*block_size (attempted transfer length)
the target has been asked to process.
The following patch which appears to do the right thing from the
perspective of the target for overflow, but AFAICT open-iscsi still
returns GOOD status w/ no data-in payload (at least via sg_raw) when the
iscsi-target is signaling overflow bit in iSCSI Data IN PDU. Not sure
yet why this is the case, but drivers/scsi/libiscsi.c:iscsi_data_in_rsp
code:
if (rhdr-flags (ISCSI_FLAG_DATA_UNDERFLOW |
ISCSI_FLAG_DATA_OVERFLOW)) {
int res_count = be32_to_cpu(rhdr-residual_count);
if (res_count 0
(rhdr-flags ISCSI_FLAG_CMD_OVERFLOW ||
res_count = scsi_in(sc)-length))
scsi_in(sc)-resid = res_count;
else
sc-result = (DID_BAD_TARGET 16) |
rhdr-cmd_status;
}
OK I admit I kind of rearranged all this code a few years ago. Guilty ;-)
Well now that I look at it again, I think it is totally wrong!!
The scsi and block layer do not know anything about CMD_OVERFLOW
If a scsi_in/out(sc)-resid is set it only ever means UNDERFLOW.
Both scsi and block expects to only do transfer_length - resid.
This is why you get empty buffer back because the transfer_length=512
minus resid=512 means zero bytes.
So the || ISCSI_FLAG_CMD_OVERFLOW case is wrong.
Mmmm, your're right.. I could have sworn at one point that open-iscsi
returned DID_BAD_TARGET status for all OVERFLOW cases, and set the
proper residual + completed scsi_cmnd for UNDERFLOW cases.
Doing the reverse in the current code is definitely not correct.
Now the big question is what to do. Fail the all command, or say
nothing?
The correct thing is to teach the middle layers about overflow,
With some kind of warning level system.
I was also thinking that we can make resid signed and signal
an overflow with a negative resid. Now the math of
transfer_length - resid will become correct since it means
more, in the case above 512 - (-512) would be our 1024 CDB len.
For now this code must be fixed. And the command must fail.
Do you need that I prepare a patch? (Please you do it, I'm
swamped, it'll take me time)
There are 3 more places like this.
Swamped here as well atm.. mnc/hannes, would you mind taking a look at
a patch to address these bugs in open-iscsi..? I'm happy to test them
against target-pending to make sure we get these correct, and add them
into scsi-testsuite for your own use.
BTW did you notice how in the code above we have mixed up the
use of: ISCSI_FLAG_DATA_OVERFLOW and ISCSI_FLAG_CMD_OVERFLOW?
That's another bug, here it should be ISCSI_FLAG_DATA_OVERFLOW
only. The other places with the other flags.
nod
appears to be setting resid for non bidi cases correctly, right..? (mnc
+ boaz CC'ed)
How
Re: [PATCH] target: Remove unused se_cmd.cmd_spdtl
On Fri, 2012-08-17 at 11:46 -0700, Roland Dreier wrote:
On Thu, Aug 16, 2012 at 3:12 PM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
diff --git a/drivers/target/target_core_transport.c
b/drivers/target/target_core_transport.c
index 0eaae23..63b3594 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1183,15 +1183,20 @@ int target_cmd_size_check(struct se_cmd *cmd,
unsigned int size)
/* Returns CHECK_CONDITION + INVALID_CDB_FIELD */
goto out_invalid_cdb_field;
}
-
+ /*
+* For the overflow case keep the existing fabric provided
+* -data_length. Otherwise for the underflow case, reset
+* -data_length to the smaller SCSI expected data transfer
+* length.
+*/
if (size cmd-data_length) {
cmd-se_cmd_flags |= SCF_OVERFLOW_BIT;
cmd-residual_count = (size - cmd-data_length);
} else {
cmd-se_cmd_flags |= SCF_UNDERFLOW_BIT;
cmd-residual_count = (cmd-data_length - size);
+ cmd-data_length = size;
}
- cmd-data_length = size;
}
return 0;
I don't think this can work... if my memory of the code is right, if we do
this,
then the target core will only allocate the smaller (fabric) data length worth
of buffer space, but then it will actually execute whatever the CDB says to
do.
No, or at least that is not what happens anymore with current target
core + iscsi-target code..
The se_cmd-data_length re-assignment here is what will be used by
iscsi-target fabric code for all iSCSI descriptor related allocations
+ransfer, instead of the original fabric 'expected transfer length' that
declares the size of the SCSI initiator's available buffer at the other
end.
At one point in v4.0 code iscsi-target had it's own
iscsi_cmd-data_length (which might be what your thinking of), but this
was removed during 3.4 by Agrover in favor of just using
se_cmd-data_length from the possibly underflow adjusted value.
So in my original example we're probably OK because we'll allocate a
full page anyway, but I think something like:
# sg_raw -r512 /dev/sda 28 0 0 0 0 0 0 0 9 0
would cause the target core to allocate 1 page but then read 9 sectors.
With the patch above to target_cmd_size_check() in place to skip
re-assignment of se_cmd-data_length for the overflow case, it appears
the target (at least) is doing the right thing now with your test case:
[root@initiator ~]# sg_raw -r512 /dev/sdi 28 0 0 0 0 0 0 0 9 0
sg_raw -r512 /dev/sdi 28 0 0 0 0 0 0 0 9 0
SCSI Status: Good
Sense Information:
sense buffer empty
Received -3584 bytes of data:
Target side:
[82582.697668] Got SCSI Command, ITT: 0x7e40, CmdSN: 0x02d9,
ExpXferLen: 512, Length: 0, CID: 0
[82582.697673] TARGET_CORE[iSCSI]: Expected Transfer Length: 512 does not match
SCSI CDB Length: 4608 for SAM Opcode: 0x28
[82582.709696] Received CmdSN matches ExpCmdSN, incremented ExpCmdSN to:
0x02da
[82582.709876] Updated MaxCmdSN to 0x02f9
[82582.709881] Built DataIN ITT: 0x7e40, StatSN: 0x905625fd, DataSN:
0x, Offset: 0, Length: 512, CID: 0
So aside from the open-iscsi client bugs that Boaz has pointed out wrt
to overflow/underflow handling, I'll verify over the weekend with
loopback + tcm_qla2xxx fabrics and if everything looks fine will plan to
include this patch into master - for next week's 3.6-rc-fixes PULL
request.
Thanks Roland!
--nab
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
