On 08/06/2014 03:43 PM, Sagi Grimberg wrote: > Hi Boaz, > <> >> >> I hate that you introduced this new transfer_length variable. It does >> not exist. In BIDI supporting driver there is out_len and in_len just >> as original code. > > Effectively, out_len and in_len are the same except for the bidi case. > Moreover, the hdr->data_length is exactly the command scsi data buffer > length, the bidi length is taken from scsi_in when we build the AHS for > bidi (rlen_ahdr->read_length). > > So to me it is correct and makes sense. But I'm don't feel so strong > about it... > > Mike what's your take on this? > I have a patch to clean all that, will send tomorrow. What I mean is that this is on the out-path only the in path is different. See the code this variable is only used in the if (== DMA_TO_DEVICE) case and should be local to that scope. This is my clean up <> >> this particular driver puts them together in the same payload. There can be >> other DIFF supporting drivers that put the DIFF payload on another stream / another >> SG list, like the sata one, right? > > I think that DIF specification says that on the wire the data and > protection are interleaved i.e. Block1, DIF1, Block2, DIF2... > No it does not. This is a per transport, and actually per device host driver. Yes in iSCSI_tcp they are inline in HW cards they might come as two different SGs (Like the Linux model). Even with iscsi-offload they might want to be two SG-lists. > So I do think that the transfer length should always include > data_length + protection_length. > This is at the iscsi level. But the scsi_transfer_length() is on the scsi level which keeps them separate. So I think the proper API should be scsi_proto_length() And for LLDs that want them together they can do scsi_bufflen() + scsi_proto_length() and for other drivers they can do it separately. Don't infect iscsi level assumptions on the generic layer API. Again my patch fixes this. >> And this >> >> This print is correct as it covers all cases. If you want to print the adjusted >> length then OK, you'd need to store this I guess, but store it as a different >> variable like proto_length and print it as an additional variable. > > But it is the transfer length that is sent in iSCSI header. Why do you > want to print it as additional info? I want to see what was the length the app/FS sent, then as added info how much was added for DIFF, your way there is lost information. > for transactions that include DIF > the length is the data + protection. > > It is still one-to-one isn't it? > No! the original submitted length is lost from the print > Sagi. > Shalom 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
