diff mbox

[v2,1/3] scsi_cmnd: Introduce scsi_transfer_length helper

Message ID 53AA42E6.3090101@cs.wisc.edu (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Mike Christie June 25, 2014, 3:32 a.m. UTC
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 <michaelc@cs.wisc.edu>
> 
> Actually, let me take this back for a second. I am not sure if that 
> is right.

Sagi's patch was not correct because scsi_in was hardcoded to the transfer
len when bidi was used. I made the patch below which should fix both bidi
support in iscsi and also WRITE_SAME (and similar commands) support.

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.
------------------


[PATCH] iscsi/scsi: Fix transfer len calculation

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 <michaelc@cs.wisc.edu>



--
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

Comments

Sagi Grimberg June 25, 2014, 8:48 a.m. UTC | #1
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 <michaelc@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 <michaelc@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig June 25, 2014, 9:14 a.m. UTC | #2
Mike, I'd prefer a fix on top of the core-for-3.16 branch in my
scsi-queue tree, which already has the fix from Martin.

I also really don't like these three confusing helpers:

> +static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
> +{
> +	return __scsi_calculate_transfer_length(scmd,
> +						blk_rq_bytes(scmd->request));
> +}

So here we use blk_rq_bytes still, which is incorrect for WRITE SAME.

> +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);

And here we use the in/out length.  And no documentation whatsover which
one you'd want to choose.

I think the easiest fix is to just pass a scsi_data_buffer to
scsi_transfer_length(), and let the caller use scsi_in/scsi_out to find
the right one.

--
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
Christoph Hellwig June 25, 2014, 9:17 a.m. UTC | #3
> >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.

We could also pass the dma direction indeed.  Compared to my suggestion
of passing thr scsi_data_buffer this makes life a lot easier for drivers
that don't try to support the bidi madness.

> >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.

drivers/target/loopback/tcm_loop.c doesn't (and absolutely shouldn't!)
use scsi_transfer_length in target context, it uses it in it's initiator
side in the same way as iscsi, so the same semantics apply.

--
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
Sagi Grimberg June 25, 2014, 10:32 a.m. UTC | #4
On 6/25/2014 11:48 AM, Sagi Grimberg wrote:

<SNIP>
>
>> 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.
>

So I tested a bidirectional command using:
$ sg_raw --infile=/root/filex --send=1024 --request=1024 
--outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00

And I see:
kernel: session1: iscsi_prep_scsi_cmd_pdu iscsi prep [bidirectional cid 
0 sc ffff880468ca1e00 cdb 0x53 itt 0x16 len 1024 bidi_len 1024 cmdsn 223 
win 64]

This confirms what I wrote above, so AFAICT no additional fix is 
required for libiscsi wrt bidi commands support.

Mike?

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
Martin K. Petersen June 25, 2014, 11:29 a.m. UTC | #5
>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

Christoph> So here we use blk_rq_bytes still, which is incorrect for
Christoph> WRITE SAME.

Yeah, scsi_transfer_length() needs to go away completely if we go with
the in and out variants.

Christoph> I think the easiest fix is to just pass a scsi_data_buffer to
Christoph> scsi_transfer_length(), and let the caller use
Christoph> scsi_in/scsi_out to find the right one.

I'm perfectly OK with that approach.
Christoph Hellwig June 25, 2014, 11:35 a.m. UTC | #6
On Wed, Jun 25, 2014 at 01:32:39PM +0300, Sagi Grimberg wrote:
> So I tested a bidirectional command using:
> $ sg_raw --infile=/root/filex --send=1024 --request=1024
> --outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00
> 
> And I see:
> kernel: session1: iscsi_prep_scsi_cmd_pdu iscsi prep [bidirectional cid 0 sc
> ffff880468ca1e00 cdb 0x53 itt 0x16 len 1024 bidi_len 1024 cmdsn 223 win 64]
> 
> This confirms what I wrote above, so AFAICT no additional fix is required
> for libiscsi wrt bidi commands support.

Good to know.  I'd really prefer just going with the fix from Martin
that I have already queued up for 3.16, and then we can have the fully
fledged out "new" scsi_transfer_length() for 3.17.

--
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
Mike Christie June 25, 2014, 3:59 p.m. UTC | #7
On Jun 25, 2014, at 6:35 AM, Christoph Hellwig <hch@infradead.org> wrote:

> On Wed, Jun 25, 2014 at 01:32:39PM +0300, Sagi Grimberg wrote:
>> So I tested a bidirectional command using:
>> $ sg_raw --infile=/root/filex --send=1024 --request=1024
>> --outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00
>> 
>> And I see:
>> kernel: session1: iscsi_prep_scsi_cmd_pdu iscsi prep [bidirectional cid 0 sc
>> ffff880468ca1e00 cdb 0x53 itt 0x16 len 1024 bidi_len 1024 cmdsn 223 win 64]
>> 
>> This confirms what I wrote above, so AFAICT no additional fix is required
>> for libiscsi wrt bidi commands support.
> 
> Good to know.  I'd really prefer just going with the fix from Martin
> that I have already queued up for 3.16, and then we can have the fully
> fledged out "new" scsi_transfer_length() for 3.17.
> 

I am fine with this too.

I was way off track. I was more concerned with not wanting to use multiple functions/macros to get the transfer len. That should definitely be done when there is more time.
--
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
Boaz Harrosh July 27, 2014, 9:11 a.m. UTC | #8
On 06/25/2014 01:32 PM, Sagi Grimberg wrote:
> On 6/25/2014 11:48 AM, Sagi Grimberg wrote:
> 
> <SNIP>
>>
>>> 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.
>>
> 
> So I tested a bidirectional command using:
> $ sg_raw --infile=/root/filex --send=1024 --request=1024 
> --outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00
> 
> And I see:
> kernel: session1: iscsi_prep_scsi_cmd_pdu iscsi prep [bidirectional cid 
> 0 sc ffff880468ca1e00 cdb 0x53 itt 0x16 len 1024 bidi_len 1024 cmdsn 223 
> win 64]
> 

This is a very bad example and tested nothing, since len && bidi_len
are the same. So even if you had a bug and took length from the
wrong side it would come out the same.

You must test with a bidi command that has two different lengths for
each side

If you have a tree that you want me to test I will be glad too.
From this thread I'm confused as to what patches you want me to
test? please point me to a tree you need testing. You can bug me
any time, any tree. I will be happy to test.

Cheers
Boaz

> This confirms what I wrote above, so AFAICT no additional fix is 
> required for libiscsi wrt bidi commands support.
> 
> Mike?
> 
> Sagi.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
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
Christoph Hellwig July 27, 2014, 1:52 p.m. UTC | #9
On Sun, Jul 27, 2014 at 12:11:11PM +0300, Boaz Harrosh wrote:
> If you have a tree that you want me to test I will be glad too.
> >From this thread I'm confused as to what patches you want me to
> test? please point me to a tree you need testing. You can bug me
> any time, any tree. I will be happy to test.

You're about a month late to the game :)

I think everything is sorted out properly, but if you want to verify
it yourself just test the latest 3.16 release candidate from Linus.

--
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
Sagi Grimberg Aug. 6, 2014, 12:15 p.m. UTC | #10
On 7/27/2014 12:11 PM, Boaz Harrosh wrote:
> On 06/25/2014 01:32 PM, Sagi Grimberg wrote:
>> On 6/25/2014 11:48 AM, Sagi Grimberg wrote:
>>
>> <SNIP>
>>>
>>>> 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.
>>>
>>
>> So I tested a bidirectional command using:
>> $ sg_raw --infile=/root/filex --send=1024 --request=1024
>> --outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00
>>
>> And I see:
>> kernel: session1: iscsi_prep_scsi_cmd_pdu iscsi prep [bidirectional cid
>> 0 sc ffff880468ca1e00 cdb 0x53 itt 0x16 len 1024 bidi_len 1024 cmdsn 223
>> win 64]
>>
>
> This is a very bad example and tested nothing, since len && bidi_len
> are the same. So even if you had a bug and took length from the
> wrong side it would come out the same.
>
> You must test with a bidi command that has two different lengths for
> each side
>

Yes, I thought the same thing right after I sent this, so I tested it
with different lengths and it does work. I guess I was lazy in replying
it on top...

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
diff mbox

Patch

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));
 
 		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),
 			  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 */