On 6/25/2014 6:32 AM, Mike Christie wrote:
On 06/24/2014 12:08 PM, Mike Christie wrote:
On 06/24/2014 12:00 PM, Mike Christie wrote:
On 06/24/2014 11:30 AM, Christoph Hellwig wrote:
On Tue, Jun 24, 2014 at 07:27:46PM +0300, Sagi Grimberg wrote:
This condition only matters in the bidi case, which is not
relevant for the PI case. I suggested to condition that in
libiscsi (posted in the second thread, copy-paste below).
Although I do agree that scsi_transfer_length() helper is not
really just for PI and not more. I think Mike's way is
cleaner.
But for bidi there are two transfers. So either
scsi_transfer_length() needs to take the scsi_data_buffer, or we
need to avoid using it.
For 3.16 I'd prefer something like you're patch below. This
patch which has been rushed in last minute and not through the
scsi tree has already causes enough harm. If you can come up
with a clean version to transparently handle the bidi case I'd be
happy to pick that up for 3.17.
In the meantime please provide a version of the patch below with
a proper description and signoff.
It would be nice to just have one function to call and it just do
the right thing for the drivers. I am fine with Sagi's libiscsi
patch for now though:
Acked-by: Mike Christie <micha...@cs.wisc.edu>
Actually, let me take this back for a second. I am not sure if that
is right.
Hey Mike,
Sagi's patch was not correct because scsi_in was hardcoded to the transfer
len when bidi was used.
Right, should have condition that in the direction. something like:
transfer_length = sc->sc_data_direction == DMA_TO_DEVICE ?
scsi_out(sc)->length : scsi_in(sc)->length;
would probably hit that in testing before sending out a patch.
I made the patch below which should fix both bidi
support in iscsi and also WRITE_SAME (and similar commands) support.
I'm a bit confused, for all commands (bidi or not) the iscsi header data_length
should describe the scsi_data_buffer length, bidi information will lie in AHS
header.
(in case bidi will ever co-exist with PI, we might need another helper that
will look
in req->special + PI, something like scsi_bidi_transfer_length)
So why not keep scsi_transfer_length to work on sdb length (take
scsi_bufflen(scmnd) or
scsi_out(scmnd)->length as MKP suggested) and that's it - don't touch libiscsi.
Let me test that.
There is one issue/question though. Is this working ok with LIO? I left
scsi_transfer_length() with the same behavior as before assuming LIO was
ok and only the iscsi initiator had suffered a regression.
I think that if we go with scsi_in/out_transfer_length then
scsi_transfer_length should be removed
and LIO can be modified to use scsi_in/out_transfer_length.
------------------
[PATCH] iscsi/scsi: Fix transfer len calculation
I'll comment on the patch as well if we decide to go this way.
This patch fixes the following regressions added in commit
d77e65350f2d82dfa0557707d505711f5a43c8fd
1. The iscsi header data length is supposed to be the amount of
data being transferred and not the total length of the operation
like is given with blk_rq_bytes.
2. scsi_transfer_length is used in generic iscsi code paths, but
did not take into account bidi commands.
To fix both issues, this patch adds 2 new helpers that use the
scsi_out/in(cmnd)->length values instead of blk_rq_bytes.
Signed-off-by: Mike Christie <micha...@cs.wisc.edu>
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 3d1bc67..ee79cdf 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -338,7 +338,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
struct iscsi_session *session = conn->session;
struct scsi_cmnd *sc = task->sc;
struct iscsi_scsi_req *hdr;
- unsigned hdrlength, cmd_len, transfer_length;
+ unsigned hdrlength, cmd_len;
itt_t itt;
int rc;
@@ -391,11 +391,11 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
if (scsi_get_prot_op(sc) != SCSI_PROT_NORMAL)
task->protected = true;
- transfer_length = scsi_transfer_length(sc);
- hdr->data_length = cpu_to_be32(transfer_length);
if (sc->sc_data_direction == DMA_TO_DEVICE) {
+ unsigned out_len = scsi_out_transfer_length(sc);
struct iscsi_r2t_info *r2t = &task->unsol_r2t;
+ hdr->data_length = cpu_to_be32(out_len);
hdr->flags |= ISCSI_FLAG_CMD_WRITE;
/*
* Write counters:
@@ -414,19 +414,18 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task
*task)
memset(r2t, 0, sizeof(*r2t));
if (session->imm_data_en) {
- if (transfer_length >= session->first_burst)
+ if (out_len >= session->first_burst)
task->imm_count = min(session->first_burst,
conn->max_xmit_dlength);
else
- task->imm_count = min(transfer_length,
+ task->imm_count = min(out_len,
conn->max_xmit_dlength);
hton24(hdr->dlength, task->imm_count);
} else
zero_data(hdr->dlength);
if (!session->initial_r2t_en) {
- r2t->data_length = min(session->first_burst,
- transfer_length) -
+ r2t->data_length = min(session->first_burst, out_len) -
task->imm_count;
r2t->data_offset = task->imm_count;
r2t->ttt = cpu_to_be32(ISCSI_RESERVED_TAG);
@@ -439,6 +438,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
} else {
hdr->flags |= ISCSI_FLAG_CMD_FINAL;
zero_data(hdr->dlength);
+ hdr->data_length = cpu_to_be32(scsi_in_transfer_length(sc));
In light of last comment from Vlad where bidi and PI may co-exist,
shouldn't
scsi_in_transfer_length(sc) be used also in iscsi_prep_bidi_ahs()? and
also in the print below?
if (sc->sc_data_direction == DMA_FROM_DEVICE)
hdr->flags |= ISCSI_FLAG_CMD_READ;
@@ -466,7 +466,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
scsi_bidi_cmnd(sc) ? "bidirectional" :
sc->sc_data_direction == DMA_TO_DEVICE ?
"write" : "read", conn->id, sc, sc->cmnd[0],
- task->itt, transfer_length,
+ task->itt, scsi_bufflen(sc),
In the DIF case length print would be wrong (doesn't include PI).
scsi_bidi_cmnd(sc) ? scsi_in(sc)->length : 0,
session->cmdsn,
session->max_cmdsn - session->exp_cmdsn + 1);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 42ed789..b04812d 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -316,9 +316,9 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd,
char status)
cmd->result = (cmd->result & 0x00ffffff) | (status << 24);
}
-static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
+static inline unsigned __scsi_calculate_transfer_length(struct scsi_cmnd *scmd,
+ unsigned int xfer_len)
{
- unsigned int xfer_len = blk_rq_bytes(scmd->request);
unsigned int prot_op = scsi_get_prot_op(scmd);
unsigned int sector_size = scmd->device->sector_size;
@@ -332,4 +332,20 @@ static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;
}
+static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
+{
+ return __scsi_calculate_transfer_length(scmd,
+ blk_rq_bytes(scmd->request));
+}
+
+static inline unsigned scsi_in_transfer_length(struct scsi_cmnd *scmd)
+{
+ return __scsi_calculate_transfer_length(scmd, scsi_in(scmd)->length);
+}
+
+static inline unsigned scsi_out_transfer_length(struct scsi_cmnd *scmd)
+{
+ return __scsi_calculate_transfer_length(scmd, scsi_out(scmd)->length);
+}
+
#endif /* _SCSI_SCSI_CMND_H */
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
