All,
Comments inline.
Regards,
Quinn Tran
On 6/10/14 1:04 AM, "Nicholas A. Bellinger" <nab@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> ---
>> 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));
>
________________________________
This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
<<attachment: winmail.dat>>
