[PATCH] IB/iser: extended CDB support

[PATCH] IB/iser: extended CDB support

[Vladimir Neyelov]

Support of vendor specific CDBs in iser. SCSI supports max command size
SCSI_MAX_VARLEN_CDB_SIZE (260 bytes). ISER currently supports max scsi
command 16 bytes. This commit changes max scsi command for ISER
(to align with iscsi/tcp) to SCSI_MAX_VARLEN_CDB_SIZE.

Signed-off-by: Vladimir Neyelov <vladimirn-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 17 +++++++++++++----
 drivers/infiniband/ulp/iser/iscsi_iser.h |  1 +
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 64b3d11..865ce48 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -163,7 +163,8 @@ iscsi_iser_pdu_alloc(struct iscsi_task *task, uint8_t opcode)
 	struct iscsi_iser_task *iser_task = task->dd_data;
 
 	task->hdr = (struct iscsi_hdr *)&iser_task->desc.iscsi_header;
-	task->hdr_max = sizeof(iser_task->desc.iscsi_header);
+	task->hdr_max = sizeof(iser_task->desc.iscsi_header) + 
+			sizeof(iser_task->desc.iscsi_ecdb_header);
 
 	return 0;
 }
@@ -189,6 +190,14 @@ iser_initialize_task_headers(struct iscsi_task *task,
 	u64 dma_addr;
 	const bool mgmt_task = !task->sc && !in_interrupt();
 	int ret = 0;
+	int headers_len = ISER_HEADERS_LEN;
+	struct scsi_cmnd *sc = task->sc;
+
+	if(sc) {
+		if(sc->cmd_len > ISCSI_CDB_SIZE)
+			headers_len += offsetof(struct iscsi_ecdb_ahdr, ecdb) + 
+					(sc->cmd_len - ISCSI_CDB_SIZE);
+	}
 
 	if (unlikely(mgmt_task))
 		mutex_lock(&iser_conn->state_mutex);
@@ -199,7 +208,7 @@ iser_initialize_task_headers(struct iscsi_task *task,
 	}
 
 	dma_addr = ib_dma_map_single(device->ib_device, (void *)tx_desc,
-				ISER_HEADERS_LEN, DMA_TO_DEVICE);
+				headers_len, DMA_TO_DEVICE );
 	if (ib_dma_mapping_error(device->ib_device, dma_addr)) {
 		ret = -ENOMEM;
 		goto out;
@@ -209,7 +218,7 @@ iser_initialize_task_headers(struct iscsi_task *task,
 	tx_desc->mapped = true;
 	tx_desc->dma_addr = dma_addr;
 	tx_desc->tx_sg[0].addr   = tx_desc->dma_addr;
-	tx_desc->tx_sg[0].length = ISER_HEADERS_LEN;
+	tx_desc->tx_sg[0].length = headers_len;
 	tx_desc->tx_sg[0].lkey   = device->pd->local_dma_lkey;
 
 	iser_task->iser_conn = iser_conn;
@@ -623,7 +632,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
 	shost->max_lun = iscsi_max_lun;
 	shost->max_id = 0;
 	shost->max_channel = 0;
-	shost->max_cmd_len = 16;
+	shost->max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE;
 
 	/*
 	 * older userspace tools (before 2.0-870) did not pass us
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
index 0be6a7c..e1ae9b8 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -254,6 +254,7 @@ enum iser_desc_type {
 struct iser_tx_desc {
 	struct iser_ctrl             iser_header;
 	struct iscsi_hdr             iscsi_header;
+	struct iscsi_ecdb_ahdr       iscsi_ecdb_header;
 	enum   iser_desc_type        type;
 	u64		             dma_addr;
 	struct ib_sge		     tx_sg[2];
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: extended CDB support

[Max Gurtovoy]

Sagi/Christopth,

any thoughts on this onw ?

On 11/23/2016 10:52 AM, Vladimir Neyelov wrote:
> Support of vendor specific CDBs in iser. SCSI supports max command size
> SCSI_MAX_VARLEN_CDB_SIZE (260 bytes). ISER currently supports max scsi
> command 16 bytes. This commit changes max scsi command for ISER
> (to align with iscsi/tcp) to SCSI_MAX_VARLEN_CDB_SIZE.
>
> Signed-off-by: Vladimir Neyelov <vladimirn-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/infiniband/ulp/iser/iscsi_iser.c | 17 +++++++++++++----
>  drivers/infiniband/ulp/iser/iscsi_iser.h |  1 +
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
> index 64b3d11..865ce48 100644
> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
> @@ -163,7 +163,8 @@ iscsi_iser_pdu_alloc(struct iscsi_task *task, uint8_t opcode)
>  	struct iscsi_iser_task *iser_task = task->dd_data;
>
>  	task->hdr = (struct iscsi_hdr *)&iser_task->desc.iscsi_header;
> -	task->hdr_max = sizeof(iser_task->desc.iscsi_header);
> +	task->hdr_max = sizeof(iser_task->desc.iscsi_header) +
> +			sizeof(iser_task->desc.iscsi_ecdb_header);
>
>  	return 0;
>  }
> @@ -189,6 +190,14 @@ iser_initialize_task_headers(struct iscsi_task *task,
>  	u64 dma_addr;
>  	const bool mgmt_task = !task->sc && !in_interrupt();
>  	int ret = 0;
> +	int headers_len = ISER_HEADERS_LEN;
> +	struct scsi_cmnd *sc = task->sc;
> +
> +	if(sc) {
> +		if(sc->cmd_len > ISCSI_CDB_SIZE)
> +			headers_len += offsetof(struct iscsi_ecdb_ahdr, ecdb) +
> +					(sc->cmd_len - ISCSI_CDB_SIZE);
> +	}
>
>  	if (unlikely(mgmt_task))
>  		mutex_lock(&iser_conn->state_mutex);
> @@ -199,7 +208,7 @@ iser_initialize_task_headers(struct iscsi_task *task,
>  	}
>
>  	dma_addr = ib_dma_map_single(device->ib_device, (void *)tx_desc,
> -				ISER_HEADERS_LEN, DMA_TO_DEVICE);
> +				headers_len, DMA_TO_DEVICE );
>  	if (ib_dma_mapping_error(device->ib_device, dma_addr)) {
>  		ret = -ENOMEM;
>  		goto out;
> @@ -209,7 +218,7 @@ iser_initialize_task_headers(struct iscsi_task *task,
>  	tx_desc->mapped = true;
>  	tx_desc->dma_addr = dma_addr;
>  	tx_desc->tx_sg[0].addr   = tx_desc->dma_addr;
> -	tx_desc->tx_sg[0].length = ISER_HEADERS_LEN;
> +	tx_desc->tx_sg[0].length = headers_len;
>  	tx_desc->tx_sg[0].lkey   = device->pd->local_dma_lkey;
>
>  	iser_task->iser_conn = iser_conn;
> @@ -623,7 +632,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
>  	shost->max_lun = iscsi_max_lun;
>  	shost->max_id = 0;
>  	shost->max_channel = 0;
> -	shost->max_cmd_len = 16;
> +	shost->max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE;
>
>  	/*
>  	 * older userspace tools (before 2.0-870) did not pass us
> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
> index 0be6a7c..e1ae9b8 100644
> --- a/drivers/infiniband/ulp/iser/iscsi_iser.h
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
> @@ -254,6 +254,7 @@ enum iser_desc_type {
>  struct iser_tx_desc {
>  	struct iser_ctrl             iser_header;
>  	struct iscsi_hdr             iscsi_header;
> +	struct iscsi_ecdb_ahdr       iscsi_ecdb_header;
>  	enum   iser_desc_type        type;
>  	u64		             dma_addr;
>  	struct ib_sge		     tx_sg[2];
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: extended CDB support

[Christoph Hellwig]

On Tue, Nov 29, 2016 at 09:43:12AM +0200, Max Gurtovoy wrote:
> Sagi/Christopth,
> 
> any thoughts on this onw ?
> 
> On 11/23/2016 10:52 AM, Vladimir Neyelov wrote:
> > Support of vendor specific CDBs in iser.

This command surely is wrong.

Except for that: what's the use case?  The only use case for gigantic
CDBs in iSCSI was the T10 OSD code, which hopefully is on it's way out.

Or do you just need 32 byte CDBs, in whіch case things could probably
be done much simpler.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: extended CDB support

[Max Gurtovoy]

On 11/29/2016 10:02 AM, Christoph Hellwig wrote:
> On Tue, Nov 29, 2016 at 09:43:12AM +0200, Max Gurtovoy wrote:
>> Sagi/Christopth,
>>
>> any thoughts on this onw ?
>>
>> On 11/23/2016 10:52 AM, Vladimir Neyelov wrote:
>>> Support of vendor specific CDBs in iser.
>
> This command surely is wrong.
>
> Except for that: what's the use case?  The only use case for gigantic
> CDBs in iSCSI was the T10 OSD code, which hopefully is on it's way out.
>
> Or do you just need 32 byte CDBs, in whіch case things could probably
> be done much simpler.
>

we needed to support 22 byte CDB (in the case we encountered the 
problem) but we wanted to be aligned with the tcp implementation that 
support SCSI_MAX_VARLEN_CDB_SIZE.
notice that on the wire we send only the actual CDB size and not all the 
SCSI_MAX_VARLEN_CDB_SIZE.
I guess you're concern about allocating extra 244B for each task, right ?
AFAIK we didn't see difference in perf with this patch but it worth to 
re-check it and share numbers with the community.

regarding the simplicity, I guess the support for 32B CDB will be pretty 
much similar to this commit, unless you see some simpler solution.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html