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:
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..?
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?
Sagi.
--
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
