Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux