On Tue, 2014-03-11 at 11:56 +0200, Sagi Grimberg wrote:
> On 3/11/2014 4:21 AM, Nicholas A. Bellinger wrote:
> > 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.
>
> Are you referring to iser initiator or iser target? or both?
>
I was referring to the initiator side performing the READ_STRIP +
WRITE_GENERATE ops using hardware offload.
> >
> > Care to re-spin this particular patch with this in mind..?
> >
> > --nab
> >
>
> Hey Nic,
>
> So if I understand the suggestion correctly, I'm facing a chicken and
> egg conflict loop here for isert.
> iser/iscsi calls target_setup_cmd_from_cdb() and then
> target_generic_new_cmd().
> Meaning it allocates the data+protection buffers according the
> specification in se_cmd
> (set according to the CDB and the backstore configuration). So re
> resetting se_cmd->prot_op
> to TARGET_PROT_NORMAL when no accompanying protection buffer has been
> passed by the
> fabric driver would not allow isert to support ADD/STRIP since the
> allocation part comes after.
>
> What we can do here is reset se_cmd->prot_op to TARGET_PROT_NORMAL in
> target_submit_cmd_map_sgls()
> *after* calling target_setup_cmd_from_cdb() . This will address tcm_loop
> & vhost_scsi but would keep
> isert working also against legacy initiators.
>
> Is the below acceptable?
>
> diff --git a/drivers/target/target_core_transport.c
> b/drivers/target/target_core_transport.c
> index ed84783..f48256f 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1387,11 +1387,9 @@ int target_submit_cmd_map_sgls(struct se_cmd
> *se_cmd, struct se_session *se_sess
> */
> if ((!se_cmd->t_prot_sg || !se_cmd->t_prot_nents) &&
> (se_cmd->prot_op != TARGET_PROT_NORMAL)) {
> - pr_err("ERROR: protection information was requested but "
> - "protection buffers weren't provided.\n");
> - transport_generic_request_failure(se_cmd,
> - TCM_INVALID_CDB_FIELD);
> - return 0;
> + pr_debug("protection information was requested but "
> + "protection buffers weren't provided. continue
> unprotected...\n");
> + se_cmd->prot_op = TARGET_PROT_NORMAL;
> }
>
> /*
>
Mmmm, so this would work for tcm_loop + vhost-scsi who are using
target_submit_cmd_map_sgls(), and ib_isert + iscsi-target who's not due
to some strange immediatedata + initialr2t requirements..
However, another fabric like ib_srpt who's using target_submit_cmd()
would run into the problem of hitting a false positive here, and end up
resetting to TARGET_PROT_NORMAL for all cases..
I'm starting to wonder if adding a new se_cmd bit that is set in
tcm_loop + vhost-scsi when this special case is detected, and forces
TARGET_PROT_NORMAL is easier to manage for all cases..?
--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
