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 this code exercised on the first target that is in
> pass-through over the initiator. Or this code is at the other target at the other
> side of the initiator? (The final real target)
>
> Because if it's at the pass-through target then this test might or might not
> be correct because we lost the overflow information by now. (Now if resid was
> negative that would be another thing)
>
>> 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;
>>
>
>
> Thanks
> Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
