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..? 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> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
