| Message ID | 1402387450.5774.56.camel@haakon3.risingtidesystems.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Il 10/06/2014 10:04, Nicholas A. Bellinger ha scritto: > > That said, there is one other small qla2xxx change to enable per-session > PI that is currently missing in Quinn's patch in scsi/for-next code. > > Sooo, I'll go ahead and include Sagi's patches with the vhost-scsi > change below if there are no objections from MKP and/or driver > maintainers, and post an -rc2 series after the merge window closes to > address the two remaining qla2xxx specific items. Indeed the patch looks like a tcm bugfix. There is no reason to modify the LLD. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
All, Comments inline. Regards, Quinn Tran On 6/10/14 1:04 AM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote: >Hi Sagi & Co, > >On Sun, 2014-06-08 at 13:27 +0300, Sagi Grimberg wrote: >> In various areas of the code, it is assumed that >> se_cmd->data_length describes pure data. In case >> that protection information exists over the wire >> (protect bits is are on) the target core decrease >> the protection length from the data length (instead >> of each transport peeking in the cdb). >> >> Modify loopback device to include protection information >> in the transferred data length (like other scsi transports). >> >> Signed-off-by: Sagi Grimberg <sagig@mellanox.com> >> --- >> drivers/target/loopback/tcm_loop.c | 15 ++++++++++++--- >> drivers/target/target_core_sbc.c | 15 +++++++++++++-- >> 2 files changed, 25 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/target/loopback/tcm_loop.c >>b/drivers/target/loopback/tcm_loop.c >> index c886ad1..1f4c015 100644 >> --- a/drivers/target/loopback/tcm_loop.c >> +++ b/drivers/target/loopback/tcm_loop.c >> @@ -179,7 +179,7 @@ static void tcm_loop_submission_work(struct >>work_struct *work) >> struct tcm_loop_hba *tl_hba; >> struct tcm_loop_tpg *tl_tpg; >> struct scatterlist *sgl_bidi = NULL; >> - u32 sgl_bidi_count = 0; >> + u32 sgl_bidi_count = 0, transfer_length; >> int rc; >> >> tl_hba = *(struct tcm_loop_hba **)shost_priv(sc->device->host); >> @@ -213,12 +213,21 @@ static void tcm_loop_submission_work(struct >>work_struct *work) >> >> } >> >> - if (!scsi_prot_sg_count(sc) && scsi_get_prot_op(sc) != >>SCSI_PROT_NORMAL) >> + transfer_length = scsi_transfer_length(sc); >> + if (!scsi_prot_sg_count(sc) && >> + scsi_get_prot_op(sc) != SCSI_PROT_NORMAL) { >> se_cmd->prot_pto = true; >> + /* >> + * loopback transport doesn't support >> + * WRITE_GENERATE, READ_STRIP protection >> + * information operations, go ahead unprotected. >> + */ >> + transfer_length = scsi_bufflen(sc); >> + } >> >> rc = target_submit_cmd_map_sgls(se_cmd, tl_nexus->se_sess, sc->cmnd, >> &tl_cmd->tl_sense_buf[0], tl_cmd->sc->device->lun, >> - scsi_bufflen(sc), tcm_loop_sam_attr(sc), >> + transfer_length, tcm_loop_sam_attr(sc), >> sc->sc_data_direction, 0, >> scsi_sglist(sc), scsi_sg_count(sc), >> sgl_bidi, sgl_bidi_count, >> diff --git a/drivers/target/target_core_sbc.c >>b/drivers/target/target_core_sbc.c >> index e022959..06f8ecd 100644 >> --- a/drivers/target/target_core_sbc.c >> +++ b/drivers/target/target_core_sbc.c >> @@ -665,8 +665,19 @@ sbc_check_prot(struct se_device *dev, struct >>se_cmd *cmd, unsigned char *cdb, >> >> cmd->prot_type = dev->dev_attrib.pi_prot_type; >> cmd->prot_length = dev->prot_length * sectors; >> - pr_debug("%s: prot_type=%d, prot_length=%d prot_op=%d >>prot_checks=%d\n", >> - __func__, cmd->prot_type, cmd->prot_length, >> + >> + /** >> + * In case protection information exists over the wire >> + * we modify command data length to describe pure data. >> + * The actual transfer length is data length + protection >> + * length >> + **/ >> + if (protect) >> + cmd->data_length -= cmd->prot_length; >> + 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; 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. 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). 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. Thanks. > >This fabric wide assumption breaks the vhost-scsi + tcm_qla2xxx PI code >already queued for v3.16-rc1.. > >A vhost-scsi side conversion to combine exp_data_len + prot_bytes is >easy enough in target-pending/for-next (see below), given that >vhost-scsi's exp_data_len is coming from virtio buffer iovecs, so >changing virtio-scsi LLD to use scsi_transfer_length() doesn't really >make any sense. (Adding CC's for MST + Paolo) > >The qla2xxx target T10 PI patch in scsi/for-next (adding Quinn CC) will >also need a similar change to update qlt_do_work() to include PI bytes >for data_length being passed into tgt_ops->handle_cmd(), following what >qlt_build_ctio_crc2_pkt() is already doing for calculating >transfer_length for HW offload. > >That said, there is one other small qla2xxx change to enable per-session >PI that is currently missing in Quinn's patch in scsi/for-next code. QT> I've been bog down with other front, I have not had a chance to come back and re-patch this area. Will find some cycle to work on this soon. > >Sooo, I'll go ahead and include Sagi's patches with the vhost-scsi >change below if there are no objections from MKP and/or driver >maintainers, and post an -rc2 series after the merge window closes to >address the two remaining qla2xxx specific items. > >--nab > >diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c >index 667e72d..03e484f 100644 >--- a/drivers/vhost/scsi.c >+++ b/drivers/vhost/scsi.c >@@ -1144,7 +1144,8 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct >vhost_virtqueue *vq) > } > > cmd = vhost_scsi_get_tag(vq, tpg, cdb, tag, lun, >task_attr, >- exp_data_len, data_direction); >+ exp_data_len + prot_bytes, >+ data_direction); > if (IS_ERR(cmd)) { > vq_err(vq, "vhost_scsi_get_tag failed %ld\n", > PTR_ERR(cmd)); >
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. > > 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... > > 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. > 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? Sagi. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/11/14 2:30 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote: >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..? QT> Initiator: DOUT_STRIP/DIN_Insert: FCP_DL = data length only (no dif length) Dif PASS: FCP_DL = data length + Dif length. Target: DOUT_strip/DIN_Insert: qla_tgt_ops->handle_cmd(data length only) sbc_check_prot() If (protect) cmd->data_length -= cmd->prot_length; << truncation --- DIF_PASS: qla_tgt_ops->handle_cmd(data length + Dif length) > >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> QT> agree. > >> > 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 > > > >
>>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:
nab> Hard to say. Discarding the transport length in v2 doesn't seem
nab> like a good idea, but subtracting from cmd->prot_length in v1 is
nab> using the sector count from the CDB anyways, so it's essentially
nab> the same tradeoff of recalculating the transport's cmd->data_length
nab> from values within the CDB w/ prot=1.
nab> MKP, WDYT..?
My general feeling is that once sbc on the target sees {rd,wr}protect >
0 we're in the territory where you should start separating data and
PI internally.
On Wed, 2014-06-11 at 22:32 +0000, Quinn Tran wrote: > > On 6/11/14 2:30 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote: > > >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..? > > QT> > Initiator: > DOUT_STRIP/DIN_Insert: FCP_DL = data length only (no dif length) > Dif PASS: FCP_DL = data length + Dif length. > > Target: > DOUT_strip/DIN_Insert: qla_tgt_ops->handle_cmd(data length only) > > sbc_check_prot() > If (protect) > cmd->data_length -= cmd->prot_length; << truncation > > <nod> > --- > DIF_PASS: qla_tgt_ops->handle_cmd(data length + Dif length) > > Ok, so Sagi's patches for legacy fabric <-> PI backend and PI fabric <-> PI backend cases look OK, but not for the PI fabric <-> legacy backend case as noted above.. Given DOUT_STRIP + DIN_INSERT have not been enabled in sbc_check_prot() just yet, I'll go ahead and merge his v2 series to address the two existing cases in -rc1 + v3.15.y stable code. From there, enabling PI fabric <-> legacy backend support in >= rc2 with target-core will be easy, as it's just a matter of updating sbc_set_prot_op_checks() to assign prot_ops, avoid sbc_check_prot() cmd->data_length recalculation, and making sure PROTECTION control feature bits are still exposed for legacy -> unprotected backends. Thanks! --nab -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index 667e72d..03e484f 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -1144,7 +1144,8 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq) } cmd = vhost_scsi_get_tag(vq, tpg, cdb, tag, lun, task_attr, - exp_data_len, data_direction); + exp_data_len + prot_bytes, + data_direction); if (IS_ERR(cmd)) { vq_err(vq, "vhost_scsi_get_tag failed %ld\n", PTR_ERR(cmd));