Hello Arun, Roland & Co,
Thanks for your feedback. Apologies for delayed response on this bug.
On Wed, 2015-07-08 at 13:25 -0700, Arun Easi wrote:
> Hi Nick,
>
> On Mon, 6 Jul 2015, 9:08pm -0700, Nicholas A. Bellinger wrote:
>
> > (Adding Giri + Andrew CC')
> >
> > On Wed, 2015-07-01 at 15:50 -0700, Roland Dreier wrote:
> >> On Wed, Jul 1, 2015 at 3:41 PM, Craig Watson
> >> <craig.watson@xxxxxxxxxxxxxxxxxxx> wrote:
> >>> So, is it really a limitation in the hardware? .. or is it in the qla2xxx
> >>> driver? Does the qla2xxx driver not support chained scatter/gather lists?
> >>>
> >>> Since the removal of fabric_max_sectors et.al. how is this size limit
> >>> communicated to an initiator? From what I understood, the target front end
> >>> is going to reflect the limit of the back end storage. Right now what I
> >>> (and our customers) see is just a refusal to operate at large (> 4.9MB)
> >>> transfer sizes. At some write sizes there aren't even any error indications
> >>> on the target. The target just stops responding. This isn't a good thing.
> >>> It's better than crashing but not by much. Other sizes do seem to crash the
> >>> kernel which really isn't a good thing.
> >>
> >> The hardware really has the limitation that a single CTIO ("continue
> >> target IO") control block can only have 1200-something gather-scatter
> >> entries (which matches well with your 4.9 MB limit, given that each
> >> entry will typically be a 4KB page). However the fact that that
> >> limits the size of a SCSI command is of course in driver code. LIO
> >> can use multiple CTIOs to respond to a single SCSI command. It
> >> doesn't at the moment though.
> >>
> >> There is no way this is communicated to the initiator that I know of
> >> at the moment. I did not understand the "remove IO size limit"
> >> commits when they went in, but they definitely leave the target stack
> >> in a broken state WRT this type of issue.
> >>
> >
> > Ok, so qla2xxx really does need some manner of max_sectors enforced by
> > target-core due to it's hw sgl limits, but I think the original way of
> > using a backend attribute hard-coded across all LUNs was wrong.
>
> I agree with the general idea here (at least short term).
>
> >
> > How about a fabric driver provided field like the following instead..?
> >
> > diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> > index 33364b6..ce4e84f 100644
> > --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> > +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> > @@ -1814,6 +1814,7 @@ static const struct target_core_fabric_ops tcm_qla2xxx_ops = {
> > .module = THIS_MODULE,
> > .name = "qla2xxx",
> > .node_acl_size = sizeof(struct tcm_qla2xxx_nacl),
> > + .fabric_hw_max_sectors = 8192,
>
> As the limit is in number of SG entries, my preference would be to just
> say that, instead of in sectors terms, something like:
> .max_sg_entries = XX,
>
> Stack could internally convert that to max-sectors using page-size and
> sector-size calculation and report in EVPD-b0 and use for enforcement.
>
> Longer term (Roland alluded to this, I think), it would be best to have
> stack call .queue_data_in/.write_pending with a max of "max_sg_entries" SG
> entries at one time and repeat until the whole data is transferred; thus
> achieving a possible unlimited data transfer length.
>
Here's a better work-around following your earlier comments to enforce
target-core level se_cmd->t_data_sg fabric driver limits, currently
assuming 1:1 PAGE_SIZE mapping per internally allocated scatterlist
entry.
The max_data_sg_nents=1200 default for tcm_qla2xxx is following Roland's
earlier comments.
Please review.
>From 272f87488fc8681eedb39937cba0b39fb59c0bba Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
Date: Mon, 6 Jul 2015 20:47:55 -0700
Subject: [PATCH] target: Enforce fabric max_data_sg_nents I/O payload limit
This patch changes target-core to enforce optional fabric driver
provided max_data_sg_nents limit, using target_alloc_sgl() level
checking against existing single PAGE_SIZE per scatterlist entry.
If se_cmd payload length meets or exceeds the provided value,
this patch returns SAM_STAT_CHECK_CONDITION + ILLEGAL_REQUEST
xception status.
This is currently required to restrict scatter-gather-list entries
in per se_cmd->t_data_sg descriptor payload for tcm_qla2xxx.
It also updates associated target_alloc_sgl() callers, and
transport_generic_new_cmd() to handle proper sense_reason_t for
TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE + TCM_SECTOR_COUNT_TOO_MANY.
Reported-by: Craig Watson <craig.watson@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
---
drivers/scsi/qla2xxx/tcm_qla2xxx.c | 5 ++++
drivers/target/target_core_spc.c | 13 +++++++---
drivers/target/target_core_transport.c | 45 ++++++++++++++++++++--------------
drivers/target/target_core_xcopy.c | 2 +-
include/target/target_core_backend.h | 2 +-
include/target/target_core_fabric.h | 9 +++++++
6 files changed, 53 insertions(+), 23 deletions(-)
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 9224a06..6649809 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1804,6 +1804,11 @@ static const struct target_core_fabric_ops tcm_qla2xxx_ops = {
.module = THIS_MODULE,
.name = "qla2xxx",
.node_acl_size = sizeof(struct tcm_qla2xxx_nacl),
+ /*
+ * XXX: Limit assumes single page per scatter-gather-list entry.
+ * Current maximum is ~4.9 MB per se_cmd->t_data_sg with PAGE_SIZE=4096
+ */
+ .max_data_sg_nents = 1200,
.get_fabric_name = tcm_qla2xxx_get_fabric_name,
.tpg_get_wwn = tcm_qla2xxx_get_fabric_wwn,
.tpg_get_tag = tcm_qla2xxx_get_tag,
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index b5ba1ec..dd5249f 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -484,8 +484,8 @@ static sense_reason_t
spc_emulate_evpd_b0(struct se_cmd *cmd, unsigned char *buf)
{
struct se_device *dev = cmd->se_dev;
- int have_tp = 0;
- int opt, min;
+ u32 mtl = 0;
+ int have_tp = 0, opt, min;
/*
* Following spc3r22 section 6.5.3 Block Limits VPD page, when
@@ -516,8 +516,15 @@ spc_emulate_evpd_b0(struct se_cmd *cmd, unsigned char *buf)
/*
* Set MAXIMUM TRANSFER LENGTH
+ *
+ * Currently assumes single PAGE_SIZE per scatterlist for fabrics
+ * enforcing maximum HW scatter-gather-list entry limit
*/
- put_unaligned_be32(dev->dev_attrib.hw_max_sectors, &buf[8]);
+ if (cmd->se_tfo->max_data_sg_nents) {
+ mtl = (cmd->se_tfo->max_data_sg_nents * PAGE_SIZE) /
+ dev->dev_attrib.block_size;
+ }
+ put_unaligned_be32(min_not_zero(mtl, dev->dev_attrib.hw_max_sectors), &buf[8]);
/*
* Set OPTIMAL TRANSFER LENGTH
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index ce8574b..2c934fc 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2254,9 +2254,9 @@ void transport_kunmap_data_sg(struct se_cmd *cmd)
}
EXPORT_SYMBOL(transport_kunmap_data_sg);
-int
+sense_reason_t
target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
- bool zero_page)
+ bool zero_page, u32 max_nents)
{
struct scatterlist *sg;
struct page *page;
@@ -2265,9 +2265,16 @@ target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
int i = 0;
nent = DIV_ROUND_UP(length, PAGE_SIZE);
+ if (max_nents && max_nents >= nent) {
+ pr_err_ratelimited("Required sg nents: %u exceeded fabric driver"
+ " enforced max_nents: %u, failing command\n",
+ nent, max_nents);
+ return TCM_SECTOR_COUNT_TOO_MANY;
+ }
+
sg = kmalloc(sizeof(struct scatterlist) * nent, GFP_KERNEL);
if (!sg)
- return -ENOMEM;
+ return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
sg_init_table(sg, nent);
@@ -2283,7 +2290,7 @@ target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
}
*sgl = sg;
*nents = nent;
- return 0;
+ return TCM_NO_SENSE;
out:
while (i > 0) {
@@ -2291,7 +2298,7 @@ out:
__free_page(sg_page(&sg[i]));
}
kfree(sg);
- return -ENOMEM;
+ return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
}
/*
@@ -2302,15 +2309,15 @@ out:
sense_reason_t
transport_generic_new_cmd(struct se_cmd *cmd)
{
- int ret = 0;
+ sense_reason_t ret = TCM_NO_SENSE;
bool zero_flag = !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB);
if (cmd->prot_op != TARGET_PROT_NORMAL &&
!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) {
ret = target_alloc_sgl(&cmd->t_prot_sg, &cmd->t_prot_nents,
- cmd->prot_length, true);
- if (ret < 0)
- return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+ cmd->prot_length, true, 0);
+ if (ret)
+ return ret;
}
/*
@@ -2333,15 +2340,17 @@ transport_generic_new_cmd(struct se_cmd *cmd)
ret = target_alloc_sgl(&cmd->t_bidi_data_sg,
&cmd->t_bidi_data_nents,
- bidi_length, zero_flag);
- if (ret < 0)
- return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+ bidi_length, zero_flag,
+ cmd->se_tfo->max_data_sg_nents);
+ if (ret)
+ return ret;
}
ret = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents,
- cmd->data_length, zero_flag);
- if (ret < 0)
- return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+ cmd->data_length, zero_flag,
+ cmd->se_tfo->max_data_sg_nents);
+ if (ret)
+ return ret;
} else if ((cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE) &&
cmd->data_length) {
/*
@@ -2353,9 +2362,9 @@ transport_generic_new_cmd(struct se_cmd *cmd)
ret = target_alloc_sgl(&cmd->t_bidi_data_sg,
&cmd->t_bidi_data_nents,
- caw_length, zero_flag);
- if (ret < 0)
- return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+ caw_length, zero_flag, 0);
+ if (ret)
+ return ret;
}
/*
* If this command is not a write we can execute it right here,
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index 4515f52..945b460 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -561,7 +561,7 @@ static int target_xcopy_setup_pt_cmd(
if (alloc_mem) {
rc = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents,
- cmd->data_length, false);
+ cmd->data_length, false, 0);
if (rc < 0) {
ret = rc;
goto out;
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 1e5c8f9..5f2e8e1 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -85,7 +85,7 @@ extern struct configfs_attribute *passthrough_attrib_attrs[];
void *transport_kmap_data_sg(struct se_cmd *);
void transport_kunmap_data_sg(struct se_cmd *);
/* core helpers also used by xcopy during internal command setup */
-int target_alloc_sgl(struct scatterlist **, unsigned int *, u32, bool);
+sense_reason_t target_alloc_sgl(struct scatterlist **, unsigned int *, u32, bool, u32);
sense_reason_t transport_generic_map_mem_to_cmd(struct se_cmd *,
struct scatterlist *, u32, struct scatterlist *, u32);
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 18afef9..357dc96 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -5,6 +5,15 @@ struct target_core_fabric_ops {
struct module *module;
const char *name;
size_t node_acl_size;
+ /*
+ * Limits number of scatterlist entries per SCF_SCSI_DATA_CDB payload.
+ * Setting this value tells target-core to enforce this limit, and
+ * report as INQUIRY EVPD=b0 MAXIMUM TRANSFER LENGTH.
+ *
+ * XXX: Not all initiator hosts honor this block-limit EVPD.
+ * Currently assumes single PAGE_SIZE per scatterlist entry
+ */
+ u32 max_data_sg_nents;
char *(*get_fabric_name)(void);
char *(*tpg_get_wwn)(struct se_portal_group *);
u16 (*tpg_get_tag)(struct se_portal_group *);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
