On Sun, 2014-03-09 at 13:16 +0200, Sagi Grimberg wrote: > On 3/7/2014 10:33 PM, Martin K. Petersen wrote: > >>>>>> "nab" == Nicholas A Bellinger <nab@xxxxxxxxxxxxxxx> writes: > >>> The alternative is to avoid this patch but then we might miss DIF > >>> support when working against legacy initiators. > > nab> Mmmm, not sure about this one.. > > > > nab> Considering that it's only the first two READ_10s that this effects > > nab> before normal SCSI_PROT_*_PASS operation kicks in, I'm not sure if > > nab> it's worth it to add these to tcm_loop.. Note that virtio-scsi + > > nab> vhost-scsi would need similar bits as well.. > > > > The fact that initiator and target are capable of handling protection > > information does not in any way guarantee that all reads and writes will > > have PI included. We need to be able to turn off checking for things > > like recovery and RAID. > > > > So in my book it is crucial that both initiator and target do exactly > > what they are told by the ULD. This means the initiator should only do > > DIX if the prot_op tells it to. > > Right, but what do we expect from the LLD to do when prot_op is > READ_STRIP/WRITE_GENERATE (non-DIX)? > I would expect that for WRITE_GENERATE the LLD will allocate protection > sg-list and compute DIF right? > And for READ_STRIP I would imagine that the LLD will "receive" > protection sg-list from the target and "strip" > it. Now since vhost_scsi/tcm_loop are calling target_submit_map_sgls() > they emulate both the initiator LLD > and the target LLD. So perhaps they should provide the target protection > sg-list for target to do checks and just > not pass it to scsi_cmnd. what do you think? > > > And on the target end you should only do > > DIF transfers and checks if *PROTECT is set in the CDB. > > So you don't think there is any value in offering target that can > protect it's stored data against legacy initiators? > > > Some HBAs support a mode in which they snoop (or silently issue) > > INQUIRY/READ CAPACITY(16). And they will turn on protected transfers > > behind the OS' back under the assumption that the OS is not aware of > > DIF. Let's not go there! > > > > Yes, this doesn't seem like a good isea to me as well. > So back to the original breakage.. I think resetting se_cmd->prot_op to TARGET_PROT_NORMAL when no accompanying protection buffer has been passed by the fabric driver makes the most sense here for the two pre-protection READ_10s headed towards local tcm_loop + vhost-scsi ports. Other than these two special fabric cases, allowing ib_iser (and other fabrics that actually move data over the wire) to perform hardware assisted READ_STRIP + WRITE_GENERATE is perfectly fine based on scsi_cmnd prot_op hints or RDPROTECT + WRPROTECT CDB bits. Care to re-spin this particular patch with this in mind..? --nab -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
