On 6/11/14 2:30 PM, "Nicholas A. Bellinger" <nab@xxxxxxxxxxxxxxx> wrote: >On Wed, 2014-06-11 at 10:24 +0300, Sagi Grimberg wrote: >> On 6/11/2014 12:17 AM, Quinn Tran wrote: >> >> <SNIP> >> >> > QT> Instead of using existing value within cmd->data_length, can we >> > calculated data_length based on secstors & blocksize. >> > >> > cmd->data_length = sectors * dev->dev_attrib.block_size; >> >> We can do that I suppose... >> Although it seems weird that the core discards the transport >>byte-count... >> Just wandering if we should recalc on the DIF case only or always. >> > >Yeah, same concern here wrt to discarding the transport length. > >This effectively makes the residual handling further down in >sbc_parse_cdb() -> target_cmd_size_check() always match size == >cmd->data_length, because the latter as been recalculated by the former. > >Honestly, I don't know if this is a problem for normal READ + WRITE with >prot=1 as I've never seen size != cmd->data_length for I/O related >commands, but it does seem potentially dangerous. > >> > >> > From the QLogic perfective, the cmd->data_length is pull directly >>from the >> > wire (i.e. FCP header) when cmd is received. In essence, it's >>whatever >> > the Initiator is set it to. We does not have any indicator to spot >>DIF vs >> > none-DIF cmd when 1st received, unless the code do a peek. >> >> Same for all transports I assume... >> > >So just to confirm Quinn, the Qlogic the initiator includes the PI bytes >into the FCP header data_length for the target-side *_PASS and >DOUT_STRIP / DIN_INSERT, that is currently passed into >qla_tgt_ops->handle_cmd(), right..? QT> Initiator: DOUT_STRIP/DIN_Insert: FCP_DL = data length only (no dif length) Dif PASS: FCP_DL = data length + Dif length. Target: DOUT_strip/DIN_Insert: qla_tgt_ops->handle_cmd(data length only) sbc_check_prot() If (protect) cmd->data_length -= cmd->prot_length; << truncation --- DIF_PASS: qla_tgt_ops->handle_cmd(data length + Dif length) > >If that is the case, Sagi's v1 to cmd->data_length -= cmd->prot_length >seems like it would still do right thing for *_PASS and DOUT_STRIP / >DIN_INSERT, given that cmd->prot_length is calculated in >sbc_check_prot() based upon dev->prot_length * sectors.. > >> > >> > With that said, the cmd->data_length does not guarantee to contain >>both >> > "data length" & "protection length" when cmd is submit to >> > TCM/target_submit_cmd(). In Dif-Insert mode, data_length will only >> > contain the actual data(no Dif). >> >> No, in the DOUT_INSERT/DIN_STRIP case, protect bits are off and the >>core >> will take the data length as is. >> This case is covered. >> > ><nod> QT> agree. > >> > It's best that the SBC code re-calculate the actual data length and >>dif >> > data length based on the number of sectors derived from the cmd. >> >> Nic, what's your take on this? >> > >Hard to say. Discarding the transport length in v2 doesn't seem like a >good idea, but subtracting from cmd->prot_length in v1 is using the >sector count from the CDB anyways, so it's essentially the same tradeoff >of recalculating the transport's cmd->data_length from values within the >CDB w/ prot=1. > >MKP, WDYT..? > >--nab > > > > ________________________________ This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
<<attachment: winmail.dat>>
