Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
> "Sagi" == Sagi Grimberg writes: Sagi> In case protection information exists on the wire scsi transports Sagi> should include it in the transfer byte count (even if protection Sagi> information does not exist in the host memory space). This helper Sagi> will compute the total transfer length from the scsi command data Sagi> length and protection attributes. Looks good! -- Martin K. Petersen Oracle Linux Engineering -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 06/11/2014 04:09 AM, Sagi Grimberg wrote:
> In case protection information exists on the wire
> scsi transports should include it in the transfer
> byte count (even if protection information does not
> exist in the host memory space). This helper will
> compute the total transfer length from the scsi
> command data length and protection attributes.
>
> Signed-off-by: Sagi Grimberg
> Signed-off-by: Martin K. Petersen
> ---
> include/scsi/scsi_cmnd.h | 17 +
> 1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index dd7c998..a100c6e 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -7,6 +7,7 @@
> #include
> #include
> #include
> +#include
>
> struct Scsi_Host;
> struct scsi_device;
> @@ -306,4 +307,20 @@ static inline void set_driver_byte(struct scsi_cmnd
> *cmd, char status)
> cmd->result = (cmd->result & 0x00ff) | (status << 24);
> }
>
> +static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
> +{
> + unsigned int xfer_len = blk_rq_bytes(scmd->request);
Can you do bidi and dif/dix? If so, then instead of using blk_rq_bytes
directly should it use the scsi_out/scsi_in macros and access the length
through the scsi_data_buffer?
This does not fix Christoph's bug in the other mail. Just noticed it
while looking at the code.
> + unsigned int prot_op = scsi_get_prot_op(scmd);
> + unsigned int sector_size = scmd->device->sector_size;
> +
> + switch (prot_op) {
> + case SCSI_PROT_NORMAL:
> + case SCSI_PROT_WRITE_STRIP:
> + case SCSI_PROT_READ_INSERT:
> + return xfer_len;
> + }
> +
> + return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;
> +}
> +
> #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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
> "Mike" == Mike Christie writes: >> + unsigned int xfer_len = blk_rq_bytes(scmd->request); Mike> Can you do bidi and dif/dix? Nope. -- Martin K. Petersen Oracle Linux Engineering -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 06/11/2014 04:09 AM, Sagi Grimberg wrote:
> In case protection information exists on the wire
> scsi transports should include it in the transfer
> byte count (even if protection information does not
> exist in the host memory space). This helper will
> compute the total transfer length from the scsi
> command data length and protection attributes.
>
> Signed-off-by: Sagi Grimberg
> Signed-off-by: Martin K. Petersen
> ---
> include/scsi/scsi_cmnd.h | 17 +
> 1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index dd7c998..a100c6e 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -7,6 +7,7 @@
> #include
> #include
> #include
> +#include
>
> struct Scsi_Host;
> struct scsi_device;
> @@ -306,4 +307,20 @@ static inline void set_driver_byte(struct scsi_cmnd
> *cmd, char status)
> cmd->result = (cmd->result & 0x00ff) | (status << 24);
> }
>
> +static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
> +{
> + 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;
> +
> + switch (prot_op) {
> + case SCSI_PROT_NORMAL:
> + case SCSI_PROT_WRITE_STRIP:
> + case SCSI_PROT_READ_INSERT:
> + return xfer_len;
> + }
> +
> + return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;
> +}
> +
> #endif /* _SCSI_SCSI_CMND_H */
>
I found the issue Christoph is hitting in the other thread.
The problem is WRITE_SAME requests are setup so that req->__data_len is
the value of the entire request when the setup is completed but during
the setup process it's value changes
So __data_len could be thousands of bytes but
scsi_out(scsi_cmnd)->length for this case was only returning 512 which
is the sector size. This is because sd_setup_-write_same_cmnd does:
rq->__data_len = sdp->sector_size;
scsi_setup_blk_pc_cmnd()
rq->__data_len = nr_bytes;
and scsi_setup_blk_pc_cmnd does scsi_init_io() -> scsi_init_sgtable()
and that does
sdb->length = blk_rq_bytes(req);
and at this time because before we called scsi_setup_blk_pc_cmnd we set
the __data_len to sector size, the sdb length is going to be only 512
but the final request->__data_len is the total size of the operation.
--
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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
> "Mike" == Mike Christie writes:
Mike> The problem is WRITE_SAME requests are setup so that
Mike> req->__data_len is the value of the entire request when the setup
Mike> is completed but during the setup process it's value changes
Oh, I see. So things break because iSCSI uses scsi_transfer_length()
where the scatterlist length was used in the past.
How about this?
SCSI: Use SCSI data buffer length to extract transfer size
Commit 8846bab180fa introduced a helper that can be used to query the
wire transfer size for a SCSI command taking protection information into
account.
However, some commands do not have a 1:1 mapping between the block range
they work on and the payload size (discard, write same). After the
scatterlist has been set up these requests use __data_len to store the
number of bytes to report completion on. This means that callers of
scsi_transfer_length() would get the wrong byte count for these types of
requests.
To overcome this we make scsi_transfer_length() use the scatterlist
length in the scsi_data_buffer as basis for the wire transfer
calculation instead of __data_len.
Reported-by: Christoph Hellwig
Debugged-by: Mike Christie
Signed-off-by: Martin K. Petersen
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 42ed789ebafc..e0ae71098144 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -318,7 +318,7 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd,
char status)
static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
{
- unsigned int xfer_len = blk_rq_bytes(scmd->request);
+ unsigned int xfer_len = scsi_out(scmd)->length;
unsigned int prot_op = scsi_get_prot_op(scmd);
unsigned int sector_size = scmd->device->sector_size;
--
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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 6/24/2014 9:54 AM, Mike Christie wrote:
On 06/11/2014 04:09 AM, Sagi Grimberg wrote:
In case protection information exists on the wire
scsi transports should include it in the transfer
byte count (even if protection information does not
exist in the host memory space). This helper will
compute the total transfer length from the scsi
command data length and protection attributes.
Signed-off-by: Sagi Grimberg
Signed-off-by: Martin K. Petersen
---
include/scsi/scsi_cmnd.h | 17 +
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index dd7c998..a100c6e 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -7,6 +7,7 @@
#include
#include
#include
+#include
struct Scsi_Host;
struct scsi_device;
@@ -306,4 +307,20 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd,
char status)
cmd->result = (cmd->result & 0x00ff) | (status << 24);
}
+static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
+{
+ 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;
+
+ switch (prot_op) {
+ case SCSI_PROT_NORMAL:
+ case SCSI_PROT_WRITE_STRIP:
+ case SCSI_PROT_READ_INSERT:
+ return xfer_len;
+ }
+
+ return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;
+}
+
#endif /* _SCSI_SCSI_CMND_H */
I found the issue Christoph is hitting in the other thread.
The problem is WRITE_SAME requests are setup so that req->__data_len is
the value of the entire request when the setup is completed but during
the setup process it's value changes
So __data_len could be thousands of bytes but
scsi_out(scsi_cmnd)->length for this case was only returning 512 which
is the sector size. This is because sd_setup_-write_same_cmnd does:
rq->__data_len = sdp->sector_size;
scsi_setup_blk_pc_cmnd()
rq->__data_len = nr_bytes;
and scsi_setup_blk_pc_cmnd does scsi_init_io() -> scsi_init_sgtable()
and that does
sdb->length = blk_rq_bytes(req);
and at this time because before we called scsi_setup_blk_pc_cmnd we set
the __data_len to sector size, the sdb length is going to be only 512
but the final request->__data_len is the total size of the operation.
Hey Christoph, Mike &MKP,
So just got first look in this thread, didn't have time to reproduce it yet.
Mike's analysis makes sense to me, I think the below change should
resolve this one.
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a100c6e..2afd9c2 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -309,7 +309,7 @@ static inline void set_driver_byte(struct scsi_cmnd
*cmd, char status)
static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
{
- unsigned int xfer_len = blk_rq_bytes(scmd->request);
+ unsigned int xfer_len = scsi_bufflen(scmd);
unsigned int prot_op = scsi_get_prot_op(scmd);
unsigned int sector_size = scmd->device->sector_size;
Moreover, since bidi and dif are adjacent, this will also needed:
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 3f46234..abf0c3e 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -386,12 +386,14 @@ static int iscsi_prep_scsi_cmd_pdu(struct
iscsi_task *task)
rc = iscsi_prep_bidi_ahs(task);
if (rc)
return rc;
+ transfer_length = scsi_in(sc)->length;
+ } else {
+ transfer_length = scsi_transfer_length(sc);
}
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) {
struct iscsi_r2t_info *r2t = &task->unsol_r2t;
Let me test and queue it up.
Sagi.
--
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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 6/24/2014 3:53 PM, Martin K. Petersen wrote: "Mike" == Mike Christie writes: Mike> The problem is WRITE_SAME requests are setup so that Mike> req->__data_len is the value of the entire request when the setup Mike> is completed but during the setup process it's value changes Oh, I see. So things break because iSCSI uses scsi_transfer_length() where the scatterlist length was used in the past. Ohhh, didn't notice this one and sent out a duplicate... The patch looks good to me obviously. Sagi. -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On Tue, Jun 24, 2014 at 08:53:25AM -0400, Martin K. Petersen wrote: > Oh, I see. So things break because iSCSI uses scsi_transfer_length() > where the scatterlist length was used in the past. > > How about this? This fixes the regression for me. -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On Tue, Jun 24, 2014 at 04:08:25PM +0300, Sagi Grimberg wrote: > >Oh, I see. So things break because iSCSI uses scsi_transfer_length() > >where the scatterlist length was used in the past. > > Ohhh, didn't notice this one and sent out a duplicate... > > The patch looks good to me obviously. Can you give me a real reviewed-by? > > Sagi. ---end quoted text--- -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 6/24/2014 3:53 PM, Martin K. Petersen wrote:
SCSI: Use SCSI data buffer length to extract transfer size
Commit 8846bab180fa introduced a helper that can be used to query the
wire transfer size for a SCSI command taking protection information into
account.
However, some commands do not have a 1:1 mapping between the block range
they work on and the payload size (discard, write same). After the
scatterlist has been set up these requests use __data_len to store the
number of bytes to report completion on. This means that callers of
scsi_transfer_length() would get the wrong byte count for these types of
requests.
To overcome this we make scsi_transfer_length() use the scatterlist
length in the scsi_data_buffer as basis for the wire transfer
calculation instead of __data_len.
Reported-by: Christoph Hellwig
Debugged-by: Mike Christie
Signed-off-by: Martin K. Petersen
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 42ed789ebafc..e0ae71098144 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -318,7 +318,7 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd,
char status)
static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
{
- unsigned int xfer_len = blk_rq_bytes(scmd->request);
+ unsigned int xfer_len = scsi_out(scmd)->length;
unsigned int prot_op = scsi_get_prot_op(scmd);
unsigned int sector_size = scmd->device->sector_size;
Reviewed-by: Sagi Grimberg
--
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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On Jun 24, 2014, at 7:53 AM, Martin K. Petersen
wrote:
>> "Mike" == Mike Christie writes:
>
> Mike> The problem is WRITE_SAME requests are setup so that
> Mike> req->__data_len is the value of the entire request when the setup
> Mike> is completed but during the setup process it's value changes
>
> Oh, I see. So things break because iSCSI uses scsi_transfer_length()
> where the scatterlist length was used in the past.
>
> How about this?
>
>
> SCSI: Use SCSI data buffer length to extract transfer size
>
> Commit 8846bab180fa introduced a helper that can be used to query the
> wire transfer size for a SCSI command taking protection information into
> account.
>
> However, some commands do not have a 1:1 mapping between the block range
> they work on and the payload size (discard, write same). After the
> scatterlist has been set up these requests use __data_len to store the
> number of bytes to report completion on. This means that callers of
> scsi_transfer_length() would get the wrong byte count for these types of
> requests.
>
> To overcome this we make scsi_transfer_length() use the scatterlist
> length in the scsi_data_buffer as basis for the wire transfer
> calculation instead of __data_len.
>
> Reported-by: Christoph Hellwig
> Debugged-by: Mike Christie
> Signed-off-by: Martin K. Petersen
>
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 42ed789ebafc..e0ae71098144 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -318,7 +318,7 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd,
> char status)
>
> static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
> {
> - unsigned int xfer_len = blk_rq_bytes(scmd->request);
> + unsigned int xfer_len = scsi_out(scmd)->length;
> unsigned int prot_op = scsi_get_prot_op(scmd);
> unsigned int sector_size = scmd->device->sector_size;
Do we need to check for the data direction. Something like
if (scmd->sc_data_direction == DMA_TO_DEVICE)
xfer_len = scsi_out(scmnd)->length;
else
xfer_len = scsi_in(scmnd)->length;
--
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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On Tue, Jun 24, 2014 at 11:08:54AM -0500, Michael Christie wrote:
> > static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
> > {
> > - unsigned int xfer_len = blk_rq_bytes(scmd->request);
> > + unsigned int xfer_len = scsi_out(scmd)->length;
> > unsigned int prot_op = scsi_get_prot_op(scmd);
> > unsigned int sector_size = scmd->device->sector_size;
>
>
> Do we need to check for the data direction. Something like
>
> if (scmd->sc_data_direction == DMA_TO_DEVICE)
> xfer_len = scsi_out(scmnd)->length;
> else
> xfer_len = scsi_in(scmnd)->length;
For non-bidi commands those are the same, but I suspect we'd need
something like that for bidi commands.
--
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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 6/24/2014 7:08 PM, Michael Christie wrote:
On Jun 24, 2014, at 7:53 AM, Martin K. Petersen
wrote:
"Mike" == Mike Christie writes:
Mike> The problem is WRITE_SAME requests are setup so that
Mike> req->__data_len is the value of the entire request when the setup
Mike> is completed but during the setup process it's value changes
Oh, I see. So things break because iSCSI uses scsi_transfer_length()
where the scatterlist length was used in the past.
How about this?
SCSI: Use SCSI data buffer length to extract transfer size
Commit 8846bab180fa introduced a helper that can be used to query the
wire transfer size for a SCSI command taking protection information into
account.
However, some commands do not have a 1:1 mapping between the block range
they work on and the payload size (discard, write same). After the
scatterlist has been set up these requests use __data_len to store the
number of bytes to report completion on. This means that callers of
scsi_transfer_length() would get the wrong byte count for these types of
requests.
To overcome this we make scsi_transfer_length() use the scatterlist
length in the scsi_data_buffer as basis for the wire transfer
calculation instead of __data_len.
Reported-by: Christoph Hellwig
Debugged-by: Mike Christie
Signed-off-by: Martin K. Petersen
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 42ed789ebafc..e0ae71098144 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -318,7 +318,7 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd,
char status)
static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
{
- unsigned int xfer_len = blk_rq_bytes(scmd->request);
+ unsigned int xfer_len = scsi_out(scmd)->length;
unsigned int prot_op = scsi_get_prot_op(scmd);
unsigned int sector_size = scmd->device->sector_size;
Do we need to check for the data direction. Something like
if (scmd->sc_data_direction == DMA_TO_DEVICE)
xfer_len = scsi_out(scmnd)->length;
else
xfer_len = scsi_in(scmnd)->length;
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.
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 3f46234..abf0c3e 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -386,12 +386,14 @@ static int iscsi_prep_scsi_cmd_pdu(struct
iscsi_task *task)
rc = iscsi_prep_bidi_ahs(task);
if (rc)
return rc;
+ transfer_length = scsi_in(sc)->length;
+ } else {
+ transfer_length = scsi_transfer_length(sc);
}
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) {
struct iscsi_r2t_info *r2t = &task->unsol_r2t;
Sagi.
--
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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
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. -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
> "Mike" == Michael Christie writes: Mike> Do we need to check for the data direction. Something like Mike> if (scmd->sc_data_direction == DMA_TO_DEVICE) Mike> xfer_len = scsi_out(scmnd)->length; Mike> else Mike> xfer_len = scsi_in(scmnd)->length; I guess that depends on the context the wrapper is called in. I think iscsi is the only place where there's a distinction thanks to bidi. Looks like there are several places where that's done. In that case I wonder if we should have explicit scsi_in_transfer_length() and scsi_out_transfer_length() wrappers? -- Martin K. Petersen Oracle Linux Engineering -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
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 -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
> "Mike" == Mike Christie writes: Mike> It would be nice to just have one function to call and it just do Mike> the right thing for the drivers. But what is the right thing when there are buffers for both directions? -- Martin K. Petersen Oracle Linux Engineering -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 06/24/2014 11:31 AM, Martin K. Petersen wrote: >> "Mike" == Michael Christie writes: > > Mike> Do we need to check for the data direction. Something like > > Mike> if (scmd->sc_data_direction == DMA_TO_DEVICE) > Mike> xfer_len = scsi_out(scmnd)->length; > Mike> else > Mike> xfer_len = scsi_in(scmnd)->length; > > I guess that depends on the context the wrapper is called in. I think > iscsi is the only place where there's a distinction thanks to bidi. > We were using it generically, so we did not check if bidi or t10 pi. We were calling it just expecting it to do the right thing for us. > Looks like there are several places where that's done. In that case I > wonder if we should have explicit scsi_in_transfer_length() and > scsi_out_transfer_length() wrappers? Yeah, maybe. -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
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 Actually, let me take this back for a second. I am not sure if that is right. -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
Martin K. Petersen, on 06/23/2014 06:58 PM wrote: "Mike" == Mike Christie writes: + unsigned int xfer_len = blk_rq_bytes(scmd->request); Mike> Can you do bidi and dif/dix? Nope. Correction: at the moment. There is a proposal of READ GATHERED command, which is bidirectional and potentially DIF/DIX. Vlad -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
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
>
> 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
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_FLA
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
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
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
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 =
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
> >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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 6/25/2014 11:48 AM, Sagi Grimberg wrote: 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 880468ca1e00 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
> "Christoph" == Christoph Hellwig 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. -- Martin K. Petersen Oracle Linux Engineering -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
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 > 880468ca1e00 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On Jun 25, 2014, at 6:35 AM, Christoph Hellwig 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 >> 880468ca1e00 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 06/11/14 11:09, Sagi Grimberg wrote: > + return xfer_len + (xfer_len >> ilog2(sector_size)) * 8; Sorry that I just noticed this now, but why is a shift-right and ilog2() used in the above expression instead of just dividing the transfer length by the sector size ? Thanks, Bart. -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote: > On 06/11/14 11:09, Sagi Grimberg wrote: > > + return xfer_len + (xfer_len >> ilog2(sector_size)) * 8; > > Sorry that I just noticed this now, but why is a shift-right and ilog2() > used in the above expression instead of just dividing the transfer > length by the sector size ? It's a performance thing. Division is really slow on most CPUs. However, we know the divisor is a power of two so we re-express the division as a shift, which the processor can do really fast. James -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On Jun 26, 2014, at 10:55 AM, James Bottomley wrote: > On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote: >> On 06/11/14 11:09, Sagi Grimberg wrote: >>> + return xfer_len + (xfer_len >> ilog2(sector_size)) * 8; >> >> Sorry that I just noticed this now, but why is a shift-right and ilog2() >> used in the above expression instead of just dividing the transfer >> length by the sector size ? > > It's a performance thing. Division is really slow on most CPUs. > However, we know the divisor is a power of two so we re-express the > division as a shift, which the processor can do really fast. > > James I have done this in the past as well, but have you benchmarked it? Compilers typically do the right thing in this case (i.e replace division with shift). Scott-- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On June 26, 2014 11:41:48 AM EDT, "Atchley, Scott" wrote: >On Jun 26, 2014, at 10:55 AM, James Bottomley > wrote: > >> On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote: >>> On 06/11/14 11:09, Sagi Grimberg wrote: + return xfer_len + (xfer_len >> ilog2(sector_size)) * 8; >>> >>> Sorry that I just noticed this now, but why is a shift-right and >ilog2() >>> used in the above expression instead of just dividing the transfer >>> length by the sector size ? >> >> It's a performance thing. Division is really slow on most CPUs. >> However, we know the divisor is a power of two so we re-express the >> division as a shift, which the processor can do really fast. >> >> James > >I have done this in the past as well, but have you benchmarked it? >Compilers typically do the right thing in this case (i.e replace >division with shift). The compiler can only do that for values which are reducible to constants at compile time. This is a runtime value, the compiler has no way of deducing that it will be a power of 2 James -- Sent from my Android device with K-9 Mail. Please excuse my brevity. -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On Jun 26, 2014, at 12:38 PM, James Bottomley wrote: > On June 26, 2014 11:41:48 AM EDT, "Atchley, Scott" wrote: >> On Jun 26, 2014, at 10:55 AM, James Bottomley >> wrote: >> >>> On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote: On 06/11/14 11:09, Sagi Grimberg wrote: > + return xfer_len + (xfer_len >> ilog2(sector_size)) * 8; Sorry that I just noticed this now, but why is a shift-right and >> ilog2() used in the above expression instead of just dividing the transfer length by the sector size ? >>> >>> It's a performance thing. Division is really slow on most CPUs. >>> However, we know the divisor is a power of two so we re-express the >>> division as a shift, which the processor can do really fast. >>> >>> James >> >> I have done this in the past as well, but have you benchmarked it? >> Compilers typically do the right thing in this case (i.e replace >> division with shift). > > The compiler can only do that for values which are reducible to constants at > compile time. This is a runtime value, the compiler has no way of deducing > that it will be a power of 2 > > James You're right, I should have said runtime. However, gcc on Intel seems to choose the right algorithm at runtime. On a trivial app with -O0, I see the same performance for shift and division if the divisor is a power of two. Is see ~38% penalty if the divisor is not a power of 2. With -O3, shift is faster than division by about ~17% when the divisor is a power of two. Scott-- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
Sagi, can you send an update for the core-for-3.17 tree to pass a dma_direction or the scsi data buffer to scsi_transfer_length so we can make safe for bidi usage in the future? -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
> "Christoph" == Christoph Hellwig writes: Christoph> Sagi, can you send an update for the core-for-3.17 tree to Christoph> pass a dma_direction or the scsi data buffer to Christoph> scsi_transfer_length so we can make safe for bidi usage in Christoph> the future? I have cleaned this up in my integrity patch set since we already have the flag to determine whether to transfer PI or not. I'll get those patches out today. -- Martin K. Petersen Oracle Linux Engineering -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 06/25/2014 04:17 AM, Vladislav Bolkhovitin wrote: > Martin K. Petersen, on 06/23/2014 06:58 PM wrote: >>> "Mike" == Mike Christie writes: + unsigned int xfer_len = blk_rq_bytes(scmd->request); >> >> Mike> Can you do bidi and dif/dix? >> >> Nope. > > Correction: at the moment. > > There is a proposal of READ GATHERED command, which is bidirectional and > potentially > DIF/DIX. > That's all very good. But current infrastructure can not support BIDI+DIFF. Because you we'll need *two* diff vectors one for each side. Now in fact at the block layer this is actually supported. Since BIDI is two requests, and each can have its own DIFF bio. But on the scsi layer this is not implemented. So I'd say: *For now DIFF and BIDI are mutually exclusive.* (You'll need someone to love these new commands a lot in order to enable it) > Vlad > Cheers Boaz -- 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
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 06/25/2014 01:32 PM, Sagi Grimberg wrote: > On 6/25/2014 11:48 AM, Sagi Grimberg wrote: > > >> >>> 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 880468ca1e00 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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
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: 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 880468ca1e00 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
