On Thu, 2014-03-13 at 10:15 +0200, Sagi Grimberg wrote:
> On 3/12/2014 8:11 PM, Nicholas A. Bellinger wrote:
> > On Tue, 2014-03-11 at 11:56 +0200, Sagi Grimberg wrote:
> >> On 3/11/2014 4:21 AM, Nicholas A. Bellinger wrote:
<SNIP>
> >>> 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..?
>
> I think that it will be easiest to control if the transports could
> execute the same flow
> (or at least vary less). Since this is not the case, we can carry an
> extra bit "prot_pto"
> (stands for protection pass-through-only) in se_cmd for
> tcm_loop/vhost_scsi to set.
>
> Agreed?
>
<nod>
Sounds reasonable to add a se_cmd->prot_pto bit for these two special
cases, and drop the target_submit_cmd_map_sgls() specific parts of this
patch.
--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
