BUG in copy_page_to_iter() when iscsi sets ENABLE_CLUSTERING
fff7acc40de180 R08: 8f1703786000 R09: >> 2000 >> [ 78.644599] R10: 02c0 R11: 8f16b3f31400 R12: >> >> [ 78.644600] R13: 2000 R14: 0008 R15: >> 3800 >> [ 78.644601] FS: () GS:8f1739c0() >> knlGS: >> [ 78.644601] CS: 0010 DS: ES: CR0: 80050033 >> [ 78.644602] CR2: 7f0f54772000 CR3: 00012e670004 CR4: >> 003606f0 >> [ 78.644624] DR0: DR1: DR2: >> >> [ 78.644625] DR3: DR6: fffe0ff0 DR7: >> 0400 >> [ 78.644625] Call Trace: >> [ 78.644631] skb_copy_datagram_iter+0x16f/0x270 >> [ 78.644633] tcp_recvmsg+0x223/0xc30 >> [ 78.644637] inet_recvmsg+0x4b/0xe0 >> [ 78.644644] iscsit_do_rx_data.constprop.9+0x89/0x110 [iscsi_target_mod] >> [ 78.644650] rx_data+0x58/0x70 [iscsi_target_mod] >> [ 78.644654] iscsit_get_rx_pdu+0xa7b/0xd20 [iscsi_target_mod] >> [ 78.644657] ? preempt_count_sub+0x43/0x50 >> [ 78.644659] ? _raw_spin_unlock_irq+0x22/0x40 >> [ 78.644663] ? iscsi_target_tx_thread+0x1d0/0x1d0 [iscsi_target_mod] >> [ 78.644667] ? iscsi_target_rx_thread+0x71/0xd0 [iscsi_target_mod] >> [ 78.644670] iscsi_target_rx_thread+0x71/0xd0 [iscsi_target_mod] >> [ 78.644672] kthread+0x116/0x130 >> [ 78.644673] ? kthread_create_worker_on_cpu+0x40/0x40 >> [ 78.644675] ret_from_fork+0x24/0x50 >> [ 78.644678] ---[ end trace a0d6d5ab0e8625ee ]--- >> [ 78.644725] connection1:0: detected conn error (1020) >> ... >> [ 108.859031] connection1:0: detected conn error (1020) >> [ 108.859104] sd 3:0:0:0: [sdc] tag#0 FAILED Result: >> hostbyte=DID_TRANSPORT_DISRUPTED driverbyte=DRIVER_OK >> [ 108.859106] sd 3:0:0:0: [sdc] tag#0 CDB: Write(10) 2a 00 00 00 11 a2 00 >> 00 50 00 >> [ 108.859107] print_req_error: I/O error, dev sdc, sector 4514 >> ... TO REPRODUCE: I used openSUSE Tumbleweed using a 4.20 kernel, but any recent kernel should display this issue. I used the same system for both the iSCSI initiator (using open-iscsi) and the iscsi target (using targetcli-fb). [I believe you _could_ use a different system for the initiator, but I did not test this.] I set up a file-based iSCSI target of size 5G using the targetcli shell, with an ACL allowing the local iscsi initiator to connect. The backing store file's contents do not matter, but I initialized my file to all zeros. I then connect from the same system using "iscsiadm -m discovery $HOST" to discover the new target, then "iscsiadm -m node -l" to login to the discovered target. [Any connection strategy should work, though.] This should create a new disc device. In my case, it was "/dev/sdc", since I already have two hard discs. I then do writes to the new device using "dd". There must be enough writes, so I wrote 100 64k blocks of zeros: > dd if=/dev/zero of=/dev/sdc bs=64k count=100 Observe "dmesg --follow" output from the kernel to see the errors and stack traces. -- Lee Duncan
Re: DISABLE_CLUSTERING in scsi drivers
On 11/21/18 1:41 AM, Christoph Hellwig wrote:
> Hi all,
>
> you in the To list maintain or wrote SCSI drivers that set the
> DISABLE_CLUSTERING flag, which basically disable merges of any
> bio segments. We already have the actual max_segment size limit
> to say which length a segment should have, independent of merged
> or originally created, so this limit generally should rarely if
> ever be required, and mostly is an old cut an paste error.
>
> Can you go over your drivers and check if it could be removed?
>
I have tested changing this to ENABLE_CLUSTERING in iscsi_tcp.c. This
code is used for both the iscsi initiator and the target.
On the initiator side, there is no problem with enabling clustering of bios.
But on the target side, the code seems to assume that IOs do not cross
page boundaries, resulting in WARN_ON messages and failed IO when
ENABLE_CLUSTERING is set:
> [ 78.644595] RIP: 0010:copy_page_to_iter+0x1a6/0x2e0
> [ 78.644596] Code: 0c 24 48 98 49 89 ce 48 89 c2 49 29 c6 48 29 ca 4d 01 f4
> 48 01 d5 48 85 c0 0f 85 71 ff ff ff 48 85 ed 0f 84 68 ff ff ff eb b2 <0f> 0b
> 45 31 ed eb 8d bf 01 00 00 00 e8 d9 1e c5 ff 65 4c 8b 34 25
> [ 78.644596] RSP: 0018:a04b41eefbe0 EFLAGS: 00010206
> [ 78.644597] RAX: f7acc40de180 RBX: a04b41eefd80 RCX:
> 0028a014
> [ 78.644598] RDX: 2000 RSI: 1000 RDI:
> f7acc000
> [ 78.644598] RBP: f7acc40de180 R08: 8f1703786000 R09:
> 2000
> [ 78.644599] R10: 02c0 R11: 8f16b3f31400 R12:
>
> [ 78.644600] R13: 2000 R14: 0008 R15:
> 3800
> [ 78.644601] FS: () GS:8f1739c0()
> knlGS:
> [ 78.644601] CS: 0010 DS: ES: CR0: 80050033
> [ 78.644602] CR2: 7f0f54772000 CR3: 00012e670004 CR4:
> 003606f0
> [ 78.644624] DR0: DR1: DR2:
>
> [ 78.644625] DR3: DR6: fffe0ff0 DR7:
> 0400
> [ 78.644625] Call Trace:
> [ 78.644631] skb_copy_datagram_iter+0x16f/0x270
> [ 78.644633] tcp_recvmsg+0x223/0xc30
> [ 78.644637] inet_recvmsg+0x4b/0xe0
> [ 78.644644] iscsit_do_rx_data.constprop.9+0x89/0x110 [iscsi_target_mod]
> [ 78.644650] rx_data+0x58/0x70 [iscsi_target_mod]
> [ 78.644654] iscsit_get_rx_pdu+0xa7b/0xd20 [iscsi_target_mod]
> [ 78.644657] ? preempt_count_sub+0x43/0x50
> [ 78.644659] ? _raw_spin_unlock_irq+0x22/0x40
> [ 78.644663] ? iscsi_target_tx_thread+0x1d0/0x1d0 [iscsi_target_mod]
> [ 78.644667] ? iscsi_target_rx_thread+0x71/0xd0 [iscsi_target_mod]
> [ 78.644670] iscsi_target_rx_thread+0x71/0xd0 [iscsi_target_mod]
> [ 78.644672] kthread+0x116/0x130
> [ 78.644673] ? kthread_create_worker_on_cpu+0x40/0x40
> [ 78.644675] ret_from_fork+0x24/0x50
> [ 78.644678] ---[ end trace a0d6d5ab0e8625ee ]---
>
The problem seems to be in iov_iter.c:copy_page_to_iter(), where it
first calls page_copy_sane(), which makes sure the copy does not cross a
page boundary:
> static inline bool page_copy_sane(struct page *page, size_t offset, size_t n)
> {
> struct page *head = compound_head(page);
> size_t v = n + offset + page_address(page) - page_address(head);
>
> if (likely(n <= v && v <= (PAGE_SIZE << compound_order(head
> return true;
> WARN_ON(1);
> return false;
> }
I will submit a separate BUG email about this to the linux-scsi and
target-devel mailing lists, since it's not clear to me how to fix this.
In the mean time, the iscsi_tcp.c file still needs DISABLE_CLUSTERING.
--
Lee
"Deviants will be sacrificed to ensure group solidarity."
Re: [PATCH v4 1/7] target: use consistent left-aligned ASCII INQUIRY data
On 11/30/18 4:44 AM, David Disseldorp wrote: > On Wed, 28 Nov 2018 17:23:07 -0800, Lee Duncan wrote: > >>> +* unused bytes at the end of the field (i.e., highest offset) and the >>> +* unused bytes shall be filled with ASCII space characters (20h). >>> +*/ >>> + memset([8], 0x20, 8 + 16 + 4); >> >> I dislike that you are using 0x20 here (and below) instead of ' '. > > Given that this patch already has a couple of reviewed-bys, I'd prefer > to avoid respinning it for this. Besides, I think the comment above > makes it pretty clear. > > Thanks for your feedback. > > Cheers, David > Also, the "0x20" is in use widely, I see, so your patch is fine with me. Please feel free to also add my: Reviewed-by: Lee Duncan If you wish.
Re: [PATCH v4 7/7] target: use printf width specifier for t10_wwn field dumps
On 11/28/18 5:01 PM, David Disseldorp wrote:
> From: Bart Van Assche
>
> The existing for loops step over null-terminators for right-padding.
> Padding can be achieved in a much simpler way using printf width
> specifiers.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_device.c | 35 ---
> drivers/target/target_core_stat.c | 32 +++-
> 2 files changed, 15 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index b3d0bd1ab09f..7f2d307e411b 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -720,36 +720,17 @@ void core_dev_free_initiator_node_lun_acl(
> static void scsi_dump_inquiry(struct se_device *dev)
> {
> struct t10_wwn *wwn = >t10_wwn;
> - char buf[INQUIRY_MODEL_LEN + 1];
> - int i, device_type;
> + int device_type = dev->transport->get_device_type(dev);
> +
> /*
>* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
>*/
> - for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> - if (wwn->vendor[i] >= 0x20)
> - buf[i] = wwn->vendor[i];
> - else
> - buf[i] = ' ';
> - buf[i] = '\0';
> - pr_debug(" Vendor: %s\n", buf);
> -
> - for (i = 0; i < INQUIRY_MODEL_LEN; i++)
> - if (wwn->model[i] >= 0x20)
> - buf[i] = wwn->model[i];
> - else
> - buf[i] = ' ';
> - buf[i] = '\0';
> - pr_debug(" Model: %s\n", buf);
> -
> - for (i = 0; i < INQUIRY_REVISION_LEN; i++)
> - if (wwn->revision[i] >= 0x20)
> - buf[i] = wwn->revision[i];
> - else
> - buf[i] = ' ';
> - buf[i] = '\0';
> - pr_debug(" Revision: %s\n", buf);
> -
> - device_type = dev->transport->get_device_type(dev);
> + pr_debug(" Vendor: %-" __stringify(INQUIRY_VENDOR_LEN) "s\n",
> + wwn->vendor);
> + pr_debug(" Model: %-" __stringify(INQUIRY_MODEL_LEN) "s\n",
> + wwn->model);
> + pr_debug(" Revision: %-" __stringify(INQUIRY_REVISION_LEN) "s\n",
> + wwn->revision);
> pr_debug(" Type: %s ", scsi_device_type(device_type));
> }
>
> diff --git a/drivers/target/target_core_stat.c
> b/drivers/target/target_core_stat.c
> index e437ba494865..87fd2b11fe3b 100644
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
> @@ -246,43 +246,25 @@ static ssize_t target_stat_lu_lu_name_show(struct
> config_item *item, char *page)
> static ssize_t target_stat_lu_vend_show(struct config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> - int i;
> - char str[INQUIRY_VENDOR_LEN+1];
>
> - /* scsiLuVendorId */
> - for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> - str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
> - dev->t10_wwn.vendor[i] : ' ';
> - str[i] = '\0';
> - return snprintf(page, PAGE_SIZE, "%s\n", str);
> + return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_VENDOR_LEN)
> + "s\n", dev->t10_wwn.vendor);
> }
>
> static ssize_t target_stat_lu_prod_show(struct config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> - int i;
> - char str[INQUIRY_MODEL_LEN+1];
>
> - /* scsiLuProductId */
> - for (i = 0; i < INQUIRY_MODEL_LEN; i++)
> - str[i] = ISPRINT(dev->t10_wwn.model[i]) ?
> - dev->t10_wwn.model[i] : ' ';
> - str[i] = '\0';
> - return snprintf(page, PAGE_SIZE, "%s\n", str);
> + return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_MODEL_LEN)
> + "s\n", dev->t10_wwn.model);
> }
>
> static ssize_t target_stat_lu_rev_show(struct config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> - int i;
> - char str[INQUIRY_REVISION_LEN+1];
> -
> - /* scsiLuRevisionId */
> - for (i = 0; i < INQUIRY_REVISION_LEN; i++)
> - str[i] = ISPRINT(dev->t10_wwn.revision[i]) ?
> - dev->t10_wwn.revision[i] : ' ';
> - str[i] = '\0';
> - return snprintf(page, PAGE_SIZE, "%s\n", str);
> +
> + return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_REVISION_LEN)
> + "s\n", dev->t10_wwn.revision);
> }
>
> static ssize_t target_stat_lu_dev_type_show(struct config_item *item, char
> *page)
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
On 11/28/18 5:01 PM, David Disseldorp wrote:
> Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
> can be reconfigured via the vendor_id ConfigFS attribute.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_spc.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/target/target_core_spc.c
> b/drivers/target/target_core_spc.c
> index 8ffe712cb44d..4503f3336bc2 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -115,7 +115,8 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char
> *buf)
>*/
> memset([8], 0x20,
> INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
> - memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> + memcpy([8], dev->t10_wwn.vendor,
> +strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
> memcpy([16], dev->t10_wwn.model,
> strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
> memcpy([32], dev->t10_wwn.revision,
> @@ -258,8 +259,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char
> *buf)
> buf[off+1] = 0x1; /* T10 Vendor ID */
> buf[off+2] = 0x0;
> /* left align Vendor ID and pad with spaces */
> - memset([off+4], 0x20, 8);
> - memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
> + memset([off+4], 0x20, INQUIRY_VENDOR_LEN);
> + memcpy([off+4], dev->t10_wwn.vendor,
> + strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
> /* Extra Byte for NULL Terminator */
> id_len++;
> /* Identifier Length */
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 5/7] target: add device vendor_id configfs attribute
On 11/28/18 5:01 PM, David Disseldorp wrote:
> The vendor_id attribute will allow for the modification of the T10
> Vendor Identification string returned in inquiry responses. Its value
> can be viewed and modified via the ConfigFS path at:
> target/core/$backstore/$name/wwn/vendor_id
>
> "LIO-ORG" remains the default value, which is set when the backstore
> device is enabled.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_configfs.c | 48
> +++
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/target/target_core_configfs.c
> b/drivers/target/target_core_configfs.c
> index 9f49b1afd685..fc60c4db718d 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -1213,6 +1213,52 @@ static struct t10_wwn *to_t10_wwn(struct config_item
> *item)
> }
>
> /*
> + * STANDARD and VPD page 0x80 T10 Vendor Identification
> + */
> +static ssize_t target_wwn_vendor_id_show(struct config_item *item,
> + char *page)
> +{
> + return sprintf(page, "%s\n", _t10_wwn(item)->vendor[0]);
> +}
> +
> +static ssize_t target_wwn_vendor_id_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct t10_wwn *t10_wwn = to_t10_wwn(item);
> + struct se_device *dev = t10_wwn->t10_dev;
> + unsigned char buf[INQUIRY_VENDOR_LEN + 1];
> +
> + if (strlen(page) > INQUIRY_VENDOR_LEN) {
> + pr_err("Emulated T10 Vendor Identification exceeds"
> + " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
> + "\n");
> + return -EOVERFLOW;
> + }
> + strlcpy(buf, page, sizeof(buf));
> + /*
> + * Check to see if any active exports exist. If they do exist, fail
> + * here as changing this information on the fly (underneath the
> + * initiator side OS dependent multipath code) could cause negative
> + * effects.
> + */
> + if (dev->export_count) {
> + pr_err("Unable to set T10 Vendor Identification while"
> + " active %d exports exist\n", dev->export_count);
> + return -EINVAL;
> + }
> +
> + /* Assume ASCII encoding. Strip any newline added from userspace. */
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> + strlcpy(dev->t10_wwn.vendor, strstrip(buf),
> + sizeof(dev->t10_wwn.vendor));
> +
> + pr_debug("Target_Core_ConfigFS: Set emulated T10 Vendor Identification:"
> + " %s\n", dev->t10_wwn.vendor);
> +
> + return count;
> +}
> +
> +/*
> * VPD page 0x80 Unit serial
> */
> static ssize_t target_wwn_vpd_unit_serial_show(struct config_item *item,
> @@ -1358,6 +1404,7 @@ DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_target_port, 0x10);
> /* VPD page 0x83 Association: SCSI Target Device */
> DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_scsi_target_device, 0x20);
>
> +CONFIGFS_ATTR(target_wwn_, vendor_id);
> CONFIGFS_ATTR(target_wwn_, vpd_unit_serial);
> CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
> CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
> @@ -1365,6 +1412,7 @@ CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
> CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
>
> static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
> + _wwn_attr_vendor_id,
> _wwn_attr_vpd_unit_serial,
> _wwn_attr_vpd_protocol_identifier,
> _wwn_attr_vpd_assoc_logical_unit,
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 4/7] target: consistently null-terminate t10_wwn.revision
; + for (i = 0; i < INQUIRY_REVISION_LEN; i++)
> str[i] = ISPRINT(dev->t10_wwn.revision[i]) ?
> dev->t10_wwn.revision[i] : ' ';
> str[i] = '\0';
> diff --git a/include/target/target_core_base.h
> b/include/target/target_core_base.h
> index cfc279686cf4..497853a09fee 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -48,6 +48,7 @@
>
> #define INQUIRY_VENDOR_LEN 8
> #define INQUIRY_MODEL_LEN16
> +#define INQUIRY_REVISION_LEN 4
>
> /* Attempts before moving from SHORT to LONG */
> #define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD3
> @@ -323,7 +324,7 @@ struct t10_wwn {
>*/
> char vendor[INQUIRY_VENDOR_LEN + 1];
> char model[INQUIRY_MODEL_LEN + 1];
> - char revision[4];
> + char revision[INQUIRY_REVISION_LEN + 1];
> char unit_serial[INQUIRY_VPD_SERIAL_LEN];
> spinlock_t t10_vpd_lock;
> struct se_device *t10_dev;
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
DOR_LEN + 1);
> memcpy(>vendor[0], [8], INQUIRY_VENDOR_LEN);
> wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
> - memcpy(>model[0], [16], sizeof(wwn->model));
> + BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
> + memcpy(>model[0], [16], INQUIRY_MODEL_LEN);
> + wwn->model[INQUIRY_MODEL_LEN] = '\0';
> memcpy(>revision[0], [32], sizeof(wwn->revision));
> }
>
> @@ -835,7 +837,7 @@ static ssize_t pscsi_show_configfs_dev_params(struct
> se_device *dev, char *b)
> bl += sprintf(b + bl, " ");
> }
> bl += sprintf(b + bl, " Model: ");
> - for (i = 0; i < 16; i++) {
> + for (i = 0; i < INQUIRY_MODEL_LEN; i++) {
> if (ISPRINT(sd->model[i])) /* printable character ? */
> bl += sprintf(b + bl, "%c", sd->model[i]);
> else
> diff --git a/drivers/target/target_core_spc.c
> b/drivers/target/target_core_spc.c
> index c37dd36ec77d..78eddee4b6e6 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -116,7 +116,7 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char
> *buf)
> memset([8], 0x20, 8 + 16 + 4);
> memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> memcpy([16], dev->t10_wwn.model,
> -strnlen(dev->t10_wwn.model, 16));
> +strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
> memcpy([32], dev->t10_wwn.revision,
> strnlen(dev->t10_wwn.revision, 4));
> buf[4] = 31; /* Set additional length to 31 */
> diff --git a/drivers/target/target_core_stat.c
> b/drivers/target/target_core_stat.c
> index 4210cf625d84..9123c5137da5 100644
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
> @@ -261,10 +261,10 @@ static ssize_t target_stat_lu_prod_show(struct
> config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> int i;
> - char str[sizeof(dev->t10_wwn.model)+1];
> + char str[INQUIRY_MODEL_LEN+1];
>
> /* scsiLuProductId */
> - for (i = 0; i < sizeof(dev->t10_wwn.model); i++)
> + for (i = 0; i < INQUIRY_MODEL_LEN; i++)
> str[i] = ISPRINT(dev->t10_wwn.model[i]) ?
> dev->t10_wwn.model[i] : ' ';
> str[i] = '\0';
> diff --git a/include/target/target_core_base.h
> b/include/target/target_core_base.h
> index cb1f3f574e2a..cfc279686cf4 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -47,6 +47,7 @@
> #define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN254
>
> #define INQUIRY_VENDOR_LEN 8
> +#define INQUIRY_MODEL_LEN16
>
> /* Attempts before moving from SHORT to LONG */
> #define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD3
> @@ -321,7 +322,7 @@ struct t10_wwn {
>* null terminator is always present.
>*/
> char vendor[INQUIRY_VENDOR_LEN + 1];
> - char model[16];
> + char model[INQUIRY_MODEL_LEN + 1];
> char revision[4];
> char unit_serial[INQUIRY_VPD_SERIAL_LEN];
> spinlock_t t10_vpd_lock;
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 2/7] target: consistently null-terminate t10_wwn.vendor
On 11/28/18 5:01 PM, David Disseldorp wrote:
> In preparation for supporting user provided vendor strings, add an extra
> byte to t10_wwn.vendor which will ensure null string termination when an
> eight byte vendor string is provided by the user.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_device.c | 6 --
> drivers/target/target_core_pscsi.c | 6 --
> drivers/target/target_core_stat.c | 4 ++--
> include/target/target_core_base.h | 8 +++-
> 4 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index 47b5ef153135..fe4c4db51137 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -725,7 +725,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
> /*
>* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
>*/
> - for (i = 0; i < 8; i++)
> + for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> if (wwn->vendor[i] >= 0x20)
> buf[i] = wwn->vendor[i];
> else
> @@ -1008,8 +1008,10 @@ int target_configure_device(struct se_device *dev)
>* anything virtual (IBLOCK, FILEIO, RAMDISK), but not for TCM/pSCSI
>* passthrough because this is being provided by the backend LLD.
>*/
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
> - strncpy(>t10_wwn.vendor[0], "LIO-ORG", 8);
> + strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
> + dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
> strncpy(>t10_wwn.model[0],
> dev->transport->inquiry_prod, 16);
> strncpy(>t10_wwn.revision[0],
> diff --git a/drivers/target/target_core_pscsi.c
> b/drivers/target/target_core_pscsi.c
> index 47d76c862014..ee65b5bb674c 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -190,7 +190,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
> t10_wwn *wwn)
> /*
>* Use sdev->inquiry from drivers/scsi/scsi_scan.c:scsi_alloc_sdev()
>*/
> - memcpy(>vendor[0], [8], sizeof(wwn->vendor));
> + BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
> + memcpy(>vendor[0], [8], INQUIRY_VENDOR_LEN);
> + wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
> memcpy(>model[0], [16], sizeof(wwn->model));
> memcpy(>revision[0], [32], sizeof(wwn->revision));
> }
> @@ -826,7 +828,7 @@ static ssize_t pscsi_show_configfs_dev_params(struct
> se_device *dev, char *b)
> if (sd) {
> bl += sprintf(b + bl, "");
> bl += sprintf(b + bl, "Vendor: ");
> - for (i = 0; i < 8; i++) {
> + for (i = 0; i < INQUIRY_VENDOR_LEN; i++) {
> if (ISPRINT(sd->vendor[i])) /* printable character? */
> bl += sprintf(b + bl, "%c", sd->vendor[i]);
> else
> diff --git a/drivers/target/target_core_stat.c
> b/drivers/target/target_core_stat.c
> index f0db91ebd735..4210cf625d84 100644
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
> @@ -247,10 +247,10 @@ static ssize_t target_stat_lu_vend_show(struct
> config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> int i;
> - char str[sizeof(dev->t10_wwn.vendor)+1];
> + char str[INQUIRY_VENDOR_LEN+1];
>
> /* scsiLuVendorId */
> - for (i = 0; i < sizeof(dev->t10_wwn.vendor); i++)
> + for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
> dev->t10_wwn.vendor[i] : ' ';
> str[i] = '\0';
> diff --git a/include/target/target_core_base.h
> b/include/target/target_core_base.h
> index e3bdb0550a59..cb1f3f574e2a 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -46,6 +46,8 @@
> /* Used by transport_get_inquiry_vpd_device_ident() */
> #define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN254
>
> +#define INQUIRY_VENDOR_LEN 8
> +
> /* Attempts before moving from SHORT to LONG */
> #define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD3
> #define PYX_TRANSPORT_WINDOW_CLOSED_WAIT_SHORT 3 /* In milliseconds */
> @@ -314,7 +316,11 @@ struct t10_vpd {
> };
>
> struct t10_wwn {
> - char vendor[8];
> + /*
> + * SCSI left aligned strings may not be null terminated. +1 to ensure a
> + * null terminator is always present.
> + */
> + char vendor[INQUIRY_VENDOR_LEN + 1];
> char model[16];
> char revision[4];
> char unit_serial[INQUIRY_VPD_SERIAL_LEN];
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 1/7] target: use consistent left-aligned ASCII INQUIRY data
On 11/28/18 5:01 PM, David Disseldorp wrote:
> spc5r17.pdf specifies:
> 4.3.1 ASCII data field requirements
> ASCII data fields shall contain only ASCII printable characters (i.e.,
> code values 20h to 7Eh) and may be terminated with one or more ASCII
> null (00h) characters.
> ASCII data fields described as being left-aligned shall have any
> unused bytes at the end of the field (i.e., highest offset) and the
> unused bytes shall be filled with ASCII space characters (20h).
>
> LIO currently space-pads the T10 VENDOR IDENTIFICATION and PRODUCT
> IDENTIFICATION fields in the standard INQUIRY data. However, the
> PRODUCT REVISION LEVEL field in the standard INQUIRY data as well as the
> T10 VENDOR IDENTIFICATION field in the INQUIRY Device Identification VPD
> Page are zero-terminated/zero-padded.
>
> Fix this inconsistency by using space-padding for all of the above
> fields.
>
> Signed-off-by: David Disseldorp
> Reviewed-by: Christoph Hellwig
> ---
> drivers/target/target_core_spc.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/target/target_core_spc.c
> b/drivers/target/target_core_spc.c
> index f459118bc11b..c37dd36ec77d 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -108,12 +108,17 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned
> char *buf)
>
> buf[7] = 0x2; /* CmdQue=1 */
>
> - memcpy([8], "LIO-ORG ", 8);
> - memset([16], 0x20, 16);
> + /*
> + * ASCII data fields described as being left-aligned shall have any
> + * unused bytes at the end of the field (i.e., highest offset) and the
> + * unused bytes shall be filled with ASCII space characters (20h).
> + */
> + memset([8], 0x20, 8 + 16 + 4);
I dislike that you are using 0x20 here (and below) instead of ' '.
> + memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> memcpy([16], dev->t10_wwn.model,
> -min_t(size_t, strlen(dev->t10_wwn.model), 16));
> +strnlen(dev->t10_wwn.model, 16));
> memcpy([32], dev->t10_wwn.revision,
> -min_t(size_t, strlen(dev->t10_wwn.revision), 4));
> +strnlen(dev->t10_wwn.revision, 4));
> buf[4] = 31; /* Set additional length to 31 */
>
> return 0;
> @@ -251,7 +256,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char
> *buf)
> buf[off] = 0x2; /* ASCII */
> buf[off+1] = 0x1; /* T10 Vendor ID */
> buf[off+2] = 0x0;
> - memcpy([off+4], "LIO-ORG", 8);
> + /* left align Vendor ID and pad with spaces */
> + memset([off+4], 0x20, 8);
> + memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
> /* Extra Byte for NULL Terminator */
> id_len++;
> /* Identifier Length */
>
Re: [PATCH v2 8/9] qedi: Move LL2 producer index processing in BH.
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> From: Manish Rangankar
>
> 1. Removed logic to update HW producer index in interrupt context.
> 2. Update HW producer index after UIO ring and buffer gets initialized.
>
> Signed-off-by: Manish Rangankar
> ---
> drivers/scsi/qedi/qedi_main.c | 31 +++
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 8942f62..053a947 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -665,7 +665,6 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff *skb,
> u32 arg1, u32 arg2)
> struct qedi_uio_ctrl *uctrl;
> struct skb_work_list *work;
> struct ethhdr *eh;
> - u32 prod;
>
> if (!qedi) {
> QEDI_ERR(NULL, "qedi is NULL\n");
> @@ -724,17 +723,10 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff
> *skb, u32 arg1, u32 arg2)
>
> spin_lock_bh(>ll2_lock);
> list_add_tail(>list, >ll2_skb_list);
> + spin_unlock_bh(>ll2_lock);
>
> - ++uctrl->hw_rx_prod_cnt;
> - prod = (uctrl->hw_rx_prod + 1) % RX_RING;
> - if (prod != uctrl->host_rx_cons) {
> - uctrl->hw_rx_prod = prod;
> - spin_unlock_bh(>ll2_lock);
> - wake_up_process(qedi->ll2_recv_thread);
> - return 0;
> - }
> + wake_up_process(qedi->ll2_recv_thread);
>
> - spin_unlock_bh(>ll2_lock);
> return 0;
> }
>
> @@ -749,6 +741,7 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi,
> struct sk_buff *skb,
> u32 rx_bd_prod;
> void *pkt;
> int len = 0;
> + u32 prod;
>
> if (!qedi) {
> QEDI_ERR(NULL, "qedi is NULL\n");
> @@ -757,12 +750,16 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi,
> struct sk_buff *skb,
>
> udev = qedi->udev;
> uctrl = udev->uctrl;
> - pkt = udev->rx_pkt + (uctrl->hw_rx_prod * qedi_ll2_buf_size);
> +
> + ++uctrl->hw_rx_prod_cnt;
> + prod = (uctrl->hw_rx_prod + 1) % RX_RING;
> +
> + pkt = udev->rx_pkt + (prod * qedi_ll2_buf_size);
> len = min_t(u32, skb->len, (u32)qedi_ll2_buf_size);
> memcpy(pkt, skb->data, len);
>
> memset(, 0, sizeof(rxbd));
> - rxbd.rx_pkt_index = uctrl->hw_rx_prod;
> + rxbd.rx_pkt_index = prod;
> rxbd.rx_pkt_len = len;
> rxbd.vlan_id = vlan_id;
>
> @@ -773,6 +770,16 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi,
> struct sk_buff *skb,
>
> memcpy(p_rxbd, , sizeof(rxbd));
>
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "hw_rx_prod [%d] prod [%d] hw_rx_bd_prod [%d] rx_pkt_idx [%d]
> rx_len [%d].\n",
> + uctrl->hw_rx_prod, prod, uctrl->hw_rx_bd_prod,
> + rxbd.rx_pkt_index, rxbd.rx_pkt_len);
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "host_rx_cons [%d] hw_rx_bd_cons [%d].\n",
> + uctrl->host_rx_cons, uctrl->host_rx_bd_cons);
> +
> + uctrl->hw_rx_prod = prod;
> +
> /* notify the iscsiuio about new packet */
> uio_event_notify(>qedi_uinfo);
>
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 7/9] qedi: add module param to set ping packet size
On 11/21/18 1:25 AM, Nilesh Javali wrote: > Default packet size is 0x400. > For jumbo packets set to 0x2400. > > Signed-off-by: Nilesh Javali > --- > drivers/scsi/qedi/qedi.h | 1 - > drivers/scsi/qedi/qedi_main.c | 13 + > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h > index 6fa02c5..a26bb506 100644 > --- a/drivers/scsi/qedi/qedi.h > +++ b/drivers/scsi/qedi/qedi.h > @@ -63,7 +63,6 @@ > #define QEDI_LOCAL_PORT_INVALID 0x > #define TX_RX_RING 16 > #define RX_RING (TX_RX_RING - 1) > -#define LL2_SINGLE_BUF_SIZE 0x400 > #define QEDI_PAGE_ALIGN(addr)ALIGN(addr, QEDI_PAGE_SIZE) > #define QEDI_PAGE_MASK (~((QEDI_PAGE_SIZE) - 1)) > > diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c > index 2621dee..8942f62 100644 > --- a/drivers/scsi/qedi/qedi_main.c > +++ b/drivers/scsi/qedi/qedi_main.c > @@ -44,6 +44,11 @@ > MODULE_PARM_DESC(qedi_io_tracing, >" Enable logging of SCSI requests/completions into trace > buffer. (default off)."); > > +uint qedi_ll2_buf_size = 0x400; > +module_param(qedi_ll2_buf_size, uint, 0644); > +MODULE_PARM_DESC(qedi_ll2_buf_size, > + "parameter to set ping packet size, default - 0x400, Jumbo > packets - 0x2400."); > + > const struct qed_iscsi_ops *qedi_ops; > static struct scsi_transport_template *qedi_scsi_transport; > static struct pci_driver qedi_pci_driver; > @@ -228,7 +233,7 @@ static int __qedi_alloc_uio_rings(struct qedi_uio_dev > *udev) > } > > /* Allocating memory for Tx/Rx pkt buffer */ > - udev->ll2_buf_size = TX_RX_RING * LL2_SINGLE_BUF_SIZE; > + udev->ll2_buf_size = TX_RX_RING * qedi_ll2_buf_size; > udev->ll2_buf_size = QEDI_PAGE_ALIGN(udev->ll2_buf_size); > udev->ll2_buf = (void *)__get_free_pages(GFP_KERNEL | __GFP_COMP | >__GFP_ZERO, 2); > @@ -283,7 +288,7 @@ static int qedi_alloc_uio_rings(struct qedi_ctx *qedi) > qedi->udev = udev; > > udev->tx_pkt = udev->ll2_buf; > - udev->rx_pkt = udev->ll2_buf + LL2_SINGLE_BUF_SIZE; > + udev->rx_pkt = udev->ll2_buf + qedi_ll2_buf_size; > return 0; > > err_uctrl: > @@ -752,8 +757,8 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi, > struct sk_buff *skb, > > udev = qedi->udev; > uctrl = udev->uctrl; > - pkt = udev->rx_pkt + (uctrl->hw_rx_prod * LL2_SINGLE_BUF_SIZE); > - len = min_t(u32, skb->len, (u32)LL2_SINGLE_BUF_SIZE); > + pkt = udev->rx_pkt + (uctrl->hw_rx_prod * qedi_ll2_buf_size); > + len = min_t(u32, skb->len, (u32)qedi_ll2_buf_size); > memcpy(pkt, skb->data, len); > > memset(, 0, sizeof(rxbd)); > Reviewed-by: Lee Duncan
Re: [PATCH v2 6/9] qedi: Add packet filter in light L2 Rx path.
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> From: Manish Rangankar
>
> Add packet filter to avoid unnecessary packet processing in iscsiuio.
>
> Signed-off-by: Manish Rangankar
> ---
> drivers/scsi/qedi/qedi_main.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 713db9c..2621dee 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -659,6 +659,7 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff *skb,
> u32 arg1, u32 arg2)
> struct qedi_uio_dev *udev;
> struct qedi_uio_ctrl *uctrl;
> struct skb_work_list *work;
> + struct ethhdr *eh;
> u32 prod;
>
> if (!qedi) {
> @@ -673,6 +674,29 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff
> *skb, u32 arg1, u32 arg2)
> return 0;
> }
>
> + eh = (struct ethhdr *)skb->data;
> + /* Undo VLAN encapsulation */
> + if (eh->h_proto == htons(ETH_P_8021Q)) {
> + memmove((u8 *)eh + VLAN_HLEN, eh, ETH_ALEN * 2);
> + eh = (struct ethhdr *)skb_pull(skb, VLAN_HLEN);
> + skb_reset_mac_header(skb);
> + }
> +
> + /* Filter out non FIP/FCoE frames here to free them faster */
> + if (eh->h_proto != htons(ETH_P_ARP) &&
> + eh->h_proto != htons(ETH_P_IP) &&
> + eh->h_proto != htons(ETH_P_IPV6)) {
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "Dropping frame ethertype [0x%x] len [0x%x].\n",
> + eh->h_proto, skb->len);
> + kfree_skb(skb);
> + return 0;
> + }
> +
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "Allowed frame ethertype [0x%x] len [0x%x].\n",
> + eh->h_proto, skb->len);
> +
> udev = qedi->udev;
> uctrl = udev->uctrl;
>
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 5/9] qedi: Check for session online before getting iSCSI TLV data.
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> From: Manish Rangankar
>
> The kernel panic was observed after switch side perturbation,
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [] strcmp+0x20/0x40
> PGD 0 Oops: [#1] SMP
> CPU: 8 PID: 647 Comm: kworker/8:1 Tainted: GW OE
> 3.10.0-693.el7.x86_64 #1
> Hardware name: HPE ProLiant DL380 Gen10/ProLiant DL380 Gen10, BIOS U30
> 06/20/2018
> Workqueue: slowpath-13:00. qed_slowpath_task [qed]
> task: 880429eb8fd0 ti: 88042919 task.ti: 88042919
> RIP: 0010:[] [] strcmp+0x20/0x40
> RSP: 0018:880429193c68 EFLAGS: 00010202
> RAX: 000a RBX: 0002 RCX:
> RDX: 0001 RSI: 0001 RDI: 88042bda7a41
> RBP: 880429193c68 R08: R09:
> R10: 0007 R11: 88042b3af338 R12: 880420b007a0
> R13: 88081aa56af8 R14: 0001 R15: 88081aa50410
> FS: () GS:88042fe0() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: CR3: 019f2000 CR4: 003407e0
> DR0: DR1: DR2:
> DR3: DR6: fffe0ff0 DR7: 0400
> Stack:
> 880429193d20 c02a0c90 c90004b32000 8803fd3ec600
> 88042bda7800 88042bda7a00 88042bda7840 88042bda7a40
> 000129193d10 2e3836312e323931 ff000a342e363232 c01ad99d
> Call Trace:
> [] qedi_get_protocol_tlv_data+0x270/0x470 [qedi]
> [] ? qed_mfw_process_tlv_req+0x24d/0xbf0 [qed]
> [] qed_mfw_fill_tlv_data+0x5e/0xd0 [qed]
> [] qed_mfw_process_tlv_req+0x269/0xbf0 [qed]
>
> Fix kernel NULL pointer deref by checking for session is online
> before getting iSCSI TLV data.
>
> Signed-off-by: Manish Rangankar
> ---
> drivers/scsi/qedi/qedi_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 5308e6b..713db9c 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -952,6 +952,9 @@ static int qedi_find_boot_info(struct qedi_ctx *qedi,
> cls_sess = iscsi_conn_to_session(cls_conn);
> sess = cls_sess->dd_data;
>
> + if (!iscsi_is_session_online(cls_sess))
> + continue;
> +
> if (pri_ctrl_flags) {
> if (!strcmp(pri_tgt->iscsi_name, sess->targetname) &&
> !strcmp(pri_tgt->ip_addr, ep_ip_addr)) {
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 3/9] qedi: Replace PAGE_SIZE with QEDI_PAGE_SIZE
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> Use QEDI_PAGE_SIZE for enablement of module on systems with 64K page size.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi_main.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 0f8eb5f..a1225ae 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -796,7 +796,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> int rval = 0;
>
>
> - num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
> + num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / QEDI_PAGE_SIZE;
>
> qedi->num_queues = MIN_NUM_CPUS_MSIX(qedi);
>
> @@ -834,7 +834,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> qedi->pf_params.iscsi_pf_params.max_fin_rt = 2;
>
> for (log_page_size = 0 ; log_page_size < 32 ; log_page_size++) {
> - if ((1 << log_page_size) == PAGE_SIZE)
> + if ((1 << log_page_size) == QEDI_PAGE_SIZE)
> break;
> }
> qedi->pf_params.iscsi_pf_params.log_page_size = log_page_size;
> @@ -1376,7 +1376,7 @@ static void qedi_free_bdq(struct qedi_ctx *qedi)
> int i;
>
> if (qedi->bdq_pbl_list)
> - dma_free_coherent(>pdev->dev, PAGE_SIZE,
> + dma_free_coherent(>pdev->dev, QEDI_PAGE_SIZE,
> qedi->bdq_pbl_list, qedi->bdq_pbl_list_dma);
>
> if (qedi->bdq_pbl)
> @@ -1437,7 +1437,7 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
>
> /* Alloc dma memory for BDQ page buffer list */
> qedi->bdq_pbl_mem_size = QEDI_BDQ_NUM * sizeof(struct scsi_bd);
> - qedi->bdq_pbl_mem_size = ALIGN(qedi->bdq_pbl_mem_size, PAGE_SIZE);
> + qedi->bdq_pbl_mem_size = ALIGN(qedi->bdq_pbl_mem_size, QEDI_PAGE_SIZE);
> qedi->rq_num_entries = qedi->bdq_pbl_mem_size / sizeof(struct scsi_bd);
>
> QEDI_INFO(>dbg_ctx, QEDI_LOG_CONN, "rq_num_entries = %d.\n",
> @@ -1472,7 +1472,8 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
> }
>
> /* Allocate list of PBL pages */
> - qedi->bdq_pbl_list = dma_zalloc_coherent(>pdev->dev, PAGE_SIZE,
> + qedi->bdq_pbl_list = dma_zalloc_coherent(>pdev->dev,
> + QEDI_PAGE_SIZE,
>>bdq_pbl_list_dma,
>GFP_KERNEL);
> if (!qedi->bdq_pbl_list) {
> @@ -1485,13 +1486,14 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
>* Now populate PBL list with pages that contain pointers to the
>* individual buffers.
>*/
> - qedi->bdq_pbl_list_num_entries = qedi->bdq_pbl_mem_size / PAGE_SIZE;
> + qedi->bdq_pbl_list_num_entries = qedi->bdq_pbl_mem_size /
> + QEDI_PAGE_SIZE;
> list = (u64 *)qedi->bdq_pbl_list;
> page = qedi->bdq_pbl_list_dma;
> for (i = 0; i < qedi->bdq_pbl_list_num_entries; i++) {
> *list = qedi->bdq_pbl_dma;
> list++;
> - page += PAGE_SIZE;
> + page += QEDI_PAGE_SIZE;
> }
>
> return 0;
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 2/9] qedi: Fix spelling mistake "OUSTANDING" -> "OUTSTANDING"
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> Fix trivial spelling mistake within macro definition.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi.h | 4 ++--
> drivers/scsi/qedi/qedi_main.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h
> index e966855..6fa02c5 100644
> --- a/drivers/scsi/qedi/qedi.h
> +++ b/drivers/scsi/qedi/qedi.h
> @@ -45,7 +45,7 @@
> #define QEDI_MAX_TASK_NUM0x0FFF
> #define QEDI_MAX_ISCSI_CONNS_PER_HBA 1024
> #define QEDI_ISCSI_MAX_BDS_PER_CMD 255 /* Firmware max BDs is 255 */
> -#define MAX_OUSTANDING_TASKS_PER_CON 1024
> +#define MAX_OUTSTANDING_TASKS_PER_CON1024
>
> #define QEDI_MAX_BD_LEN 0x
> #define QEDI_BD_SPLIT_SZ 0x1000
> @@ -144,7 +144,7 @@ struct skb_work_list {
> };
>
> /* Queue sizes in number of elements */
> -#define QEDI_SQ_SIZE MAX_OUSTANDING_TASKS_PER_CON
> +#define QEDI_SQ_SIZE MAX_OUTSTANDING_TASKS_PER_CON
> #define QEDI_CQ_SIZE 2048
> #define QEDI_CMDQ_SIZE QEDI_MAX_ISCSI_TASK
> #define QEDI_PROTO_CQ_PROD_IDX 0
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 105b0e4..0f8eb5f 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -796,7 +796,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> int rval = 0;
>
>
> - num_sq_pages = (MAX_OUSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
> + num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
>
> qedi->num_queues = MIN_NUM_CPUS_MSIX(qedi);
>
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 1/9] qedi: Cleanup redundant QEDI_PAGE_SIZE macro definition
On 11/21/18 1:25 AM, Nilesh Javali wrote: > Remove redundant macro definition. > > Signed-off-by: Nilesh Javali > --- > drivers/scsi/qedi/qedi.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h > index a6f96b3..e966855 100644 > --- a/drivers/scsi/qedi/qedi.h > +++ b/drivers/scsi/qedi/qedi.h > @@ -64,11 +64,9 @@ > #define TX_RX_RING 16 > #define RX_RING (TX_RX_RING - 1) > #define LL2_SINGLE_BUF_SIZE 0x400 > -#define QEDI_PAGE_SIZE 4096 > #define QEDI_PAGE_ALIGN(addr)ALIGN(addr, QEDI_PAGE_SIZE) > #define QEDI_PAGE_MASK (~((QEDI_PAGE_SIZE) - 1)) > > -#define QEDI_PAGE_SIZE 4096 > #define QEDI_HW_DMA_BOUNDARY 0xfff > #define QEDI_PATH_HANDLE 0xFE000UL > > Reviewed-by: Lee Duncan
Re: [PATCH v2 4/9] qedi: Allocate IRQs based on msix_cnt
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> The driver load on some systems failed with error,
> [0004:01:00.5]:[qedi_request_msix_irq:2524]:8: request_irq failed.
>
> Allocate the IRQs based on MSIX count obtained from qed module
> instead of number of queues.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index a1225ae..5308e6b 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -1298,7 +1298,7 @@ static int qedi_request_msix_irq(struct qedi_ctx *qedi)
> int i, rc, cpu;
>
> cpu = cpumask_first(cpu_online_mask);
> - for (i = 0; i < MIN_NUM_CPUS_MSIX(qedi); i++) {
> + for (i = 0; i < qedi->int_info.msix_cnt; i++) {
> rc = request_irq(qedi->int_info.msix[i].vector,
>qedi_msix_handler, 0, "qedi",
>>fp_array[i]);
>
Reviewed-by: Lee Duncan
Re: [PATCH 6/8] qedi: Add packet filter in light L2 Rx path.
On 11/20/18 2:37 AM, Nilesh Javali wrote:
> From: Manish Rangankar
>
> Add packet filter to avoid unnecessary packet processing in iscsiuio.
>
> Signed-off-by: Manish Rangankar
> ---
> drivers/scsi/qedi/qedi_main.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 713db9c..2621dee 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -659,6 +659,7 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff *skb,
> u32 arg1, u32 arg2)
> struct qedi_uio_dev *udev;
> struct qedi_uio_ctrl *uctrl;
> struct skb_work_list *work;
> + struct ethhdr *eh;
> u32 prod;
>
> if (!qedi) {
> @@ -673,6 +674,29 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff
> *skb, u32 arg1, u32 arg2)
> return 0;
> }
>
> + eh = (struct ethhdr *)skb->data;
> + /* Undo VLAN encapsulation */
> + if (eh->h_proto == htons(ETH_P_8021Q)) {
> + memmove((u8 *)eh + VLAN_HLEN, eh, ETH_ALEN * 2);
> + eh = (struct ethhdr *)skb_pull(skb, VLAN_HLEN);
> + skb_reset_mac_header(skb);
> + }
> +
> + /* Filter out non FIP/FCoE frames here to free them faster */
> + if (eh->h_proto != htons(ETH_P_ARP) &&
> + eh->h_proto != htons(ETH_P_IP) &&
> + eh->h_proto != htons(ETH_P_IPV6)) {
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "Dropping frame ethertype [0x%x] len [0x%x].\n",
> + eh->h_proto, skb->len);
> + kfree_skb(skb);
> + return 0;
> + }
> +
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "Allowed frame ethertype [0x%x] len [0x%x].\n",
> + eh->h_proto, skb->len);
> +
> udev = qedi->udev;
> uctrl = udev->uctrl;
>
>
Reviewed-by: Lee Duncan
Re: [PATCH 5/8] qedi: Check for session online before getting iSCSI TLV data.
On 11/20/18 2:37 AM, Nilesh Javali wrote:
> From: Manish Rangankar
>
> The kernel panic was observed after switch side perturbation,
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [] strcmp+0x20/0x40
> PGD 0 Oops: [#1] SMP
> CPU: 8 PID: 647 Comm: kworker/8:1 Tainted: GW OE
> 3.10.0-693.el7.x86_64 #1
> Hardware name: HPE ProLiant DL380 Gen10/ProLiant DL380 Gen10, BIOS U30
> 06/20/2018
> Workqueue: slowpath-13:00. qed_slowpath_task [qed]
> task: 880429eb8fd0 ti: 88042919 task.ti: 88042919
> RIP: 0010:[] [] strcmp+0x20/0x40
> RSP: 0018:880429193c68 EFLAGS: 00010202
> RAX: 000a RBX: 0002 RCX:
> RDX: 0001 RSI: 0001 RDI: 88042bda7a41
> RBP: 880429193c68 R08: R09:
> R10: 0007 R11: 88042b3af338 R12: 880420b007a0
> R13: 88081aa56af8 R14: 0001 R15: 88081aa50410
> FS: () GS:88042fe0() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: CR3: 019f2000 CR4: 003407e0
> DR0: DR1: DR2:
> DR3: DR6: fffe0ff0 DR7: 0400
> Stack:
> 880429193d20 c02a0c90 c90004b32000 8803fd3ec600
> 88042bda7800 88042bda7a00 88042bda7840 88042bda7a40
> 000129193d10 2e3836312e323931 ff000a342e363232 c01ad99d
> Call Trace:
> [] qReviewed-by: Lee Duncan
> edi_get_protocol_tlv_data+0x270/0x470 [qedi]
> [] ? qed_mfw_process_tlv_req+0x24d/0xbf0 [qed]
> [] qed_mfw_fill_tlv_data+0x5e/0xd0 [qed]
> [] qed_mfw_process_tlv_req+0x269/0xbf0 [qed]
>
> Fix kernel NULL pointer deref by checking for session is online
> before getting iSCSI TLV data.
>
> Signed-off-by: Manish Rangankar
> ---
> drivers/scsi/qedi/qedi_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 5308e6b..713db9c 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -952,6 +952,9 @@ static int qedi_find_boot_info(struct qedi_ctx *qedi,
> cls_sess = iscsi_conn_to_session(cls_conn);
> sess = cls_sess->dd_data;
>
> + if (!iscsi_is_session_online(cls_sess))
> + continue;
> +
> if (pri_ctrl_flags) {
> if (!strcmp(pri_tgt->iscsi_name, sess->targetname) &&
> !strcmp(pri_tgt->ip_addr, ep_ip_addr)) {
>
Reviewed-by: Lee Duncan
Re: [PATCH 4/8] qedi: Allocate IRQs based on msix_cnt
On 11/20/18 2:37 AM, Nilesh Javali wrote:
> The driver load on some systems failed with error,
> [0004:01:00.5]:[qedi_request_msix_irq:2524]:8: request_irq failed.
>
> Allocate the IRQs based on MSIX count obtained from qed module
> instead of number of queues.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index a1225ae..5308e6b 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -1298,7 +1298,7 @@ static int qedi_request_msix_irq(struct qedi_ctx *qedi)
> int i, rc, cpu;
>
> cpu = cpumask_first(cpu_online_mask);
> - for (i = 0; i < MIN_NUM_CPUS_MSIX(qedi); i++) {
> + for (i = 0; i < qedi->int_info.msix_cnt; i++) {
> rc = request_irq(qedi->int_info.msix[i].vector,
>qedi_msix_handler, 0, "qedi",
>>fp_array[i]);
>
Reviewed-by: Lee Duncan
Re: [PATCH 3/8] qedi: Replace PAGE_SIZE with QEDI_PAGE_SIZE
On 11/20/18 2:37 AM, Nilesh Javali wrote:
> Use QEDI_PAGE_SIZE for enablement of module on systems with 64K page size.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi_main.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 0f8eb5f..a1225ae 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -796,7 +796,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> int rval = 0;
>
>
> - num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
> + num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / QEDI_PAGE_SIZE;
>
> qedi->num_queues = MIN_NUM_CPUS_MSIX(qedi);
>
> @@ -834,7 +834,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> qedi->pf_params.iscsi_pf_params.max_fin_rt = 2;
>
> for (log_page_size = 0 ; log_page_size < 32 ; log_page_size++) {
> - if ((1 << log_page_size) == PAGE_SIZE)
> + if ((1 << log_page_size) == QEDI_PAGE_SIZE)
> break;
> }
> qedi->pf_params.iscsi_pf_params.log_page_size = log_page_size;
> @@ -1376,7 +1376,7 @@ static void qedi_free_bdq(struct qedi_ctx *qedi)
> int i;
>
> if (qedi->bdq_pbl_list)
> - dma_free_coherent(>pdev->dev, PAGE_SIZE,
> + dma_free_coherent(>pdev->dev, QEDI_PAGE_SIZE,
> qedi->bdq_pbl_list, qedi->bdq_pbl_list_dma);
>
> if (qedi->bdq_pbl)
> @@ -1437,7 +1437,7 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
>
> /* Alloc dma memory for BDQ page buffer list */
> qedi->bdq_pbl_mem_size = QEDI_BDQ_NUM * sizeof(struct scsi_bd);
> - qedi->bdq_pbl_mem_size = ALIGN(qedi->bdq_pbl_mem_size, PAGE_SIZE);
> + qedi->bdq_pbl_mem_size = ALIGN(qedi->bdq_pbl_mem_size, QEDI_PAGE_SIZE);
> qedi->rq_num_entries = qedi->bdq_pbl_mem_size / sizeof(struct scsi_bd);
>
> QEDI_INFO(>dbg_ctx, QEDI_LOG_CONN, "rq_num_entries = %d.\n",
> @@ -1472,7 +1472,8 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
> }
>
> /* Allocate list of PBL pages */
> - qedi->bdq_pbl_list = dma_zalloc_coherent(>pdev->dev, PAGE_SIZE,
> + qedi->bdq_pbl_list = dma_zalloc_coherent(>pdev->dev,
> + QEDI_PAGE_SIZE,
>>bdq_pbl_list_dma,
>GFP_KERNEL);
> if (!qedi->bdq_pbl_list) {
> @@ -1485,13 +1486,14 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
>* Now populate PBL list with pages that contain pointers to the
>* individual buffers.
>*/
> - qedi->bdq_pbl_list_num_entries = qedi->bdq_pbl_mem_size / PAGE_SIZE;
> + qedi->bdq_pbl_list_num_entries = qedi->bdq_pbl_mem_size /
> + QEDI_PAGE_SIZE;
> list = (u64 *)qedi->bdq_pbl_list;
> page = qedi->bdq_pbl_list_dma;
> for (i = 0; i < qedi->bdq_pbl_list_num_entries; i++) {
> *list = qedi->bdq_pbl_dma;
> list++;
> - page += PAGE_SIZE;
> + page += QEDI_PAGE_SIZE;
> }
>
> return 0;
>
Reviewed-by: Lee Duncan
Re: [PATCH 1/8] qedi: Cleanup redundant QEDI_PAGE_SIZE macro definition
On 11/20/18 2:37 AM, Nilesh Javali wrote: > Remove redundant macro definition. > > Signed-off-by: Nilesh Javali > --- > drivers/scsi/qedi/qedi.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h > index a6f96b3..e966855 100644 > --- a/drivers/scsi/qedi/qedi.h > +++ b/drivers/scsi/qedi/qedi.h > @@ -64,11 +64,9 @@ > #define TX_RX_RING 16 > #define RX_RING (TX_RX_RING - 1) > #define LL2_SINGLE_BUF_SIZE 0x400 > -#define QEDI_PAGE_SIZE 4096 > #define QEDI_PAGE_ALIGN(addr)ALIGN(addr, QEDI_PAGE_SIZE) > #define QEDI_PAGE_MASK (~((QEDI_PAGE_SIZE) - 1)) > > -#define QEDI_PAGE_SIZE 4096 > #define QEDI_HW_DMA_BOUNDARY 0xfff > #define QEDI_PATH_HANDLE 0xFE000UL > > Reviewed-by: Lee Duncan
Re: [PATCH 2/8] qedi: Fix spelling mistake "OUSTANDING" -> "OUTSTANDING"
On 11/20/18 2:37 AM, Nilesh Javali wrote:
> Fix trivial spelling mistake within macro definition.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi.h | 4 ++--
> drivers/scsi/qedi/qedi_main.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h
> index e966855..6fa02c5 100644
> --- a/drivers/scsi/qedi/qedi.h
> +++ b/drivers/scsi/qedi/qedi.h
> @@ -45,7 +45,7 @@
> #define QEDI_MAX_TASK_NUM0x0FFF
> #define QEDI_MAX_ISCSI_CONNS_PER_HBA 1024
> #define QEDI_ISCSI_MAX_BDS_PER_CMD 255 /* Firmware max BDs is 255 */
> -#define MAX_OUSTANDING_TASKS_PER_CON 1024
> +#define MAX_OUTSTANDING_TASKS_PER_CON1024
>
> #define QEDI_MAX_BD_LEN 0x
> #define QEDI_BD_SPLIT_SZ 0x1000
> @@ -144,7 +144,7 @@ struct skb_work_list {
> };
>
> /* Queue sizes in number of elements */
> -#define QEDI_SQ_SIZE MAX_OUSTANDING_TASKS_PER_CON
> +#define QEDI_SQ_SIZE MAX_OUTSTANDING_TASKS_PER_CON
> #define QEDI_CQ_SIZE 2048
> #define QEDI_CMDQ_SIZE QEDI_MAX_ISCSI_TASK
> #define QEDI_PROTO_CQ_PROD_IDX 0
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 105b0e4..0f8eb5f 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -796,7 +796,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> int rval = 0;
>
>
> - num_sq_pages = (MAX_OUSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
> + num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
>
> qedi->num_queues = MIN_NUM_CPUS_MSIX(qedi);
>
>
Reviewed-by: Lee Duncan
Re: [RESEND] TCMUser: add read length support
On 05/24/2018 09:49 AM, Bodo Stroesser wrote: > This is patch version 3. > > Generally target core and TCMUser seem to work fine for tape devices and > media changers. > But there is at least one situation, where TCMUser is not able to support > sequential access device emulation correctly. > > ... Looks good now. Reviewed-by: Lee Duncan <ldun...@suse.com> -- Lee Duncan SUSE Labs
[PATCH v4] target: transport should handle st FM/EOM/ILI reads
When a tape drive is exported via LIO using the pscsi module, a read that
requests more bytes per block than the tape can supply returns an empty
buffer. This is because the pscsi pass-through target module sees the
"ILI" illegal length bit set and thinks there is no reason to return
the data.
This is a long-standing transport issue, since it assumes that no data
need be returned under a check condition, which isn't always the case
for tape.
Add in a check for tape reads with the ILI, EOM, or FM bits set,
with a sense code of NO_SENSE, treating such cases as if the read
succeeded. The layered tape driver then "does the right thing" when
it gets such a response.
Changes from v3:
- cleaned up comment
- Added residual handling
Changes from v2:
- Cleaned up subject line and bug text formatting
- Removed unneeded inner braces
- Removed ugly goto
- Also updated the "queue full" path to handle this case
Changes from RFC:
- Moved ugly code from transport to pscsi module
- Added checking EOM and FM bits, as well as ILI
- fixed malformed patch
- Clarified description a bit
Signed-off-by: Lee Duncan <ldun...@suse.com>
Signed-off-by: Bodo Stroesser <bstroes...@ts.fujitsu.com>)
Reviewed-by: Hannes Reinecke <h...@suse.com>
---
drivers/target/target_core_pscsi.c | 26 ++--
drivers/target/target_core_transport.c | 43 +-
include/target/target_core_base.h | 1 +
3 files changed, 62 insertions(+), 8 deletions(-)
diff --git a/drivers/target/target_core_pscsi.c
b/drivers/target/target_core_pscsi.c
index 0d99b242e82e..f31215b1d009 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -689,8 +689,29 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8
scsi_status,
}
after_mode_select:
- if (scsi_status == SAM_STAT_CHECK_CONDITION)
+ if (scsi_status == SAM_STAT_CHECK_CONDITION) {
transport_copy_sense_to_cmd(cmd, req_sense);
+
+ /*
+* check for TAPE device reads with
+* FM/EOM/ILI set, so that we can get data
+* back despite framework assumption that a
+* check condition means there is no data
+*/
+ if (sd->type == TYPE_TAPE &&
+ cmd->data_direction == DMA_FROM_DEVICE) {
+ /*
+* is sense data valid, fixed format,
+* and have FM, EOM, or ILI set?
+*/
+ if (req_sense[0] == 0xf0 && /* valid, fixed format
*/
+ req_sense[2] & 0xe0 && /* FM, EOM, or ILI */
+ (req_sense[2] & 0xf) == 0) { /* key==NO_SENSE */
+ pr_debug("Tape FM/EOM/ILI status detected.
Treat as normal read.\n");
+ cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
+ }
+ }
+ }
}
enum {
@@ -1061,7 +1082,8 @@ static void pscsi_req_done(struct request *req,
blk_status_t status)
switch (host_byte(result)) {
case DID_OK:
- target_complete_cmd(cmd, scsi_status);
+ target_complete_cmd_with_length(cmd, scsi_status,
+ cmd->data_length - scsi_req(req)->resid_len);
break;
default:
pr_debug("PSCSI Host Byte exception at cmd: %p CDB:"
diff --git a/drivers/target/target_core_transport.c
b/drivers/target/target_core_transport.c
index 74b646f165d4..791ff9ba2bc6 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -779,7 +779,9 @@ EXPORT_SYMBOL(target_complete_cmd);
void target_complete_cmd_with_length(struct se_cmd *cmd, u8 scsi_status, int
length)
{
- if (scsi_status == SAM_STAT_GOOD && length < cmd->data_length) {
+ if ((scsi_status == SAM_STAT_GOOD ||
+cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) &&
+ length < cmd->data_length) {
if (cmd->se_cmd_flags & SCF_UNDERFLOW_BIT) {
cmd->residual_count += cmd->data_length - length;
} else {
@@ -2084,12 +2086,24 @@ static void transport_complete_qf(struct se_cmd *cmd)
goto queue_status;
}
- if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE)
+ /*
+* Check if we need to send a sense buffer from
+* the struct se_cmd in question. We do NOT want
+* to take this path of the IO has been marked as
+* needing to be treated like a "normal read". This
+* is the case if it's a tape read, and either the
+* FM, EOM, or ILI bits are set, but there is no
+* sense data.
+
[PATCH v3] target: transport should allow st FM/EOM/ILI reads
When a tape drive is exported via LIO using the pscsi module, a read that
requests more bytes per block than the tape can supply returns an empty
buffer. This is because the pscsi pass-through target module sees the
"ILI" illegal length bit set and thinks there is no reason to return
the data.
This is a long-standing transport issue, since it assumes that no data
need be returned under a check condition, which isn't always the case
for tape.
Add in a check for tape reads with the ILI, EOM, or FM bits set,
with a sense code of NO_SENSE, treating such cases as if the read
succeeded. The layered tape driver then "does the right thing" when
it gets such a response.
Changes from v2:
- Cleaned up subject line and bug text formatting
- Removed unneeded inner braces
- Removed ugly goto
- Also updated the "queue full" path to handle this case
Changes from RFC:
- Moved ugly code from transport to pscsi module
- Added checking EOM and FM bits, as well as ILI
- fixed malformed patch
- Clarified description a bit
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/target/target_core_pscsi.c | 23 +++-
drivers/target/target_core_transport.c | 39 +-
include/target/target_core_base.h | 1 +
3 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/drivers/target/target_core_pscsi.c
b/drivers/target/target_core_pscsi.c
index 0d99b242e82e..a9656368a96d 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -689,8 +689,29 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8
scsi_status,
}
after_mode_select:
- if (scsi_status == SAM_STAT_CHECK_CONDITION)
+ if (scsi_status == SAM_STAT_CHECK_CONDITION) {
transport_copy_sense_to_cmd(cmd, req_sense);
+
+ /*
+* hack to check for TAPE device reads with
+* FM/EOM/ILI set, so that we can get data
+* back despite framework assumption that a
+* check condition means there is no data
+*/
+ if (sd->type == TYPE_TAPE &&
+ cmd->data_direction == DMA_FROM_DEVICE) {
+ /*
+* is sense data valid, fixed format,
+* and have FM, EOM, or ILI set?
+*/
+ if (req_sense[0] == 0xf0 && /* valid, fixed format
*/
+ req_sense[2] & 0xe0 && /* FM, EOM, or ILI */
+ (req_sense[2] & 0xf) == 0) { /* key==NO_SENSE */
+ pr_debug("Tape FM/EOM/ILI status detected.
Treat as normal read.\n");
+ cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
+ }
+ }
+ }
}
enum {
diff --git a/drivers/target/target_core_transport.c
b/drivers/target/target_core_transport.c
index 74b646f165d4..a15a9e3dce11 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2084,12 +2084,24 @@ static void transport_complete_qf(struct se_cmd *cmd)
goto queue_status;
}
- if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE)
+ /*
+* Check if we need to send a sense buffer from
+* the struct se_cmd in question. We do NOT want
+* to take this path of the IO has been marked as
+* needing to be treated like a "normal read". This
+* is the case if it's a tape read, and either the
+* FM, EOM, or ILI bits are set, but there is no
+* sense data.
+*/
+ if (!(cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) &&
+ cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE)
goto queue_status;
switch (cmd->data_direction) {
case DMA_FROM_DEVICE:
- if (cmd->scsi_status)
+ /* queue status if not treating this as a normal read */
+ if (cmd->scsi_status &&
+ !(cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL))
goto queue_status;
trace_target_cmd_complete(cmd);
@@ -2194,9 +2206,15 @@ static void target_complete_ok_work(struct work_struct
*work)
/*
* Check if we need to send a sense buffer from
-* the struct se_cmd in question.
+* the struct se_cmd in question. We do NOT want
+* to take this path of the IO has been marked as
+* needing to be treated like a "normal read". This
+* is the case if it's a tape read, and either the
+* FM, EOM, or ILI bits are set, but there is no
+* sense data.
*/
- if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE) {
+ if (!(cmd-&
Re: [PATCH v2] target: transport should allow st ILI reads
On 05/10/2018 12:19 PM, Mike Christie wrote: > On 05/10/2018 01:51 PM, Mike Christie wrote: >> On 05/09/2018 03:59 PM, Lee Duncan wrote: >>> When a tape drive is exported via LIO using the >>> pscsi module, a read that requests more bytes per block >>> than the tape can supply returns an empty buffer. This >>> is because the pscsi pass-through target module sees >>> the "ILI" illegal length bit set and thinks there >>> is no reason to return the data. >>> >>> This is a long-standing transport issue, since it >>> assume that no data need be returned under a check >>> condition, which isn't always the case for tape. >>> >>> Add in a check for tape reads with the the ILI, EOM, >>> or FM bits set, with a sense code of NO_SENSE, >>> treating such cases as if there is no sense data >>> and the read succeeded. The layered tape driver then >>> "does the right thing" when it gets such a response. >>> >>> Changes from RFC: >>> - Moved ugly code from transport to pscsi module >>> - Added checking EOM and FM bits, as well as ILI >>> - fixed malformed patch >>> - Clarified description a bit >>> >>> Signed-off-by: Lee Duncan <ldun...@suse.com> >>> --- >>> drivers/target/target_core_pscsi.c | 22 +- >>> drivers/target/target_core_transport.c | 6 ++ >>> include/target/target_core_base.h | 1 + >>> 3 files changed, 28 insertions(+), 1 deletion(-) >>> >>> ... >> >> >> Do you want to return both the data and sense or just one or the other? >> Both right? In this code path we would return both the data and sense >> for drivers like iscsi. Yes, both the sense data and the IO data get returned with this patch. >> >> If the queue_data_in call a little below this line returns >> -ENOMEM/-EAGAIN then I think the queue full handling is going to end up >> only returning the sense like in your original bug. You would need a >> similar change to transport_complete_qf so it returns the data. Good point. I will update the patch. >> > > Oh yeah, one other question/comment. The above code is bypassing the > normal sense sending code so SCF_SENT_CHECK_CONDITION is not going to be > set. For iscsi could you end up where 2 paths complete the same command > because a reassign could race with one of these completions? > Good question. I believe it will not be an issue. I believe that if the IO completes on multiple paths it will be treated the same on each path, which means that the SCF_SENT_CHECK_CONDITION will not be set on either path. Which is how it is handled now for a normal IO. -- Lee
[PATCH v2] target: transport should allow st ILI reads
When a tape drive is exported via LIO using the
pscsi module, a read that requests more bytes per block
than the tape can supply returns an empty buffer. This
is because the pscsi pass-through target module sees
the "ILI" illegal length bit set and thinks there
is no reason to return the data.
This is a long-standing transport issue, since it
assume that no data need be returned under a check
condition, which isn't always the case for tape.
Add in a check for tape reads with the the ILI, EOM,
or FM bits set, with a sense code of NO_SENSE,
treating such cases as if there is no sense data
and the read succeeded. The layered tape driver then
"does the right thing" when it gets such a response.
Changes from RFC:
- Moved ugly code from transport to pscsi module
- Added checking EOM and FM bits, as well as ILI
- fixed malformed patch
- Clarified description a bit
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/target/target_core_pscsi.c | 22 +-
drivers/target/target_core_transport.c | 6 ++
include/target/target_core_base.h | 1 +
3 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/target/target_core_pscsi.c
b/drivers/target/target_core_pscsi.c
index 0d99b242e82e..b237104af81c 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -689,8 +689,28 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8
scsi_status,
}
after_mode_select:
- if (scsi_status == SAM_STAT_CHECK_CONDITION)
+ if (scsi_status == SAM_STAT_CHECK_CONDITION) {
transport_copy_sense_to_cmd(cmd, req_sense);
+
+ /*
+* hack to check for TAPE device reads with
+* FM/EOM/ILI set, so that we can get data
+* back despite framework assumption that a
+* check condition means there is no data
+*/
+ if ((sd->type == TYPE_TAPE) &&
+ (cmd->data_direction == DMA_FROM_DEVICE)) {
+ /*
+* is sense data valid, fixed format,
+* and have FM, EOM, or ILI set?
+*/
+ if ((req_sense[0] == 0xf0) && /* valid, fixed format
*/
+ (req_sense[2] & 0xe0) &&/* FM, EOM, or ILI */
+ ((req_sense[2] & 0xf) == 0)) { /* key==NO_SENSE */
+ cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
+ }
+ }
+ }
}
enum {
diff --git a/drivers/target/target_core_transport.c
b/drivers/target/target_core_transport.c
index 74b646f165d4..56661a824266 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2192,6 +2192,11 @@ static void target_complete_ok_work(struct work_struct
*work)
if (atomic_read(>se_dev->dev_qf_count) != 0)
schedule_work(>se_dev->qf_work_queue);
+ if (cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) {
+ pr_debug("Tape FM/EOM/ILI status detected. Treating as OK\n");
+ goto treat_as_normal_read;
+ }
+
/*
* Check if we need to send a sense buffer from
* the struct se_cmd in question.
@@ -2241,6 +2246,7 @@ static void target_complete_ok_work(struct work_struct
*work)
if (cmd->scsi_status)
goto queue_status;
+treat_as_normal_read:
atomic_long_add(cmd->data_length,
>se_lun->lun_stats.tx_data_octets);
/*
diff --git a/include/target/target_core_base.h
b/include/target/target_core_base.h
index 9f9f5902af38..922a39f45abc 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -143,6 +143,7 @@ enum se_cmd_flags_table {
SCF_ACK_KREF= 0x0040,
SCF_USE_CPUID = 0x0080,
SCF_TASK_ATTR_SET = 0x0100,
+ SCF_TREAT_READ_AS_NORMAL= 0x0200,
};
/*
--
2.13.6
Re: sr: get/drop reference to device in revalidate and check_events
On 04/11/2018 09:23 AM, Jens Axboe wrote:
> We can't just use scsi_cd() to get the scsi_cd structure, we have
> to grab a live reference to the device. For both callbacks, we're
> not inside an open where we already hold a reference to the device.
>
> This fixes device removal/addition under concurrent device access,
> which otherwise could result in the below oops.
>
> NULL pointer dereference at 0010
> PGD 0 P4D 0
> Oops: [#1] PREEMPT SMP
> Modules linked in:
> sr 12:0:0:0: [sr2] scsi-1 drive
> scsi_debug crc_t10dif crct10dif_generic crct10dif_common nvme nvme_core
> sb_edac xl
> sr 12:0:0:0: Attached scsi CD-ROM sr2
> sr_mod cdrom btrfs xor zstd_decompress zstd_compress xxhash lzo_compress
> zlib_defc
> sr 12:0:0:0: Attached scsi generic sg7 type 5
> igb ahci libahci i2c_algo_bit libata dca [last unloaded: crc_t10dif]
> CPU: 43 PID: 4629 Comm: systemd-udevd Not tainted 4.16.0+ #650
> Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016
> RIP: 0010:sr_block_revalidate_disk+0x23/0x190 [sr_mod]
> RSP: 0018:883ff357bb58 EFLAGS: 00010292
> RAX: a00b07d0 RBX: 883ff3058000 RCX: 883ff357bb66
> RDX: 0003 RSI: 7530 RDI: 881fea631000
> RBP: R08: 881fe4d38400 R09:
> R10: R11: 01b6 R12: 085d
> R13: 085d R14: 883ffd9b3790 R15:
> FS: 7f7dc8e6d8c0() GS:883fff34() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: 0010 CR3: 003ffda98005 CR4: 003606e0
> DR0: DR1: DR2:
> DR3: DR6: fffe0ff0 DR7: 0400
> Call Trace:
> ? __invalidate_device+0x48/0x60
> check_disk_change+0x4c/0x60
> sr_block_open+0x16/0xd0 [sr_mod]
> __blkdev_get+0xb9/0x450
> ? iget5_locked+0x1c0/0x1e0
> blkdev_get+0x11e/0x320
> ? bdget+0x11d/0x150
> ? _raw_spin_unlock+0xa/0x20
> ? bd_acquire+0xc0/0xc0
> do_dentry_open+0x1b0/0x320
> ? inode_permission+0x24/0xc0
> path_openat+0x4e6/0x1420
> ? cpumask_any_but+0x1f/0x40
> ? flush_tlb_mm_range+0xa0/0x120
> do_filp_open+0x8c/0xf0
> ? __seccomp_filter+0x28/0x230
> ? _raw_spin_unlock+0xa/0x20
> ? __handle_mm_fault+0x7d6/0x9b0
> ? list_lru_add+0xa8/0xc0
> ? _raw_spin_unlock+0xa/0x20
> ? __alloc_fd+0xaf/0x160
> ? do_sys_open+0x1a6/0x230
> do_sys_open+0x1a6/0x230
> do_syscall_64+0x5a/0x100
> entry_SYSCALL_64_after_hwframe+0x3d/0xa2
>
> Signed-off-by: Jens Axboe <ax...@kernel.dk>
>
> ---
>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 0cf25d789d05..3f3cb72e0c0c 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -587,18 +587,28 @@ static int sr_block_ioctl(struct block_device *bdev,
> fmode_t mode, unsigned cmd,
> static unsigned int sr_block_check_events(struct gendisk *disk,
> unsigned int clearing)
> {
> - struct scsi_cd *cd = scsi_cd(disk);
> + unsigned int ret = 0;
> + struct scsi_cd *cd;
>
> - if (atomic_read(>device->disk_events_disable_depth))
> + cd = scsi_cd_get(disk);
> + if (!cd)
> return 0;
>
> - return cdrom_check_events(>cdi, clearing);
> + if (!atomic_read(>device->disk_events_disable_depth))
> + ret = cdrom_check_events(>cdi, clearing);
> +
> + scsi_cd_put(cd);
> + return ret;
> }
>
> static int sr_block_revalidate_disk(struct gendisk *disk)
> {
> - struct scsi_cd *cd = scsi_cd(disk);
> struct scsi_sense_hdr sshdr;
> + struct scsi_cd *cd;
> +
> + cd = scsi_cd_get(disk);
> + if (!cd)
> + return -ENXIO;
>
> /* if the unit is not ready, nothing more to do */
> if (scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, ))
> @@ -607,6 +617,7 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
> sr_cd_check(>cdi);
> get_sectorsize(cd);
> out:
> + scsi_cd_put(cd);
> return 0;
> }
>
>
Reviewed-by: Lee Duncan <ldun...@suse.com>
Re: [PATCH] iscsi: respond to netlink with unicast when appropriate
On 04/09/2018 03:15 PM, Chris Leech wrote: > Instead of always multicasting responses, send a unicast netlink message > directed at the correct pid. This will be needed if we ever want to > support multiple userspace processes interacting with the kernel over > iSCSI netlink simultaneously. Limitations can currently be seen if you > attempt to run multiple iscsistart commands in parallel. > > We've fixed up the userspace issues in iscsistart that prevented > multiple instances from running, so now attempts to speed up booting by > bringing up multiple iscsi sessions at once in the initramfs are just > running into misrouted responses that this fixes. As you may know, I disagree with running multiple iscsistart-s at the same time, since that's what iscsid is for. Never the less, I believe we _should_ be able to have multiple processes talking to the kernel target code, so I agree with these changes. > > Signed-off-by: Chris Leech <cle...@redhat.com> > --- > drivers/scsi/scsi_transport_iscsi.c | 29 ++--- > 1 file changed, 18 insertions(+), 11 deletions(-) > > ... (diffs removed to save electrons) > Reviewed-by: Lee Duncan <ldun...@suse.com> -- Lee Duncan SUSE Labs
[PATCHv2] target: prefer dbroot of /etc/target over /var/target
The target database root directory, dbroot, has
defaulted to /var/target for a while, but its
main client, targetcli-fb, has been moving it
to /etc/target for quite some time. With the
plethora of target drivers now appearing, it has
become more difficult to initialize this attribute
before use by any child drivers.
If the directory /etc/target exists, use that as
the DB root. Otherwise, fall back to using
/var/target.
The ability to override this dbroot attribute
still exists via sysfs.
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/target/target_core_configfs.c | 25 +
drivers/target/target_core_internal.h | 1 +
2 files changed, 26 insertions(+)
diff --git a/drivers/target/target_core_configfs.c
b/drivers/target/target_core_configfs.c
index 3f4bf126eed0..5ccef7d597fa 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -155,6 +155,8 @@ static ssize_t target_core_item_dbroot_store(struct
config_item *item,
mutex_unlock(_tf_lock);
+ pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
+
return read_bytes;
}
@@ -3213,6 +3215,27 @@ void target_setup_backend_cits(struct target_backend *tb)
target_core_setup_dev_stat_cit(tb);
}
+static void target_init_dbroot(void)
+{
+ struct file *fp;
+
+ snprintf(db_root_stage, DB_ROOT_LEN, DB_ROOT_PREFERRED);
+ fp = filp_open(db_root_stage, O_RDONLY, 0);
+ if (IS_ERR(fp)) {
+ pr_err("db_root: cannot open: %s\n", db_root_stage);
+ return;
+ }
+ if (!S_ISDIR(file_inode(fp)->i_mode)) {
+ filp_close(fp, NULL);
+ pr_err("db_root: not a valid directory: %s\n", db_root_stage);
+ return;
+ }
+ filp_close(fp, NULL);
+
+ strncpy(db_root, db_root_stage, DB_ROOT_LEN);
+ pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
+}
+
static int __init target_core_init_configfs(void)
{
struct configfs_subsystem *subsys = _core_fabrics;
@@ -3293,6 +3316,8 @@ static int __init target_core_init_configfs(void)
if (ret < 0)
goto out;
+ target_init_dbroot();
+
return 0;
out:
diff --git a/drivers/target/target_core_internal.h
b/drivers/target/target_core_internal.h
index 1d5afc3ae017..dead30b1d32c 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -166,6 +166,7 @@ extern struct se_portal_group xcopy_pt_tpg;
/* target_core_configfs.c */
#define DB_ROOT_LEN4096
#defineDB_ROOT_DEFAULT "/var/target"
+#defineDB_ROOT_PREFERRED "/etc/target"
extern char db_root[];
--
2.13.6
Re: [PATCH] target: prefer dbroot of /etc/target over /var/target
I will be re-sending this, as this version somehow missed
a change to an include file.
On 04/06/2018 08:37 AM, Lee Duncan wrote:
> The target database root directory, dbroot, has
> defaulted to /var/target for a while, but its
> main client, targetcli-fb, has been moving it
> to /etc/target for quite some time. With the
> plethora of target drivers now appearing, it has
> become more difficult to initialize this attribute
> before use by any child drivers.
>
> If the directory /etc/target exists, use that as
> the DB root. Otherwise, fall back to using
> /var/target.
>
> The ability to override this dbroot attribute
> still exists via sysfs.
>
> Signed-off-by: Lee Duncan <ldun...@suse.com>
> ---
> drivers/target/target_core_configfs.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/target/target_core_configfs.c
> b/drivers/target/target_core_configfs.c
> index 3f4bf126eed0..5ccef7d597fa 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -155,6 +155,8 @@ static ssize_t target_core_item_dbroot_store(struct
> config_item *item,
>
> mutex_unlock(_tf_lock);
>
> + pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
> +
> return read_bytes;
> }
>
> @@ -3213,6 +3215,27 @@ void target_setup_backend_cits(struct target_backend
> *tb)
> target_core_setup_dev_stat_cit(tb);
> }
>
> +static void target_init_dbroot(void)
> +{
> + struct file *fp;
> +
> + snprintf(db_root_stage, DB_ROOT_LEN, DB_ROOT_PREFERRED);
> + fp = filp_open(db_root_stage, O_RDONLY, 0);
> + if (IS_ERR(fp)) {
> + pr_err("db_root: cannot open: %s\n", db_root_stage);
> + return;
> + }
> + if (!S_ISDIR(file_inode(fp)->i_mode)) {
> + filp_close(fp, NULL);
> + pr_err("db_root: not a valid directory: %s\n", db_root_stage);
> + return;
> + }
> + filp_close(fp, NULL);
> +
> + strncpy(db_root, db_root_stage, DB_ROOT_LEN);
> + pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
> +}
> +
> static int __init target_core_init_configfs(void)
> {
> struct configfs_subsystem *subsys = _core_fabrics;
> @@ -3293,6 +3316,8 @@ static int __init target_core_init_configfs(void)
> if (ret < 0)
> goto out;
>
> + target_init_dbroot();
> +
> return 0;
>
> out:
>
--
Lee Duncan
SUSE Labs
[PATCH] target: prefer dbroot of /etc/target over /var/target
The target database root directory, dbroot, has
defaulted to /var/target for a while, but its
main client, targetcli-fb, has been moving it
to /etc/target for quite some time. With the
plethora of target drivers now appearing, it has
become more difficult to initialize this attribute
before use by any child drivers.
If the directory /etc/target exists, use that as
the DB root. Otherwise, fall back to using
/var/target.
The ability to override this dbroot attribute
still exists via sysfs.
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/target/target_core_configfs.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/drivers/target/target_core_configfs.c
b/drivers/target/target_core_configfs.c
index 3f4bf126eed0..5ccef7d597fa 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -155,6 +155,8 @@ static ssize_t target_core_item_dbroot_store(struct
config_item *item,
mutex_unlock(_tf_lock);
+ pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
+
return read_bytes;
}
@@ -3213,6 +3215,27 @@ void target_setup_backend_cits(struct target_backend *tb)
target_core_setup_dev_stat_cit(tb);
}
+static void target_init_dbroot(void)
+{
+ struct file *fp;
+
+ snprintf(db_root_stage, DB_ROOT_LEN, DB_ROOT_PREFERRED);
+ fp = filp_open(db_root_stage, O_RDONLY, 0);
+ if (IS_ERR(fp)) {
+ pr_err("db_root: cannot open: %s\n", db_root_stage);
+ return;
+ }
+ if (!S_ISDIR(file_inode(fp)->i_mode)) {
+ filp_close(fp, NULL);
+ pr_err("db_root: not a valid directory: %s\n", db_root_stage);
+ return;
+ }
+ filp_close(fp, NULL);
+
+ strncpy(db_root, db_root_stage, DB_ROOT_LEN);
+ pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
+}
+
static int __init target_core_init_configfs(void)
{
struct configfs_subsystem *subsys = _core_fabrics;
@@ -3293,6 +3316,8 @@ static int __init target_core_init_configfs(void)
if (ret < 0)
goto out;
+ target_init_dbroot();
+
return 0;
out:
--
2.13.6
Re: [PATCH] target: change default dbroot to /etc/target
On 04/05/2018 10:57 AM, Christoph Hellwig wrote: > On Thu, Apr 05, 2018 at 10:06:35AM -0700, Lee Duncan wrote: >> Also, I think it's a bit unlikely that anyone will still be using >> /var/target, since targetcli-fb has been setting the target root to >> /etc/target for a while now, and the old targetcli has been deprecated. >> (It's the only app I know that hard-codes the old location.) > > That is a a good point. How about having a search path in the kernel? > Try the configs attribute first if one is set, then /etc/target, then > /var/target? > Good suggestion. But the configfs path is only passed in optionally, and possibly much after initialization, so there's no way the code can check the configfs path at initialization time, when this decision is first made. How about if the code looks for /etc/target, then uses /var/target if /etc/target is not present? Then users can use the current sysfs logic to set the path to any other directory, if neither of these paths are to their liking? And users can always find out the current dbroot with the sysfs attribute. -- Lee Duncan SUSE Labs
Re: [PATCH] target: change default dbroot to /etc/target
On 04/05/2018 12:17 AM, Christoph Hellwig wrote: > On Wed, Apr 04, 2018 at 12:47:03PM -0700, Lee Duncan wrote: >> The dbroot (target PR database root directory) is >> configurable but default to /var/target, a historic >> value. But the reason for adding configurability >> was to move the target directory out of /var. This >> is because the File Hierarchy Standard v3.0 mandates >> that this "target" directory not be in /var. See >> https://refspecs.linuxfoundation.org/FHS_3.0/fhs-3.0.pdf >> >> This change moves the default from /var/target to >> /etc/target, but this value is still configurable, >> so those wishing to continue to use /var/target >> can still do so. > > This will break upgrades of all existing setups, so NAK. > Hi Christoph: I half expected this response, but here's the problem with the current approach ... The "dbroot" attribute is managed by target_core_mod, but this module is generally never loaded directly. Usually it is loaded because some other module (like ib_isert) is loaded, and it depends on target_core_mod. So if a user starts up rdma services, for example, they end up with a target_core_mod that will not allow dbroot to be changed. This is because the dbroot change rules will not allow a change when there are any children modules. [This problem will continue to grow as we get other target-mode drivers, such as nvme?] So setting the dbroot for all target driver requires some entity to load target_core_mod and set dbroot before any other module that uses target_core_mod loads. This means either we need a special service, at boot time, to load the core driver and set dbroot, so that all target drivers are treated equally, or we put the burden on every target driver to do this setup, which seems like an unreasonable duplication of effort. Bottom line, we need a better way to set dbroot. This is true no matter what the default value is for this attribute. I understand your reluctance to change dbroot, but I also see this leading to /var/target being the default location from now on unless we change it some time. I'm open to suggestions on how we could update this value and not break existing systems. A simple shell script could move things from /var/target to /etc/target, but bare kernels have no way to invoke "upgrade" scripts that I know of. Also, I think it's a bit unlikely that anyone will still be using /var/target, since targetcli-fb has been setting the target root to /etc/target for a while now, and the old targetcli has been deprecated. (It's the only app I know that hard-codes the old location.) For now, I believe a workable solution may be to add "dbroot" as a module parameter? If dbroot was a module param for target_core_mod, then dbroot=/etc/target could be passed in by default (via modprobe.d) for distros that wish to change the location. I will work on such a patch, but if you dislike this idea please feel free to save me some work, and let me know. -- Lee Duncan SUSE Labs
Re: [PATCH v2] Fix DID_OK handling in __scsi_error_from_host_byte()
On 04/04/2018 10:53 AM, Bart Van Assche wrote:
> Commit e39a97353e53 modified __scsi_error_from_host_byte() such
> that that function translates DID_OK into BLK_STS_OK. However,
> the description of that commit is wrong: it mentions that commit
> 2a842acab109 introduced a bug in __scsi_error_from_host_byte()
> although that commit did not change the behavior of that function.
> Additionally, commit e39a97353e53 introduced a severe bug: it causes
> commands that fail with hostbyte=DID_OK and driverbyte=DRIVER_SENSE
> to be completed with BLK_STS_OK. Fix __scsi_error_from_host_byte()
> by only translating good status values into BLK_STS_OK.
>
> Fixes: e39a97353e53 ("scsi: core: return BLK_STS_OK for DID_OK in
> __scsi_error_from_host_byte()")
> Reported-by: Damien Le Moal <damien.lem...@wdc.com>
> Signed-off-by: Bart Van Assche <bart.vanass...@wdc.com>
> Cc: Hannes Reinecke <h...@suse.com>
> Cc: Douglas Gilbert <dgilb...@interlog.com>
> Cc: Damien Le Moal <damien.lem...@wdc.com>
> Cc: Christoph Hellwig <h...@lst.de>
> Cc: sta...@vger.kernel.org
> ---
>
> Changes compared to v1:
> - Modified __scsi_error_from_host_byte() such that it again returns
> BLK_STS_OK for CONDITION MET and other result codes that represent
> success.
>
> drivers/scsi/scsi_lib.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 74a39db57d49..1496b34af409 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -736,7 +736,13 @@ static blk_status_t __scsi_error_from_host_byte(struct
> scsi_cmnd *cmd,
> {
> switch (host_byte(result)) {
> case DID_OK:
> - return BLK_STS_OK;
> + /*
> + * Also check the other bytes than the status byte in result
> + * to handle the case when a SCSI LLD sets result to
> + * DRIVER_SENSE << 24 without setting SAM_STAT_CHECK_CONDITION.
> + */
> + return scsi_status_is_good(result) && (result & ~0xff) == 0 ?
> + BLK_STS_OK : BLK_STS_IOERR;
> case DID_TRANSPORT_FAILFAST:
> return BLK_STS_TRANSPORT;
> case DID_TARGET_FAILURE:
>
Reviewed-by: Lee Duncan <ldun...@suse.com>
[PATCH] target: change default dbroot to /etc/target
The dbroot (target PR database root directory) is configurable but default to /var/target, a historic value. But the reason for adding configurability was to move the target directory out of /var. This is because the File Hierarchy Standard v3.0 mandates that this "target" directory not be in /var. See https://refspecs.linuxfoundation.org/FHS_3.0/fhs-3.0.pdf This change moves the default from /var/target to /etc/target, but this value is still configurable, so those wishing to continue to use /var/target can still do so. Signed-off-by: Lee Duncan <ldun...@suse.com> --- drivers/target/target_core_internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h index 1d5afc3ae017..34eccef975b7 100644 --- a/drivers/target/target_core_internal.h +++ b/drivers/target/target_core_internal.h @@ -165,7 +165,7 @@ extern struct se_portal_group xcopy_pt_tpg; /* target_core_configfs.c */ #define DB_ROOT_LEN4096 -#defineDB_ROOT_DEFAULT "/var/target" +#defineDB_ROOT_DEFAULT "/etc/target" extern char db_root[]; -- 2.13.6
Re: [PATCH] scsi: iscsi_tcp: set BDI_CAP_STABLE_WRITES when data digest enabled
I reviewed this several days before but mistakenly replied only to the open-iscsi list. Signed-off-by: Lee Duncan <ldun...@suse.com> -- Lee-Man Duncan Sent from my iPhone, dude > On Mar 14, 2018, at 10:11 PM, Martin K. Petersen <martin.peter...@oracle.com> > wrote: > > >> iscsi tcp will first send out data, then calculate and send data >> digest. If we don't have BDI_CAP_STABLE_WRITES, the page cache will >> be written in spite of the on going writeback. Consequently, wrong >> digest will be got and sent to target. >> >> To fix this, set BDI_CAP_STABLE_WRITES when data digest is enabled >> in iscsi_tcp .slave_configure callback. > > Lee, Chris: Please review! > > -- > Martin K. PetersenOracle Linux Engineering > > -- > You received this message because you are subscribed to a topic in the Google > Groups "open-iscsi" group. > To unsubscribe from this topic, visit > https://groups.google.com/d/topic/open-iscsi/owLIZAXfgoA/unsubscribe. > To unsubscribe from this group and all its topics, send an email to > open-iscsi+unsubscr...@googlegroups.com. > To post to this group, send email to open-is...@googlegroups.com. > Visit this group at https://groups.google.com/group/open-iscsi. > For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] libiscsi: ensure session spin lock usage consistent
On 02/07/2018 08:36 PM, Chris Leech wrote: > On Mon, Feb 05, 2018 at 11:13:23AM -0800, Lee Duncan wrote: >> The libiscsi code was using both spin_lock()/spin_unlock() >> and spin_lock_bh()/spin_unlock_bh() on its session lock. >> In addition, lock validation found that libiscsi.c was >> taking a HARDIRQ-unsafe lock while holding an HARDIRQ- >> irq-safe lock: > > Lee and I have exchanged a few mails off-list, but I don't think these > changes are the right thing to address the lockdep warning here. > > I've managed to reproduce this by exporting a scsi_debug lun over iSCSI > and running fio traffic on a lockdep enabled initiator. > > The deadlock that lockdep is worried about is: > >> Possible interrupt unsafe locking scenario: >> >>CPU0CPU1 >> >> lock(&(>frwd_lock)->rlock); >>local_irq_disable(); >>lock(&(>__queue_lock)->rlock); >>lock(&(>frwd_lock)->rlock); >> >> lock(&(>__queue_lock)->rlock); >> >> *** DEADLOCK *** > > Which I don't think can actually happen unless the queue_lock is being > taken in hardirq context by any of the iSCSI drivers, but lockdep can't > know that. But it does make me wonder if we can do less in > iscsi_eh_cmd_timed_out while interrupts are blocked. > > Lee, I'm going to test with your patch applied and see what lockdep > kicks out. Did you get a chance to run lockdep with the patch set? I have been looking at this in *much* more depth, to try to really understand the problem if any. And I believe you are right, that this deadlock could not happen because the block timmeout routine where the queue is taken is not called in hard-irq context, so this cannot happen. Which would explain why it's been there for so long and has not been seen. And it looks like the fix for this would be to use spin_lock_irq() instead of spin_lock or spin_lock_bh in this case for the session forward lock (for some paths?). > > - Chris >
Re: [PATCH] libiscsi: ensure session spin lock usage consistent
On 02/07/2018 09:17 PM, Chris Leech wrote: > I overlooked it by mentally swapping out the session->lock in the patch > for session->frwd_lock from the warning when looking at this, but what > kernel was this patch built against? It doesn't have the > frwd_lock/back_lock split stuff. > > - Chris > Please disregard this patch set. It was based on the wrong branch of linux-scsi. I will resubmit V2 soon. Thank you for your reviews. -- Lee Duncan
Re: [PATCH v3 2/2] qedi: Cleanup local str variable
On 02/07/2018 08:12 AM, Nilesh Javali wrote: > Signed-off-by: Nilesh Javali <nilesh.jav...@cavium.com> > --- > drivers/scsi/qedi/qedi_main.c | 43 > --- > 1 file changed, 20 insertions(+), 23 deletions(-) > > diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c > ... Signed-off-by: Lee Duncan <ldun...@suse.com> -- Lee Duncan SUSE Labs
Re: [PATCH v3 1/2] qedi: Fix truncation of CHAP name and secret
On 02/07/2018 08:12 AM, Nilesh Javali wrote: > From: Andrew Vasquez <andrew.vasq...@cavium.com> > > The data in NVRAM is not guaranteed to be NUL terminated. > Since snprintf expects byte-stream to accommodate null byte, > the CHAP secret is truncated. > Use sprintf instead of snprintf to fix the truncation of > CHAP name and secret. > > Signed-off-by: Andrew Vasquez <andrew.vasq...@cavium.com> > Signed-off-by: Nilesh Javali <nilesh.jav...@cavium.com> > --- > drivers/scsi/qedi/qedi_main.c | 24 > 1 file changed, 12 insertions(+), 12 deletions(-) > > ... Signed-off-by: Lee Duncan <ldun...@suse.com> -- Lee Duncan
Re: [PATCH] qedi: Fix truncation of name and secret
On 02/05/2018 11:15 AM, Lee Duncan wrote:
> On 01/31/2018 10:57 PM, Nilesh Javali wrote:
>> Adjust the NULL byte added by snprintf.
>>
>> Signed-off-by: Nilesh Javali <nilesh.jav...@cavium.com>
>> ---
>> drivers/scsi/qedi/qedi_main.c | 12 ++--
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
>> index 34a..cf8badb 100644
>> --- a/drivers/scsi/qedi/qedi_main.c
>> +++ b/drivers/scsi/qedi/qedi_main.c
>> @@ -1840,7 +1840,7 @@ static ssize_t qedi_show_boot_ini_info(void *data, int
>> type, char *buf)
>>
>> switch (type) {
>> case ISCSI_BOOT_INI_INITIATOR_NAME:
>> -rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, "%s\n",
>> +rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN + 1, "%s",
>>initiator->initiator_name.byte);
>> break;
>> default:
>> @@ -1908,7 +1908,7 @@ static umode_t qedi_ini_get_attr_visibility(void
>> *data, int type)
>>
>> switch (type) {
>> case ISCSI_BOOT_TGT_NAME:
>> -rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, "%s\n",
>> +rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN + 1, "%s",
>>block->target[idx].target_name.byte);
>> break;
>> case ISCSI_BOOT_TGT_IP_ADDR:
>> @@ -1930,19 +1930,19 @@ static umode_t qedi_ini_get_attr_visibility(void
>> *data, int type)
>>block->target[idx].lun.value[0]);
>> break;
>> case ISCSI_BOOT_TGT_CHAP_NAME:
>> -rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN, "%s\n",
>> +rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN + 1, "%s",
>>chap_name);
>> break;
>> case ISCSI_BOOT_TGT_CHAP_SECRET:
>> -rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN, "%s\n",
>> +rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN + 1, "%s",
>>chap_secret);
>> break;
>> case ISCSI_BOOT_TGT_REV_CHAP_NAME:
>> -rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN, "%s\n",
>> +rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN + 1, "%s",
>>mchap_name);
>> break;
>> case ISCSI_BOOT_TGT_REV_CHAP_SECRET:
>> -rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN, "%s\n",
>> +rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN + 1, "%s",
>>mchap_secret);
>> break;
>> case ISCSI_BOOT_TGT_FLAGS:
>>
>
> Reviewed-by: Lee Duncan <ldun...@suse.com>
>
Assuming you address Bart's comments.
--
Lee Duncan
SUSE Labs
Re: [PATCH] qedi: Fix truncation of name and secret
On 01/31/2018 10:57 PM, Nilesh Javali wrote:
> Adjust the NULL byte added by snprintf.
>
> Signed-off-by: Nilesh Javali <nilesh.jav...@cavium.com>
> ---
> drivers/scsi/qedi/qedi_main.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 34a..cf8badb 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -1840,7 +1840,7 @@ static ssize_t qedi_show_boot_ini_info(void *data, int
> type, char *buf)
>
> switch (type) {
> case ISCSI_BOOT_INI_INITIATOR_NAME:
> - rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, "%s\n",
> + rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN + 1, "%s",
> initiator->initiator_name.byte);
> break;
> default:
> @@ -1908,7 +1908,7 @@ static umode_t qedi_ini_get_attr_visibility(void *data,
> int type)
>
> switch (type) {
> case ISCSI_BOOT_TGT_NAME:
> - rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, "%s\n",
> + rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN + 1, "%s",
> block->target[idx].target_name.byte);
> break;
> case ISCSI_BOOT_TGT_IP_ADDR:
> @@ -1930,19 +1930,19 @@ static umode_t qedi_ini_get_attr_visibility(void
> *data, int type)
> block->target[idx].lun.value[0]);
> break;
> case ISCSI_BOOT_TGT_CHAP_NAME:
> - rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN, "%s\n",
> + rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN + 1, "%s",
> chap_name);
> break;
> case ISCSI_BOOT_TGT_CHAP_SECRET:
> - rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN, "%s\n",
> + rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN + 1, "%s",
> chap_secret);
> break;
> case ISCSI_BOOT_TGT_REV_CHAP_NAME:
> - rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN, "%s\n",
> + rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN + 1, "%s",
> mchap_name);
> break;
> case ISCSI_BOOT_TGT_REV_CHAP_SECRET:
> - rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN, "%s\n",
> + rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN + 1, "%s",
> mchap_secret);
> break;
> case ISCSI_BOOT_TGT_FLAGS:
>
Reviewed-by: Lee Duncan <ldun...@suse.com>
--
Lee
[PATCH] libiscsi: ensure session spin lock usage consistent
The libiscsi code was using both spin_lock()/spin_unlock()
and spin_lock_bh()/spin_unlock_bh() on its session lock.
In addition, lock validation found that libiscsi.c was
taking a HARDIRQ-unsafe lock while holding an HARDIRQ-
irq-safe lock:
> [ 2528.738454] =
> [ 2528.744679] WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
> [ 2528.749891] 4.15.0-6-g4548e6f42022-dirty #418 Not tainted
> [ 2528.754356] -
> [ 2528.759643] kworker/0:1H/100 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
> [ 2528.764608] (&(>frwd_lock)->rlock){+.-.}, at:
> [<97d8de0f>] iscsi_eh_cmd_timed_out+0x3d/0x2b0
> [ 2528.769164]
>and this task is already holding:
> [ 2528.771313] (&(>__queue_lock)->rlock){-.-.}, at: [<a105bf06>]
> blk_timeout_work+0x22/0x120
> [ 2528.773825] which would create a new lock dependency:
> [ 2528.775216] (&(>__queue_lock)->rlock){-.-.} ->
> (&(>frwd_lock)->rlock){+.-.}
> [ 2528.777071]
>but this new dependency connects a HARDIRQ-irq-safe lock:
> [ 2528.778945] (&(>__queue_lock)->rlock){-.-.}
> [ 2528.778948]
>... which became HARDIRQ-irq-safe at:
> [ 2528.781204] _raw_spin_lock_irqsave+0x3c/0x4b
> [ 2528.781966] cfq_idle_slice_timer+0x2f/0x100
> [ 2528.782705] __hrtimer_run_queues+0xdc/0x440
> [ 2528.783448] hrtimer_interrupt+0xb1/0x210
> [ 2528.784149] smp_apic_timer_interrupt+0x6d/0x260
> [ 2528.784954] apic_timer_interrupt+0xac/0xc0
> [ 2528.785679] native_safe_halt+0x2/0x10
> [ 2528.786280] default_idle+0x1f/0x180
> [ 2528.786806] do_idle+0x166/0x1e0
> [ 2528.787288] cpu_startup_entry+0x6f/0x80
> [ 2528.787874] start_secondary+0x186/0x1e0
> [ 2528.788448] secondary_startup_64+0xa5/0xb0
> [ 2528.789070]
>to a HARDIRQ-irq-unsafe lock:
> [ 2528.789913] (&(>frwd_lock)->rlock){+.-.}
> [ 2528.789915]
>... which became HARDIRQ-irq-unsafe at:
> [ 2528.791548] ...
> [ 2528.791553] _raw_spin_lock_bh+0x34/0x40
> [ 2528.792366] iscsi_conn_setup+0x166/0x220
> [ 2528.792960] iscsi_tcp_conn_setup+0x10/0x40
> [ 2528.793526] iscsi_sw_tcp_conn_create+0x1b/0x160
> [ 2528.794111] iscsi_if_rx+0xf9f/0x1580
> [ 2528.794542] netlink_unicast+0x1f7/0x2f0
> [ 2528.795105] netlink_sendmsg+0x2c9/0x3c0
> [ 2528.795636] sock_sendmsg+0x30/0x40
> [ 2528.796068] ___sys_sendmsg+0x269/0x2c0
> [ 2528.796544] __sys_sendmsg+0x51/0x90
> [ 2528.797028] entry_SYSCALL_64_fastpath+0x25/0x9c
> [ 2528.797595]
> ...
This commit avoids possible reverse deadlock.
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/scsi/libiscsi.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 82c3fd4bc938..055357b2fe9e 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1248,9 +1248,9 @@ int iscsi_complete_pdu(struct iscsi_conn *conn, struct
iscsi_hdr *hdr,
{
int rc;
- spin_lock(>session->lock);
+ spin_lock_bh(>session->lock);
rc = __iscsi_complete_pdu(conn, hdr, data, datalen);
- spin_unlock(>session->lock);
+ spin_unlock_bh(>session->lock);
return rc;
}
EXPORT_SYMBOL_GPL(iscsi_complete_pdu);
@@ -1749,14 +1749,14 @@ static void iscsi_tmf_timedout(unsigned long data)
struct iscsi_conn *conn = (struct iscsi_conn *)data;
struct iscsi_session *session = conn->session;
- spin_lock(>lock);
+ spin_lock_bh(>lock);
if (conn->tmf_state == TMF_QUEUED) {
conn->tmf_state = TMF_TIMEDOUT;
ISCSI_DBG_EH(session, "tmf timedout\n");
/* unblock eh_abort() */
wake_up(>ehwait);
}
- spin_unlock(>lock);
+ spin_unlock_bh(>lock);
}
static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
@@ -1908,7 +1908,7 @@ static enum blk_eh_timer_return
iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
ISCSI_DBG_EH(session, "scsi cmd %p timedout\n", sc);
- spin_lock(>lock);
+ spin_lock_bh(>lock);
task = (struct iscsi_task *)sc->SCp.ptr;
if (!task) {
/*
@@ -2022,7 +2022,7 @@ static enum blk_eh_timer_return
iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
done:
if (task)
task->last_timeout = jiffies;
- spin_unlock(>lock);
+ spin_unlock_bh(>lock);
ISCSI_DBG_EH(session, "return %s\n", rc == BLK_EH_RESET_TIMER ?
"timer reset" : "nh");
return rc;
@@ -2034,7 +2034,7 @@ static
[LSF/MM ATTEND] I would like to attend to discuss several matters
I am interested in attending to help discuss all things storage. In particular, I am very interested in the "Patch Submission process and Handling Internal Conflict" proposed by James, and in "Improving Asynchronous SCSI Disk Probing" proposed by Bart. Lastly, I am also interested in discussing booting from remote storage, but I do not have any suggested solutions or I'd propose a talk on the subject. -- Lee Duncan
Re: [PATCH] scsi: libiscsi: Allow sd_shutdown on bad transport
On 12/07/2017 01:59 PM, Rafael David Tinoco wrote:
> If, for any reason, userland shuts down iscsi transport interfaces
> before proper logouts - like when logging in to LUNs manually,
> without logging out on server shutdown, or when automated scripts
> can't umount/logout from logged LUNs - kernel will hang forever on
> its sd_sync_cache() logic, after issuing the SYNCHRONIZE_CACHE cmd
> to all still existent paths.
>
> PID: 1 TASK: 8801a69b8000 CPU: 1 COMMAND: "systemd-shutdow"
> #0 [8801a69c3a30] __schedule at 8183e9ee
> #1 [8801a69c3a80] schedule at 8183f0d5
> #2 [8801a69c3a98] schedule_timeout at 81842199
> #3 [8801a69c3b40] io_schedule_timeout at 8183e604
> #4 [8801a69c3b70] wait_for_completion_io_timeout at 8183fc6c
> #5 [8801a69c3bd0] blk_execute_rq at 813cfe10
> #6 [8801a69c3c88] scsi_execute at 815c3fc7
> #7 [8801a69c3cc8] scsi_execute_req_flags at 815c60fe
> #8 [8801a69c3d30] sd_sync_cache at 815d37d7
> #9 [8801a69c3da8] sd_shutdown at 815d3c3c
>
> This happens because iscsi_eh_cmd_timed_out(), the transport layer
> timeout helper, would tell the queue timeout function (scsi_times_out)
> to reset the request timer over and over, until the session state is
> back to logged in state. Unfortunately, during server shutdown, this
> might never happen again.
>
> Other option would be "not to handle" the issue in the transport
> layer. That would trigger the error handler logic, which would also
> need the session state to be logged in again.
>
> Best option, for such case, is to tell upper layers that the command
> was handled during the transport layer error handler helper, marking
> it as DID_NO_CONNECT, which will allow completion and inform about
> the problem.
>
> After the session was marked as ISCSI_STATE_FAILED, due to the first
> timeout during the server shutdown phase, all subsequent cmds will
> fail to be queued, allowing upper logic to fail faster.
>
> Signed-off-by: Rafael David Tinoco <rafael.tin...@canonical.com>
> ---
> drivers/scsi/libiscsi.c | 24 +++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 9c50d2d9f27c..785d1c55d152 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1696,6 +1696,15 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct
> scsi_cmnd *sc)
>*/
> switch (session->state) {
> case ISCSI_STATE_FAILED:
> + /*
> + * cmds should fail during shutdown, if the session
> + * state is bad, allowing completion to happen
> + */
> + if (unlikely(system_state != SYSTEM_RUNNING)) {
> + reason = FAILURE_SESSION_FAILED;
> + sc->result = DID_NO_CONNECT << 16;
> + break;
> + }
> case ISCSI_STATE_IN_RECOVERY:
> reason = FAILURE_SESSION_IN_RECOVERY;
> sc->result = DID_IMM_RETRY << 16;
> @@ -1978,6 +1987,19 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct
> scsi_cmnd *sc)
> }
>
> if (session->state != ISCSI_STATE_LOGGED_IN) {
> + /*
> + * During shutdown, if session is prematurely disconnected,
> + * recovery won't happen and there will be hung cmds. Not
> + * handling cmds would trigger EH, also bad in this case.
> + * Instead, handle cmd, allow completion to happen and let
> + * upper layer to deal with the result.
> + */
> + if (unlikely(system_state != SYSTEM_RUNNING)) {
> + sc->result = DID_NO_CONNECT << 16;
> + ISCSI_DBG_EH(session, "sc on shutdown, handled\n");
> + rc = BLK_EH_HANDLED;
> + goto done;
> + }
> /*
>* We are probably in the middle of iscsi recovery so let
>* that complete and handle the error.
> @@ -2082,7 +2104,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct
> scsi_cmnd *sc)
> task->last_timeout = jiffies;
> spin_unlock(>frwd_lock);
> ISCSI_DBG_EH(session, "return %s\n", rc == BLK_EH_RESET_TIMER ?
> - "timer reset" : "nh");
> + "timer reset" : "shutdown or nh");
> return rc;
> }
> EXPORT_SYMBOL_GPL(iscsi_eh_cmd_timed_out);
>
Reviewed-by: Lee Duncan <ldun...@suse.com>
--
Lee
Re: [PATCH] scsi: libiscsi: fix shifting of DID_REQUEUE host byte
On 10/09/2017 04:33 AM, Johannes Thumshirn wrote:
> The SCSI host byte should be shifted left by 16 in order to have
> scsi_decide_disposition() do the right thing (.i.e. requeue the command).
>
> Signed-off-by: Johannes Thumshirn <jthumsh...@suse.de>
> Fixes: 661134ad3765 ("[SCSI] libiscsi, bnx2i: make bound ep check common")
> Cc: Lee Duncan <ldun...@suse.com>
> Cc: Hannes Reinecke <h...@suse.de>
> Cc: Bart Van Assche <bart.vanass...@sandisk.com>
> Cc: Chris Leech <cle...@redhat.com>
> ---
> drivers/scsi/libiscsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index bd4605a34f54..9cba4913b43c 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1728,7 +1728,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct
> scsi_cmnd *sc)
>
> if (test_bit(ISCSI_SUSPEND_BIT, >suspend_tx)) {
> reason = FAILURE_SESSION_IN_RECOVERY;
> - sc->result = DID_REQUEUE;
> + sc->result = DID_REQUEUE << 16;
> goto fault;
> }
>
>
Good catch.
I know you're working on fixing all drivers to use the correct macros
rather than rolling there own.
Acked-by: Lee Duncan <ldun...@suse.com>
--
Lee Duncan
SUSE Labs
Re: [PATCH] scsi: ILLEGAL REQUEST + ASC==27 => target failure
On 09/27/2017 05:44 AM, Martin Wilck wrote:
> ASC 0x27 is "WRITE PROTECTED". This error code is returned e.g.
> by Fujitsu ETERNUS systems under certain conditions for
> WRITE SAME 16 commands with UNMAP bit set. It should not be
> treated as a path error. In general, it makes sense to assume
> that being write protected is a target rather than a path
> property.
>
> Signed-off-by: Martin Wilck <mwi...@suse.com>
> ---
> drivers/scsi/scsi_error.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 38942050b265..dab876c65473 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -580,7 +580,8 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
> if (sshdr.asc == 0x20 || /* Invalid command operation code */
> sshdr.asc == 0x21 || /* Logical block address out of range
> */
> sshdr.asc == 0x24 || /* Invalid field in cdb */
> - sshdr.asc == 0x26) { /* Parameter value invalid */
> + sshdr.asc == 0x26 || /* Parameter value invalid */
> + sshdr.asc == 0x27) { /* Write protected */
> set_host_byte(scmd, DID_TARGET_FAILURE);
> }
> return SUCCESS;
>
Looks good to me.
Acked-by: Lee Duncan <ldun...@suse.com>
--
Lee-Man
Re: [PATCH] scsi: ioctl reset should wait for IOs to complete
On 09/26/2017 11:58 PM, Hannes Reinecke wrote: > On 09/26/2017 08:24 PM, Bart Van Assche wrote: >> On Tue, 2017-09-26 at 10:22 -0700, Lee Duncan wrote: >>> The SCSI ioctl reset path is smart enough to set the >>> flag tmf_in_progress when a user-requested reset is >>> processed, but it does not wait for IO that is in >>> flight. This can result in lost IOs and hung >>> processes. We should wait for a reasonable amount >>> of time for either the IOs to complete or to fail >>> the request. >> >> Hello Lee, >> >> I'm using this functionality all the time to test how SCSI target code >> handles >> TMFs while SCSI commands are in progress. So I would regret if the SCSI reset >> ioctl code would be modified such that it waits for outstanding requests. >> Isn't the behavior you described a SCSI LLD bug? Shouldn't such bugs be fixed >> instead of implementing a work-around in the SCSI core? >> > Well, thing is that there is an asymmetry here; originally all SCSI EH > functions were supposed to run with no I/O in flight. > (I've modified that with the asynchronous ABORT TASK TMF, but still). > But when called with sg_reset this is no longer true, we're disallowing > new requests, but do not wait for the in-flight I/O to complete. > And we've had a customer report where calling sg_reset -t on an iSCSI > device caused I/O to become stuck as the in-flight I/O was terminated by > the target reset, but the iSCSI stack never sent a completion for that I/O. > > However, we could also defer this problem until my SCSI EH rework goes > in; that clears up the sg_reset path and might clarify things a bit. > > Cheers, > > Hannes > I will wait and see if the problem still exists there, and address it if it does. Thank you. -- Lee Duncan SUSE Labs
[PATCH] scsi: ioctl reset should wait for IOs to complete
The SCSI ioctl reset path is smart enough to set the
flag tmf_in_progress when a user-requested reset is
processed, but it does not wait for IO that is in
flight. This can result in lost IOs and hung
processes. We should wait for a reasonable amount
of time for either the IOs to complete or to fail
the request.
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/scsi/scsi_error.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 38942050b265..b964152611c3 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -57,6 +57,14 @@
#define BUS_RESET_SETTLE_TIME (10)
#define HOST_RESET_SETTLE_TIME (10)
+/*
+ * Time to wait for outstanding IOs when about to send
+ * a device reset, e.g. sg_reset. The msecs to wait must
+ * be an multiple of the msecs to wait per try.
+ */
+#define MSECS_PER_TRY_FOR_IO_ON_RESET 500
+#define MSECS_TO_WAIT_FOR_IO_ON_RESET (MSECS_PER_TRY_FOR_IO_ON_RESET * 10)
+
static int scsi_eh_try_stu(struct scsi_cmnd *scmd);
static int scsi_try_to_abort_cmd(struct scsi_host_template *,
struct scsi_cmnd *);
@@ -2269,6 +2277,7 @@ void scsi_report_device_reset(struct Scsi_Host *shost,
int channel, int target)
struct request *rq;
unsigned long flags;
int error = 0, rtn, val;
+ unsigned int msecs_to_wait = MSECS_TO_WAIT_FOR_IO_ON_RESET;
if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
return -EACCES;
@@ -2301,6 +2310,22 @@ void scsi_report_device_reset(struct Scsi_Host *shost,
int channel, int target)
spin_lock_irqsave(shost->host_lock, flags);
shost->tmf_in_progress = 1;
+
+ /* if any IOs in progress wait for them a while */
+ while ((atomic_read(>host_busy) > 0) && (msecs_to_wait > 0)) {
+ spin_unlock_irqrestore(shost->host_lock, flags);
+ msleep(MSECS_PER_TRY_FOR_IO_ON_RESET);
+ msecs_to_wait -= MSECS_PER_TRY_FOR_IO_ON_RESET;
+ spin_lock_irqsave(shost->host_lock, flags);
+ }
+ if (atomic_read(>host_busy)) {
+ shost->tmf_in_progress = 0;
+ spin_unlock_irqrestore(shost->host_lock, flags);
+ SCSI_LOG_ERROR_RECOVERY(3,
+ printk("%s: device reset failed: outstanding IO\n",
__func__));
+ goto out_put_scmd_and_free;
+ }
+
spin_unlock_irqrestore(shost->host_lock, flags);
switch (val & ~SG_SCSI_RESET_NO_ESCALATE) {
@@ -2349,6 +2374,7 @@ void scsi_report_device_reset(struct Scsi_Host *shost,
int channel, int target)
wake_up(>host_wait);
scsi_run_host_queues(shost);
+out_put_scmd_and_free:
scsi_put_command(scmd);
kfree(rq);
--
1.8.5.6
Re: [PATCH 00/13] use setup_timer() helper function.
On 09/22/2017 02:51 AM, Allen Pais wrote: > This series uses setup_timer() helper function. The series > addresses the files under drivers/scsi/*. > > Allen Pais (13): > scsi: sym53c8xx: use setup_timer() helper. > scsi: qla2xxx: use setup_timer() helper. > scsi: esas2r: use setup_timer() helper. > scsi: bnx2i: use setup_timer() helper. > scsi: arcmsr: use setup_timer() helper. > scsi: be2iscsi: use setup_timer() helper. > scsi: qla4xxx: use setup_timer() helper. > scsi: ncr53c8xx: use setup_timer() helper. > scsi: arm: fas216: use setup_timer() helper. > scsi: libiscsi: use setup_timer() helper. > scsi: dc395x: use setup_timer() helper. > scsi: qla2xxx: use setup_timer() helper. > scsi: bfa: use setup_timer() helper. > > drivers/scsi/arcmsr/arcmsr_hba.c| 10 -- > drivers/scsi/arm/fas216.c | 4 +--- > drivers/scsi/be2iscsi/be_main.c | 5 ++--- > drivers/scsi/bfa/bfad.c | 4 +--- > drivers/scsi/bnx2i/bnx2i_iscsi.c| 14 +- > drivers/scsi/dc395x.c | 4 +--- > drivers/scsi/esas2r/esas2r_main.c | 4 +--- > drivers/scsi/libiscsi.c | 5 ++--- > drivers/scsi/ncr53c8xx.c| 4 +--- > drivers/scsi/qla2xxx/qla_inline.h | 5 ++--- > drivers/scsi/qla2xxx/qla_os.c | 5 ++--- > drivers/scsi/qla4xxx/ql4_os.c | 5 ++--- > drivers/scsi/sym53c8xx_2/sym_glue.c | 4 +--- > 13 files changed, 25 insertions(+), 48 deletions(-) > Please add for the whole series: Acked-by: Lee Duncan <ldun...@suse.com> -- Lee Duncan SUSE Labs
Re: [PATCH v2 17/17] iscsi_tcp: Remove a set-but-not-used variable
On 08/25/2017 01:46 PM, Bart Van Assche wrote: > This patch avoids that gcc reports the following warning when > building with W=1: > > drivers/scsi/iscsi_tcp.c:166:24: warning: variable ?session? set but not used > [-Wunused-but-set-variable] > > Signed-off-by: Bart Van Assche <bart.vanass...@wdc.com> > Reviewed-by: Christoph Hellwig <h...@lst.de> > Reviewed-by: Hannes Reinecke <h...@suse.com> > Cc: Lee Duncan <ldun...@suse.com> > Cc: Johannes Thumshirn <jthumsh...@suse.de> > --- > drivers/scsi/iscsi_tcp.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c > index 4842fc0e809d..4d934d6c3e13 100644 > --- a/drivers/scsi/iscsi_tcp.c > +++ b/drivers/scsi/iscsi_tcp.c > @@ -163,7 +163,6 @@ static void iscsi_sw_tcp_state_change(struct sock *sk) > struct iscsi_tcp_conn *tcp_conn; > struct iscsi_sw_tcp_conn *tcp_sw_conn; > struct iscsi_conn *conn; > - struct iscsi_session *session; > void (*old_state_change)(struct sock *); > > read_lock_bh(>sk_callback_lock); > @@ -172,7 +171,6 @@ static void iscsi_sw_tcp_state_change(struct sock *sk) > read_unlock_bh(>sk_callback_lock); > return; > } > - session = conn->session; > > iscsi_sw_sk_state_check(sk); > > Reviewed-by: Lee Duncan <ldun...@suse.com> -- Lee Duncan
Re: [Patch v2 2/2] libiscsi: Remove iscsi_destroy_session
On 07/13/2017 09:11 AM, Khazhismel Kumykov wrote:
> iscsi_session_teardown was the only user of this function. Function
> currently is just short for iscsi_remove_session + iscsi_free_session.
>
> Signed-off-by: Khazhismel Kumykov <kha...@google.com>
Why is this needed? I dislike changes that don't fix anything.
> ---
> drivers/scsi/scsi_transport_iscsi.c | 16
> include/scsi/scsi_transport_iscsi.h | 1 -
> 2 files changed, 17 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c
> b/drivers/scsi/scsi_transport_iscsi.c
> index a424eaeafeb0..924ac408d8a9 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2210,22 +2210,6 @@ void iscsi_free_session(struct iscsi_cls_session
> *session)
> }
> EXPORT_SYMBOL_GPL(iscsi_free_session);
>
> -/**
> - * iscsi_destroy_session - destroy iscsi session
> - * @session: iscsi_session
> - *
> - * Can be called by a LLD or iscsi_transport. There must not be
> - * any running connections.
> - */
> -int iscsi_destroy_session(struct iscsi_cls_session *session)
> -{
> - iscsi_remove_session(session);
> - ISCSI_DBG_TRANS_SESSION(session, "Completing session destruction\n");
> - iscsi_free_session(session);
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(iscsi_destroy_session);
> -
> /**
> * iscsi_create_conn - create iscsi class connection
> * @session: iscsi cls session
> diff --git a/include/scsi/scsi_transport_iscsi.h
> b/include/scsi/scsi_transport_iscsi.h
> index 6183d20a01fb..b266d2a3bcb1 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -434,7 +434,6 @@ extern struct iscsi_cls_session
> *iscsi_create_session(struct Scsi_Host *shost,
> unsigned int target_id);
> extern void iscsi_remove_session(struct iscsi_cls_session *session);
> extern void iscsi_free_session(struct iscsi_cls_session *session);
> -extern int iscsi_destroy_session(struct iscsi_cls_session *session);
> extern struct iscsi_cls_conn *iscsi_create_conn(struct iscsi_cls_session
> *sess,
> int dd_size, uint32_t cid);
> extern int iscsi_destroy_conn(struct iscsi_cls_conn *conn);
>
--
Lee Duncan
SUSE Labs
Re: [PATCH 3/3] fcoe: open-code fcoe_destroy_work() for NETDEV_UNREGISTER
On 08/31/2017 02:19 AM, Hannes Reinecke wrote: > When a NETDEV_UNREGISTER notification is received the network > device is _deleted_ after the callback returns. > So we cannot use a workqueue here, as this would cause an > inversion when removing the device as the netdev is already gone. > This manifests with a nasty warning during shutdown: > < ... > Reviewed-by: Lee Duncan <ldun...@suse.com> -- Lee Duncan SUSE Labs
Re: [PATCH 2/3] fcoe: separate out fcoe_vport_remove()
On 08/31/2017 02:19 AM, Hannes Reinecke wrote: > Separate out fcoe_vport_remove() from fcoe_destroy_work(). > No functional change. > > Signed-off-by: Hannes Reinecke <h...@suse.com> > --- > drivers/scsi/fcoe/fcoe.c | 55 > +--- > 1 file changed, 33 insertions(+), 22 deletions(-) > > ... > Reviewed-by: Lee Duncan <ldun...@suse.com> -- Lee Duncan SUSE Labs
Re: [PATCH 1/3] fcoe: move fcoe_interface_remove() out of fcoe_interface_cleanup()
On 08/31/2017 02:19 AM, Hannes Reinecke wrote: > No functional change. Nit: Then why do it? Perhaps the description can say why such a change is being done? > > Signed-off-by: Hannes Reinecke <h...@suse.com> > --- > drivers/scsi/fcoe/fcoe.c | 12 +++- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c > index 85f9a3e..135bdcf 100644 > --- a/drivers/scsi/fcoe/fcoe.c > +++ b/drivers/scsi/fcoe/fcoe.c > @@ -501,11 +501,6 @@ static void fcoe_interface_cleanup(struct fcoe_interface > *fcoe) > struct net_device *netdev = fcoe->netdev; > struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe); > > - rtnl_lock(); > - if (!fcoe->removed) > - fcoe_interface_remove(fcoe); > - rtnl_unlock(); > - > /* Release the self-reference taken during fcoe_interface_create() */ > /* tear-down the FCoE controller */ > fcoe_ctlr_destroy(fip); > @@ -2140,6 +2135,11 @@ static void fcoe_destroy_work(struct work_struct *work) > cdev = fcoe_ctlr_to_ctlr_dev(ctlr); > > fcoe_if_destroy(port->lport); > + > + rtnl_lock(); > + if (!fcoe->removed) > + fcoe_interface_remove(fcoe); > + rtnl_unlock(); > fcoe_interface_cleanup(fcoe); > > mutex_unlock(_config_mutex); > @@ -2254,6 +2254,8 @@ static int _fcoe_create(struct net_device *netdev, enum > fip_mode fip_mode, > printk(KERN_ERR "fcoe: Failed to create interface (%s)\n", > netdev->name); > rc = -EIO; > + if (!fcoe->removed) > + fcoe_interface_remove(fcoe); > rtnl_unlock(); > fcoe_interface_cleanup(fcoe); > mutex_unlock(_config_mutex); > Looks good to me (other than the comment nit). Reviewed-by: Lee Duncan <ldun...@suse.com> -- Lee Duncan SUSE Labs
Re: [PATCH 2/2] qedi: Remove WARN_ON from clear task context.
On 06/15/2017 12:10 AM, Manish Rangankar wrote:
> Signed-off-by: Manish Rangankar <manish.rangan...@cavium.com>
> ---
> drivers/scsi/qedi/qedi_main.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 09a2946..879d3b7 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -1499,11 +1499,9 @@ int qedi_get_task_idx(struct qedi_ctx *qedi)
>
> void qedi_clear_task_idx(struct qedi_ctx *qedi, int idx)
> {
> - if (!test_and_clear_bit(idx, qedi->task_idx_map)) {
> + if (!test_and_clear_bit(idx, qedi->task_idx_map))
> QEDI_ERR(>dbg_ctx,
>"FW task context, already cleared, tid=0x%x\n", idx);
> - WARN_ON(1);
> - }
> }
>
> void qedi_update_itt_map(struct qedi_ctx *qedi, u32 tid, u32 proto_itt,
>
Reviewed-by: Lee Duncan <ldun...@suse.com>
Re: [PATCH 1/2] qedi: Remove WARN_ON for untracked cleanup.
On 06/15/2017 12:10 AM, Manish Rangankar wrote: > Signed-off-by: Manish Rangankar <manish.rangan...@cavium.com> > --- > drivers/scsi/qedi/qedi_fw.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c > index 8bc7ee1..507512c 100644 > --- a/drivers/scsi/qedi/qedi_fw.c > +++ b/drivers/scsi/qedi/qedi_fw.c > @@ -870,7 +870,6 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx > *qedi, > QEDI_ERR(>dbg_ctx, >"Delayed or untracked cleanup response, itt=0x%x, > tid=0x%x, cid=0x%x, task=%p\n", >protoitt, cqe->itid, qedi_conn->iscsi_conn_id, task); > - WARN_ON(1); > } > } > > Reviewed-by: Lee Duncan <ldun...@suse.com>
[LSF/MM TOPIC ATTEND] kernel booting using remote storage is a mess
The process of booting a Linux kernel with remote storage (such as iSCSI or FCoE) seems unnecessarily complicated if end users can even figure out how to do it. This is of course exacerbated by the fact that every company seems to CNA cards differently despite the iBFT standard. If you add DM-Multipath on top of that, most bets are off on your chances of getting it to reliably work. I'd like to discuss how we can get this to work in a general way given the world of systemd that we live in now. -- Lee Duncan SUSE Labs -- 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
Re: [PATCH 5/8] linux: drop __bitwise__ everywhere
+154,8 @@ typedef u64 dma_addr_t;
> typedef u32 dma_addr_t;
> #endif
>
> -typedef unsigned __bitwise__ gfp_t;
> -typedef unsigned __bitwise__ fmode_t;
> +typedef unsigned __bitwise gfp_t;
> +typedef unsigned __bitwise fmode_t;
>
> #ifdef CONFIG_PHYS_ADDR_T_64BIT
> typedef u64 phys_addr_t;
> diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h
> index c1260d8..df156f1 100644
> --- a/include/scsi/iscsi_proto.h
> +++ b/include/scsi/iscsi_proto.h
> @@ -74,7 +74,7 @@ static inline int iscsi_sna_gte(u32 n1, u32 n2)
> #define zero_data(p) {p[0]=0;p[1]=0;p[2]=0;}
>
> /* initiator tags; opaque for target */
> -typedef uint32_t __bitwise__ itt_t;
> +typedef uint32_t __bitwise itt_t;
> /* below makes sense only for initiator that created this tag */
> #define build_itt(itt, age) ((__force itt_t)\
> ((itt) | ((age) << ISCSI_AGE_SHIFT)))
> diff --git a/include/target/target_core_base.h
> b/include/target/target_core_base.h
> index c211900..0055828 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -149,7 +149,7 @@ enum se_cmd_flags_table {
> * Used by transport_send_check_condition_and_sense()
> * to signal which ASC/ASCQ sense payload should be built.
> */
> -typedef unsigned __bitwise__ sense_reason_t;
> +typedef unsigned __bitwise sense_reason_t;
>
> enum tcm_sense_reason_table {
> #define R(x) (__force sense_reason_t )(x)
> diff --git a/include/uapi/linux/virtio_types.h
> b/include/uapi/linux/virtio_types.h
> index e845e8c..55c3b73 100644
> --- a/include/uapi/linux/virtio_types.h
> +++ b/include/uapi/linux/virtio_types.h
> @@ -39,8 +39,8 @@
> * - __le{16,32,64} for standard-compliant virtio devices
> */
>
> -typedef __u16 __bitwise__ __virtio16;
> -typedef __u32 __bitwise__ __virtio32;
> -typedef __u64 __bitwise__ __virtio64;
> +typedef __u16 __bitwise __virtio16;
> +typedef __u32 __bitwise __virtio32;
> +typedef __u64 __bitwise __virtio64;
>
> #endif /* _UAPI_LINUX_VIRTIO_TYPES_H */
> diff --git a/net/ieee802154/6lowpan/6lowpan_i.h
> b/net/ieee802154/6lowpan/6lowpan_i.h
> index 5ac7789..ac7c96b 100644
> --- a/net/ieee802154/6lowpan/6lowpan_i.h
> +++ b/net/ieee802154/6lowpan/6lowpan_i.h
> @@ -7,7 +7,7 @@
> #include
> #include
>
> -typedef unsigned __bitwise__ lowpan_rx_result;
> +typedef unsigned __bitwise lowpan_rx_result;
> #define RX_CONTINUE ((__force lowpan_rx_result) 0u)
> #define RX_DROP_UNUSABLE ((__force lowpan_rx_result) 1u)
> #define RX_DROP ((__force lowpan_rx_result) 2u)
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index d37a577..b2069fb 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -159,7 +159,7 @@ enum ieee80211_bss_valid_data_flags {
> IEEE80211_BSS_VALID_ERP = BIT(3)
> };
>
> -typedef unsigned __bitwise__ ieee80211_tx_result;
> +typedef unsigned __bitwise ieee80211_tx_result;
> #define TX_CONTINUE ((__force ieee80211_tx_result) 0u)
> #define TX_DROP ((__force ieee80211_tx_result) 1u)
> #define TX_QUEUED((__force ieee80211_tx_result) 2u)
> @@ -180,7 +180,7 @@ struct ieee80211_tx_data {
> };
>
>
> -typedef unsigned __bitwise__ ieee80211_rx_result;
> +typedef unsigned __bitwise ieee80211_rx_result;
> #define RX_CONTINUE ((__force ieee80211_rx_result) 0u)
> #define RX_DROP_UNUSABLE ((__force ieee80211_rx_result) 1u)
> #define RX_DROP_MONITOR ((__force ieee80211_rx_result) 2u)
>
For iscsi initiator, looks good.
Akced-by: Lee Duncan <ldun...@suse.com>
--
Lee Duncan
--
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
Re: [PATCH 2/2] be2iscsi: Replace _bh with _irqsave/irqrestore
1,16 @@ beiscsi_get_wrb_handle(struct hwi_wrb_context
> *pwrb_context,
> unsigned int wrbs_per_cxn)
> {
> struct wrb_handle *pwrb_handle;
> + unsigned long flags;
>
> - spin_lock_bh(_context->wrb_lock);
> + spin_lock_irqsave(_context->wrb_lock, flags);
> pwrb_handle = pwrb_context->pwrb_handle_base[pwrb_context->alloc_index];
> pwrb_context->wrb_handles_available--;
> if (pwrb_context->alloc_index == (wrbs_per_cxn - 1))
> pwrb_context->alloc_index = 0;
> else
> pwrb_context->alloc_index++;
> - spin_unlock_bh(_context->wrb_lock);
> + spin_unlock_irqrestore(_context->wrb_lock, flags);
>
> if (pwrb_handle)
> memset(pwrb_handle->pwrb, 0, sizeof(*pwrb_handle->pwrb));
> @@ -1001,14 +1005,16 @@ beiscsi_put_wrb_handle(struct hwi_wrb_context
> *pwrb_context,
> struct wrb_handle *pwrb_handle,
> unsigned int wrbs_per_cxn)
> {
> - spin_lock_bh(_context->wrb_lock);
> + unsigned long flags;
> +
> + spin_lock_irqsave(_context->wrb_lock, flags);
> pwrb_context->pwrb_handle_base[pwrb_context->free_index] = pwrb_handle;
> pwrb_context->wrb_handles_available++;
> if (pwrb_context->free_index == (wrbs_per_cxn - 1))
> pwrb_context->free_index = 0;
> else
> pwrb_context->free_index++;
> - spin_unlock_bh(_context->wrb_lock);
> + spin_unlock_irqrestore(_context->wrb_lock, flags);
> }
>
> /**
> @@ -1037,8 +1043,9 @@ free_wrb_handle(struct beiscsi_hba *phba, struct
> hwi_wrb_context *pwrb_context,
> static struct sgl_handle *alloc_mgmt_sgl_handle(struct beiscsi_hba *phba)
> {
> struct sgl_handle *psgl_handle;
> + unsigned long flags;
>
> - spin_lock_bh(>mgmt_sgl_lock);
> + spin_lock_irqsave(>mgmt_sgl_lock, flags);
> if (phba->eh_sgl_hndl_avbl) {
> psgl_handle = phba->eh_sgl_hndl_base[phba->eh_sgl_alloc_index];
> phba->eh_sgl_hndl_base[phba->eh_sgl_alloc_index] = NULL;
> @@ -1056,14 +1063,16 @@ static struct sgl_handle
> *alloc_mgmt_sgl_handle(struct beiscsi_hba *phba)
> phba->eh_sgl_alloc_index++;
> } else
> psgl_handle = NULL;
> - spin_unlock_bh(>mgmt_sgl_lock);
> + spin_unlock_irqrestore(>mgmt_sgl_lock, flags);
> return psgl_handle;
> }
>
> void
> free_mgmt_sgl_handle(struct beiscsi_hba *phba, struct sgl_handle
> *psgl_handle)
> {
> - spin_lock_bh(>mgmt_sgl_lock);
> + unsigned long flags;
> +
> + spin_lock_irqsave(>mgmt_sgl_lock, flags);
> beiscsi_log(phba, KERN_INFO, BEISCSI_LOG_CONFIG,
> "BM_%d : In free_mgmt_sgl_handle,"
> "eh_sgl_free_index=%d\n",
> @@ -1078,7 +1087,7 @@ free_mgmt_sgl_handle(struct beiscsi_hba *phba, struct
> sgl_handle *psgl_handle)
> "BM_%d : Double Free in eh SGL ,"
> "eh_sgl_free_index=%d\n",
> phba->eh_sgl_free_index);
> - spin_unlock_bh(>mgmt_sgl_lock);
> + spin_unlock_irqrestore(>mgmt_sgl_lock, flags);
> return;
> }
> phba->eh_sgl_hndl_base[phba->eh_sgl_free_index] = psgl_handle;
> @@ -1088,7 +1097,7 @@ free_mgmt_sgl_handle(struct beiscsi_hba *phba, struct
> sgl_handle *psgl_handle)
> phba->eh_sgl_free_index = 0;
> else
> phba->eh_sgl_free_index++;
> - spin_unlock_bh(>mgmt_sgl_lock);
> + spin_unlock_irqrestore(>mgmt_sgl_lock, flags);
> }
>
> static void
>
Reviewed-by: Lee Duncan <ldun...@suse.com>
--
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
Re: [PATCH 1/2] libiscsi: Fix locking in __iscsi_conn_send_pdu
On 10/12/2016 11:38 PM, Jitendra Bhivare wrote: > The code at free_task label in __iscsi_conn_send_pdu can get executed > from blk_timeout_work which takes queue_lock using spin_lock_irq. > back_lock taken with spin_unlock_bh will cause WARN_ON_ONCE. > The code gets executed either with bottom half or IRQ disabled hence > using spin_lock/spin_unlock for back_lock is safe. > > Signed-off-by: Jitendra Bhivare <jitendra.bhiv...@broadcom.com> > --- > drivers/scsi/libiscsi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c > index c051694..f9b6fba 100644 > --- a/drivers/scsi/libiscsi.c > +++ b/drivers/scsi/libiscsi.c > @@ -791,9 +791,9 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct > iscsi_hdr *hdr, > > free_task: > /* regular RX path uses back_lock */ > - spin_lock_bh(>back_lock); > + spin_lock(>back_lock); > __iscsi_put_task(task); > - spin_unlock_bh(>back_lock); > + spin_unlock(>back_lock); > return NULL; > } > > Reviewed-by: Lee Duncan <ldun...@suse.com> -- 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
Re: [PATCHv2] MAINTAINERS: Update open-iscsi maintainers
Hello Martin: On 09/26/2016 06:26 PM, Martin K. Petersen wrote: >>>>>> "Lee" == Lee Duncan <ldun...@suse.com> writes: > > Lee, > > Lee> Chris Leech and I are taking over as open-iscsi maintainers. > > Do you want me to queue the MAINTAINER update? Yes, that would be great. Thank you. > > Lee> * Removed git repository, since code in tree > > Is it your plan to go through the SCSI tree? > Yes, the iscsi initiator kernel code updates have been going through the Linux SCSI mailing list and repository for a while, now. The open-iscsi kernel code git repository currently mentioned in the MAINTAINERS file no longer exists. -- Lee Duncan -- 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
[PATCHv2] MAINTAINERS: Update open-iscsi maintainers
Chris Leech and I are taking over as open-iscsi maintainers. Changes since v1: * Updated URL to open-iscsi.com * Removed git repository, since code in tree --- MAINTAINERS | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 01bff8ea28d8..81384a2562e7 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6448,10 +6448,10 @@ S: Maintained F: drivers/firmware/iscsi_ibft* ISCSI -M: Mike Christie <micha...@cs.wisc.edu> +M: Lee Duncan <ldun...@suse.com> +M: Chris Leech <cle...@redhat.com> L: open-is...@googlegroups.com -W: www.open-iscsi.org -T: git git://git.kernel.org/pub/scm/linux/kernel/git/mnc/linux-2.6-iscsi.git +W: www.open-iscsi.com S: Maintained F: drivers/scsi/*iscsi* F: include/scsi/*iscsi* -- 2.1.4 -- 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
Re: [PATCH] MAINTAINERS: Update open-iscsi maintainers
[Added linux-scsi to the cc list.] I will resubmit an updated version of this patch. On 09/23/2016 02:34 PM, Lee Duncan wrote: > Chris Leech and I are taking over open-iscsi > maintenance from Mike Christie. > > Signed-off-by: Lee Duncan <ldun...@suse.com> > --- > MAINTAINERS | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 01bff8ea28d8..0afaf42d5416 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6448,7 +6448,8 @@ S: Maintained > F: drivers/firmware/iscsi_ibft* > > ISCSI > -M: Mike Christie <micha...@cs.wisc.edu> > +M: Lee Duncan <ldun...@suse.com> > +M: Chris Leech <cle...@redhat.com> > L: open-is...@googlegroups.com > W: www.open-iscsi.org > T: git > git://git.kernel.org/pub/scm/linux/kernel/git/mnc/linux-2.6-iscsi.git > -- Lee Duncan -- 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
Re: [PATCH v2] fcoe: provide translation table between Ethernet and FC port speeds
On 08/19/2016 06:33 AM, Johannes Thumshirn wrote:
> Provide a translation table between Ethernet and FC port speeds so odd
> speeds (from a Ethernet POV) like 8 Gbit are correctly mapped to sysfs
> and open-fcoe's fcoeadm.
>
> Before:
> Description: BCM57840 NetXtreme II 10/20-Gigabit Ethernet
> Revision: 11
> Manufacturer: Broadcom Corporation
> Serial Number:6CC2173EA1D0
>
> Driver: bnx2x 1.712.30-0
> Number of Ports: 1
>
> Symbolic Name: bnx2fc (QLogic BCM57840) v2.10.3 over eth2
> OS Device Name:host1
> Node Name: 0x20006cc2173ea1d1
> Port Name: 0x10006cc2173ea1d1
> FabricName:0x10c0dd0ce717
> Speed: unknown
> Supported Speed: 1 Gbit, 10 Gbit
> MaxFrameSize: 2048 bytes
> FC-ID (Port ID): 0x660702
> State: Online
>
> After:
> Description: BCM57840 NetXtreme II 10/20-Gigabit Ethernet
> Revision: 11
> Manufacturer: Broadcom Corporation
> Serial Number:6CC2173EA1D0
>
> Driver: bnx2x 1.712.30-0
> Number of Ports: 1
>
> Symbolic Name: bnx2fc (QLogic BCM57840) v2.10.3 over eth2
> OS Device Name:host1
> Node Name: 0x20006cc2173ea1d1
> Port Name: 0x10006cc2173ea1d1
> FabricName:0x10c0dd0ce717
> Speed: 8 Gbit
> Supported Speed: 1 Gbit, 10 Gbit
> MaxFrameSize: 2048 bytes
> FC-ID (Port ID): 0x660701
> State: Online
>
> Signed-off-by: Johannes Thumshirn <jthumsh...@suse.de>
> ---
>
> Changes to v1:
> * Add definitions for non-native Ethernet speeds
>
> drivers/scsi/fcoe/fcoe_transport.c | 53
> ++
> 1 file changed, 36 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/scsi/fcoe/fcoe_transport.c
> b/drivers/scsi/fcoe/fcoe_transport.c
> index 7028dd3..c164eec 100644
> --- a/drivers/scsi/fcoe/fcoe_transport.c
> +++ b/drivers/scsi/fcoe/fcoe_transport.c
> @@ -83,6 +83,41 @@ static struct notifier_block libfcoe_notifier = {
> .notifier_call = libfcoe_device_notification,
> };
>
> +static const struct {
> + u32 fc_port_speed;
> +#define SPEED_2000 2000
> +#define SPEED_4000 4000
> +#define SPEED_8000 8000
> +#define SPEED_16000 16000
> +#define SPEED_32000 32000
> + u32 eth_port_speed;
> +} fcoe_port_speed_mapping[] = {
> + { FC_PORTSPEED_1GBIT, SPEED_1000 },
> + { FC_PORTSPEED_2GBIT, SPEED_2000 },
> + { FC_PORTSPEED_4GBIT, SPEED_4000 },
> + { FC_PORTSPEED_8GBIT, SPEED_8000 },
> + { FC_PORTSPEED_10GBIT, SPEED_1 },
> + { FC_PORTSPEED_16GBIT, SPEED_16000 },
> + { FC_PORTSPEED_20GBIT, SPEED_2 },
> + { FC_PORTSPEED_25GBIT, SPEED_25000 },
> + { FC_PORTSPEED_32GBIT, SPEED_32000 },
> + { FC_PORTSPEED_40GBIT, SPEED_4 },
> + { FC_PORTSPEED_50GBIT, SPEED_5 },
> + { FC_PORTSPEED_100GBIT, SPEED_10 },
> +};
> +
> +static inline u32 eth2fc_speed(u32 eth_port_speed)
> +{
> + int i;
> +
> + for (i = 0; i <= ARRAY_SIZE(fcoe_port_speed_mapping); i++) {
> + if (fcoe_port_speed_mapping[i].eth_port_speed == eth_port_speed)
> + return fcoe_port_speed_mapping[i].fc_port_speed;
> + }
> +
> + return FC_PORTSPEED_UNKNOWN;
> +}
> +
> /**
> * fcoe_link_speed_update() - Update the supported and actual link speeds
> * @lport: The local port to update speeds for
> @@ -126,23 +161,7 @@ int fcoe_link_speed_update(struct fc_lport *lport)
> SUPPORTED_4baseLR4_Full))
> lport->link_supported_speeds |= FC_PORTSPEED_40GBIT;
>
> - switch (ecmd.base.speed) {
> - case SPEED_1000:
> - lport->link_speed = FC_PORTSPEED_1GBIT;
> - break;
> - case SPEED_1:
> - lport->link_speed = FC_PORTSPEED_10GBIT;
> - break;
> - case SPEED_2:
> - lport->link_speed = FC_PORTSPEED_20GBIT;
> - break;
> - case SPEED_4:
> - lport->link_speed = FC_PORTSPEED_40GBIT;
> - break;
> - default:
> - lport->link_speed = FC_PORTSPEED_UNKNOWN;
> - break;
> - }Reviewed-by: Lee Duncan <ldun...@suse.com>
> + lport->link_speed = eth2fc_speed(ecmd.base.speed);
> return 0;
> }
> return -1;
>
Reviewed-by: Lee Duncan <ldun...@suse.com>
--
Lee Duncan
--
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
Re: [PATCHv3 0/2] target: make location of /var/targets configurable
Ping? We really need to move the target database out of /var/target On 04/14/2016 06:18 PM, Lee Duncan wrote: > These patches make the location of "/var/target" configurable, > though it still defauls to "/var/target". > > This "target database directory" can only be changed > after the target_core_mod loads but before any > fabric drivers are loaded, and must be the pathname > of an existing directory. > > This configuration is accomplished via the configfs > top-level target attribute "dbroot", i.e. dumping > out "/sys/kernel/config/target/dbroot" will normally > return "/var/target". Writing to this attribute > changes the loation where the kernel looks for the > target database. > > The first patch creates this configurable value for > the "dbroot", and the second patch modifies users > of this directory to use this new attribute. > > Changes from v2: > * Add locking around access to target driver list > > Changes from v1: > * Only allow changing target DB root before it >can be used by others > * Validate that new DB root is a valid directory > > Lee Duncan (2): > target: make target db location configurable > target: use new "dbroot" target attribute > > drivers/target/target_core_alua.c | 6 ++-- > drivers/target/target_core_configfs.c | 62 > +++++++++++ > drivers/target/target_core_internal.h | 6 > drivers/target/target_core_pr.c | 2 +- > 4 files changed, 72 insertions(+), 4 deletions(-) > -- Lee Duncan SUSE Labs -- 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
Re: [PATCHv3 0/2] target: make location of /var/targets configurable
On 04/14/2016 06:18 PM, Lee Duncan wrote: > These patches make the location of "/var/target" configurable, > though it still defauls to "/var/target". > > This "target database directory" can only be changed > after the target_core_mod loads but before any > fabric drivers are loaded, and must be the pathname > of an existing directory. > > This configuration is accomplished via the configfs > top-level target attribute "dbroot", i.e. dumping > out "/sys/kernel/config/target/dbroot" will normally > return "/var/target". Writing to this attribute > changes the loation where the kernel looks for the > target database. > > The first patch creates this configurable value for > the "dbroot", and the second patch modifies users > of this directory to use this new attribute. > > Changes from v2: > * Add locking around access to target driver list > > Changes from v1: > * Only allow changing target DB root before it >can be used by others > * Validate that new DB root is a valid directory > > Lee Duncan (2): > target: make target db location configurable > target: use new "dbroot" target attribute > > drivers/target/target_core_alua.c | 6 ++-- > drivers/target/target_core_configfs.c | 62 > +++++++ > drivers/target/target_core_internal.h | 6 > drivers/target/target_core_pr.c | 2 +- > 4 files changed, 72 insertions(+), 4 deletions(-) > Ping? -- Lee Duncan -- 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
Re: [PATCH v4] qla1280: Reduce can_queue to 512
On 04/25/2016 07:44 AM, Johannes Thumshirn wrote:
> The qla1280 driver sets the scsi_host_template's can_queue field to 0xf
> which results in an allocation failure when allocating the block layer tags
> for the driver's queues like the one shown below:
>
> [4.804166] scsi host0: QLogic QLA1040 PCI to SCSI Host Adapter Firmware
> version: 7.65.06, Driver version 3.27.1
> [4.804174] [ cut here ]
> [4.804184] WARNING: CPU: 2 PID: 305 at mm/page_alloc.c:2989
> alloc_pages_nodemask+0xae8/0xbc0()
> [4.804186] Modules linked in: amdkfd amd_iommu_v2 radeon i2c_algo_bit
> m_kms_helper ttm drm megaraid_sas serio_raw 8021q garp bnx2 stp llc mrp nhme
> qla1280(+) fjes
> [4.804208] CPU: 2 PID: 305 Comm: systemd-udevd Not tainted
> 4.6-201.fc22.x86_64 #1
> [4.804210] Hardware name: Google Enterprise Search Appliance/0DT021, OS
> 1.1.2 08/14/2006
> [4.804212] 0286 2f01064c 88042985b710
> ff813b542e
> [4.804216] 81a75024 88042985b748
> ff810a40f2
> [4.804220] 000b
> 00
> [4.804223] Call Trace:
> [4.804231] [] dump_stack+0x63/0x85
> [4.804236] [] warn_slowpath_common+0x82/0xc0
> [4.804239] [] warn_slowpath_null+0x1a/0x20
> [4.804242] [] __alloc_pages_nodemask+0xae8/0xbc0
> [4.804247] [] ? _raw_spin_unlock_irqrestore+0xe/0x10
> [4.804251] [] ? irq_work_queue+0x8e/0xa0
> [4.804256] [] ? console_unlock+0x20a/0x540
> [4.804262] [] alloc_pages_current+0x8c/0x110
> [4.804265] [] alloc_kmem_pages+0x19/0x90
> [4.804268] [] kmalloc_order_trace+0x2e/0xe0
> [4.804272] [] __kmalloc+0x232/0x260
> [4.804277] [] init_tag_map+0x3d/0xc0
> [4.804290] [] __blk_queue_init_tags+0x45/0x80
> [4.804293] [] blk_init_tags+0x14/0x20
> [4.804298] [] scsi_add_host_with_dma+0x80/0x300
> [4.804305] [] qla1280_probe_one+0x683/0x9ef [qla1280]
> [4.804309] [] local_pci_probe+0x45/0xa0
> [4.804312] [] pci_device_probe+0xfd/0x140
> [4.804316] [] driver_probe_device+0x222/0x490
> [4.804319] [] __driver_attach+0x84/0x90
> [4.804321] [] ? driver_probe_device+0x490/0x490
> [4.804324] [] bus_for_each_dev+0x6c/0xc0
> [4.804326] [] driver_attach+0x1e/0x20
> [4.804328] [] bus_add_driver+0x1eb/0x280
> [4.804331] [] ? 0xa0015000
> [4.804333] [] driver_register+0x60/0xe0
> [4.804336] [] __pci_register_driver+0x4c/0x50
> [4.804339] [] qla1280_init+0x1ce/0x1000 [qla1280]
> [4.804341] [] ? 0xa0015000
> [4.804345] [] do_one_initcall+0xb3/0x200
> [4.804348] [] ? kmem_cache_alloc_trace+0x196/0x210
> [4.804352] [] ? do_init_module+0x27/0x1cb
> [4.804354] [] do_init_module+0x5f/0x1cb
> [4.804358] [] load_module+0x2040/0x2680
> [4.804360] [] ? __symbol_put+0x60/0x60
> [4.804363] [] SYSC_init_module+0x149/0x190
> [4.804366] [] SyS_init_module+0xe/0x10
> [4.804369] [] entry_SYSCALL_64_fastpath+0x12/0x71
> [4.804371] ---[ end trace 0ea3b625f86705f7 ]---
> [4.804581] qla1280: probe of :11:04.0 failed with error -12
>
> Reduce can_queue to 512 to solve the allocation error.
>
> Signed-off-by: Johannes Thumshirn <jthumsh...@suse.de>
> Cc: Laura Abbott <labb...@redhat.com>
> Cc: Michael Reed <m...@sgi.com>
> Cc: sta...@vger.kernel.org
> Reviewed-by: Laurence Oberman <lober...@redhat.com>
> ---
> Changes to v3:
> * Use MAX_OUTSTANDING_COMMANDS insted of hard coded magical number
>
> Changes to v2:
> * Change can_queue to 512 upon James' request
>
> drivers/scsi/qla1280.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qla1280.c b/drivers/scsi/qla1280.c
> index 5d0ec42..634254a 100644
> --- a/drivers/scsi/qla1280.c
> +++ b/drivers/scsi/qla1280.c
> @@ -4214,7 +4214,7 @@ static struct scsi_host_template
> qla1280_driver_template = {
> .eh_bus_reset_handler = qla1280_eh_bus_reset,
> .eh_host_reset_handler = qla1280_eh_adapter_reset,
> .bios_param = qla1280_biosparam,
> - .can_queue = 0xf,
> + .can_queue = MAX_OUTSTANDING_COMMANDS,
> .this_id= -1,
> .sg_tablesize = SG_ALL,
> .use_clustering = ENABLE_CLUSTERING,
>
Reviewed-by: Lee Duncan <ldun...@suse.com>
--
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
[PATCHv3 1/2] target: make target db location configurable
This commit adds the read-write attribute "dbroot",
in the top-level CONFIGFS (core) target directory,
normally /sys/kernel/config/target. This attribute
defaults to "/var/target" but can be changed by
writing a new pathname string to it. Changing this
attribute is only allowed when no fabric drivers
are loaded and the supplied value specifies an
existing directory.
Target modules that care about the target database
root directory will be modified to use this
attribute in a future commit.
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/target/target_core_configfs.c | 62 +++
drivers/target/target_core_internal.h | 6
2 files changed, 68 insertions(+)
diff --git a/drivers/target/target_core_configfs.c
b/drivers/target/target_core_configfs.c
index 713c63d9681b..8cce79317971 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -99,6 +99,67 @@ static ssize_t target_core_item_version_show(struct
config_item *item,
CONFIGFS_ATTR_RO(target_core_item_, version);
+char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
+static char db_root_stage[DB_ROOT_LEN];
+
+static ssize_t target_core_item_dbroot_show(struct config_item *item,
+ char *page)
+{
+ return sprintf(page, "%s\n", db_root);
+}
+
+static ssize_t target_core_item_dbroot_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ ssize_t read_bytes;
+ struct file *fp;
+
+ mutex_lock(_tf_lock);
+ if (!list_empty(_tf_list)) {
+ mutex_unlock(_tf_lock);
+ pr_err("db_root: cannot be changed: target drivers registered");
+ return -EINVAL;
+ }
+
+ if (count > (DB_ROOT_LEN - 1)) {
+ mutex_unlock(_tf_lock);
+ pr_err("db_root: count %d exceeds DB_ROOT_LEN-1: %u\n",
+ (int)count, DB_ROOT_LEN - 1);
+ return -EINVAL;
+ }
+
+ read_bytes = snprintf(db_root_stage, DB_ROOT_LEN, "%s", page);
+ if (!read_bytes) {
+ mutex_unlock(_tf_lock);
+ return -EINVAL;
+ }
+ if (db_root_stage[read_bytes - 1] == '\n')
+ db_root_stage[read_bytes - 1] = '\0';
+
+ /* validate new db root before accepting it */
+ fp = filp_open(db_root_stage, O_RDONLY, 0);
+ if (IS_ERR(fp)) {
+ mutex_unlock(_tf_lock);
+ pr_err("db_root: cannot open: %s\n", db_root_stage);
+ return -EINVAL;
+ }
+ if (!S_ISDIR(fp->f_inode->i_mode)) {
+ filp_close(fp, 0);
+ mutex_unlock(_tf_lock);
+ pr_err("db_root: not a directory: %s\n", db_root_stage);
+ return -EINVAL;
+ }
+ filp_close(fp, 0);
+
+ strncpy(db_root, db_root_stage, read_bytes);
+
+ mutex_unlock(_tf_lock);
+
+ return read_bytes;
+}
+
+CONFIGFS_ATTR(target_core_item_, dbroot);
+
static struct target_fabric_configfs *target_core_get_fabric(
const char *name)
{
@@ -249,6 +310,7 @@ static struct configfs_group_operations
target_core_fabric_group_ops = {
*/
static struct configfs_attribute *target_core_fabric_item_attrs[] = {
_core_item_attr_version,
+ _core_item_attr_dbroot,
NULL,
};
diff --git a/drivers/target/target_core_internal.h
b/drivers/target/target_core_internal.h
index 040cf5202e54..c2a18b960c5d 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -156,4 +156,10 @@ void
target_stat_setup_mappedlun_default_groups(struct se_lun_acl *);
/* target_core_xcopy.c */
extern struct se_portal_group xcopy_pt_tpg;
+/* target_core_configfs.c */
+#define DB_ROOT_LEN4096
+#defineDB_ROOT_DEFAULT "/var/target"
+
+extern char db_root[];
+
#endif /* TARGET_CORE_INTERNAL_H */
--
2.1.4
--
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
[PATCHv3 2/2] target: use new "dbroot" target attribute
This commit updates the target core ALUA and PR
modules to use the new "dbroot" attribute instead
of assuming the target database is in "/var/target".
Signed-off-by: Lee Duncan <ldun...@suse.com>
Reviewed-by: Hannes Reinecke <h...@suse.com>
---
drivers/target/target_core_alua.c | 6 +++---
drivers/target/target_core_pr.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/target/target_core_alua.c
b/drivers/target/target_core_alua.c
index 49aba4a31747..4c82bbe19003 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -932,7 +932,7 @@ static int core_alua_update_tpg_primary_metadata(
tg_pt_gp->tg_pt_gp_alua_access_status);
snprintf(path, ALUA_METADATA_PATH_LEN,
- "/var/target/alua/tpgs_%s/%s", >unit_serial[0],
+ "%s/alua/tpgs_%s/%s", db_root, >unit_serial[0],
config_item_name(_pt_gp->tg_pt_gp_group.cg_item));
rc = core_alua_write_tpg_metadata(path, md_buf, len);
@@ -1275,8 +1275,8 @@ static int core_alua_update_tpg_secondary_metadata(struct
se_lun *lun)
atomic_read(>lun_tg_pt_secondary_offline),
lun->lun_tg_pt_secondary_stat);
- snprintf(path, ALUA_METADATA_PATH_LEN,
"/var/target/alua/%s/%s/lun_%llu",
- se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
+ snprintf(path, ALUA_METADATA_PATH_LEN, "%s/alua/%s/%s/lun_%llu",
+ db_root, se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
lun->unpacked_lun);
rc = core_alua_write_tpg_metadata(path, md_buf, len);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index b1795735eafc..47463c99c318 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1985,7 +1985,7 @@ static int __core_scsi3_write_aptpl_to_file(
return -EMSGSIZE;
}
- snprintf(path, 512, "/var/target/pr/aptpl_%s", >unit_serial[0]);
+ snprintf(path, 512, "%s/pr/aptpl_%s", db_root, >unit_serial[0]);
file = filp_open(path, flags, 0600);
if (IS_ERR(file)) {
pr_err("filp_open(%s) for APTPL metadata"
--
2.1.4
--
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
[PATCHv3 0/2] target: make location of /var/targets configurable
These patches make the location of "/var/target" configurable, though it still defauls to "/var/target". This "target database directory" can only be changed after the target_core_mod loads but before any fabric drivers are loaded, and must be the pathname of an existing directory. This configuration is accomplished via the configfs top-level target attribute "dbroot", i.e. dumping out "/sys/kernel/config/target/dbroot" will normally return "/var/target". Writing to this attribute changes the loation where the kernel looks for the target database. The first patch creates this configurable value for the "dbroot", and the second patch modifies users of this directory to use this new attribute. Changes from v2: * Add locking around access to target driver list Changes from v1: * Only allow changing target DB root before it can be used by others * Validate that new DB root is a valid directory Lee Duncan (2): target: make target db location configurable target: use new "dbroot" target attribute drivers/target/target_core_alua.c | 6 ++-- drivers/target/target_core_configfs.c | 62 +++ drivers/target/target_core_internal.h | 6 drivers/target/target_core_pr.c | 2 +- 4 files changed, 72 insertions(+), 4 deletions(-) -- 2.1.4 -- 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
Re: [PATCHv2 1/2] target: make target db location configurable
On 04/13/2016 11:10 PM, Hannes Reinecke wrote:
> On 04/13/2016 10:25 PM, Lee Duncan wrote:
>> This commit adds the read-write attribute "dbroot",
>> in the top-level CONFIGFS (core) target directory,
>> normally /sys/kernel/config/target. This attribute
>> defaults to "/var/target" but can be changed by
>> writing a new pathname string to it. Changing this
>> attribute is only allowed when no fabric drivers
>> are loaded and the supplied value specifies an
>> existing directory.
>>
>> Target modules that care about the target database
>> root directory will be modified to use this
>> attribute in a future commit.
>>
>> Signed-off-by: Lee Duncan <ldun...@suse.com>
>> ---
>> drivers/target/target_core_configfs.c | 51
>> +++
>> drivers/target/target_core_internal.h | 6 +
>> 2 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/target/target_core_configfs.c
>> b/drivers/target/target_core_configfs.c
>> index 713c63d9681b..bfedbd92b77f 100644
>> --- a/drivers/target/target_core_configfs.c
>> +++ b/drivers/target/target_core_configfs.c
>> @@ -99,6 +99,56 @@ static ssize_t target_core_item_version_show(struct
>> config_item *item,
>>
>> CONFIGFS_ATTR_RO(target_core_item_, version);
>>
>> +char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
>> +static char db_root_stage[DB_ROOT_LEN];
>> +
>> +static ssize_t target_core_item_dbroot_show(struct config_item *item,
>> +char *page)
>> +{
>> +return sprintf(page, "%s\n", db_root);
>> +}
>> +
>> +static ssize_t target_core_item_dbroot_store(struct config_item *item,
>> +const char *page, size_t count)
>> +{
>> +ssize_t read_bytes;
>> +struct file *fp;
>> +
>> +if (!list_empty(_tf_list)) {
>> +pr_err("db_root: cannot be changed: target drivers registered");
>> +return -EINVAL;
>> +}
> Locking?
Doh. I will resubmit with locking shortly.
>
> Cheers,
>
> Hannes
>
--
Lee Duncan
--
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
[PATCHv2 1/2] target: make target db location configurable
This commit adds the read-write attribute "dbroot",
in the top-level CONFIGFS (core) target directory,
normally /sys/kernel/config/target. This attribute
defaults to "/var/target" but can be changed by
writing a new pathname string to it. Changing this
attribute is only allowed when no fabric drivers
are loaded and the supplied value specifies an
existing directory.
Target modules that care about the target database
root directory will be modified to use this
attribute in a future commit.
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/target/target_core_configfs.c | 51 +++
drivers/target/target_core_internal.h | 6 +
2 files changed, 57 insertions(+)
diff --git a/drivers/target/target_core_configfs.c
b/drivers/target/target_core_configfs.c
index 713c63d9681b..bfedbd92b77f 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -99,6 +99,56 @@ static ssize_t target_core_item_version_show(struct
config_item *item,
CONFIGFS_ATTR_RO(target_core_item_, version);
+char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
+static char db_root_stage[DB_ROOT_LEN];
+
+static ssize_t target_core_item_dbroot_show(struct config_item *item,
+ char *page)
+{
+ return sprintf(page, "%s\n", db_root);
+}
+
+static ssize_t target_core_item_dbroot_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ ssize_t read_bytes;
+ struct file *fp;
+
+ if (!list_empty(_tf_list)) {
+ pr_err("db_root: cannot be changed: target drivers registered");
+ return -EINVAL;
+ }
+ if (count > (DB_ROOT_LEN - 1)) {
+ pr_err("db_root: count %d exceeds DB_ROOT_LEN-1: %u\n",
+ (int)count, DB_ROOT_LEN - 1);
+ return -EINVAL;
+ }
+
+ read_bytes = snprintf(db_root_stage, DB_ROOT_LEN, "%s", page);
+ if (!read_bytes)
+ return -EINVAL;
+ if (db_root_stage[read_bytes - 1] == '\n')
+ db_root_stage[read_bytes - 1] = '\0';
+
+ /* validate new db root before accepting it */
+ fp = filp_open(db_root_stage, O_RDONLY, 0);
+ if (IS_ERR(fp)) {
+ pr_err("db_root: cannot open: %s\n", db_root_stage);
+ return -EINVAL;
+ }
+ if (!S_ISDIR(fp->f_inode->i_mode)) {
+ pr_err("db_root: not a directory: %s\n", db_root_stage);
+ filp_close(fp, 0);
+ return -EINVAL;
+ }
+ filp_close(fp, 0);
+ strncpy(db_root, db_root_stage, read_bytes);
+
+ return read_bytes;
+}
+
+CONFIGFS_ATTR(target_core_item_, dbroot);
+
static struct target_fabric_configfs *target_core_get_fabric(
const char *name)
{
@@ -249,6 +299,7 @@ static struct configfs_group_operations
target_core_fabric_group_ops = {
*/
static struct configfs_attribute *target_core_fabric_item_attrs[] = {
_core_item_attr_version,
+ _core_item_attr_dbroot,
NULL,
};
diff --git a/drivers/target/target_core_internal.h
b/drivers/target/target_core_internal.h
index 040cf5202e54..c2a18b960c5d 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -156,4 +156,10 @@ void
target_stat_setup_mappedlun_default_groups(struct se_lun_acl *);
/* target_core_xcopy.c */
extern struct se_portal_group xcopy_pt_tpg;
+/* target_core_configfs.c */
+#define DB_ROOT_LEN4096
+#defineDB_ROOT_DEFAULT "/var/target"
+
+extern char db_root[];
+
#endif /* TARGET_CORE_INTERNAL_H */
--
2.1.4
--
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
[PATCHv2 0/2] target: make location of /var/targets configurable
These patches make the location of "/var/target" configurable, though it still defauls to "/var/target". This "target database directory" can only be changed after the target_core_mod loads but before any fabric drivers are loaded, and must be the pathname of an existing directory. This configuration is accomplished via the configfs top-level target attribute "dbroot", i.e. dumping out "/sys/kernel/config/target/dbroot" will normally return "/var/target". Writing to this attribute changes the loation where the kernel looks for the target database. The first patch creates this configurable value for the "dbroot", and the second patch modifies users of this directory to use this new attribute. Changes from v1: * Only allow changing target DB root before it can be used by others * Validate that new DB root is a valid directory Lee Duncan (2): target: make target db location configurable target: use new "dbroot" target attribute drivers/target/target_core_alua.c | 6 ++--- drivers/target/target_core_configfs.c | 51 +++ drivers/target/target_core_internal.h | 6 + drivers/target/target_core_pr.c | 2 +- 4 files changed, 61 insertions(+), 4 deletions(-) -- 2.1.4 -- 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
[PATCH 2/2] target: use new "dbroot" target attribute
This commit updates the target core ALUA and PR
modules to use the new "dbroot" attribute instead
of assuming the target database is in "/var/target".
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/target/target_core_alua.c | 6 +++---
drivers/target/target_core_pr.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/target/target_core_alua.c
b/drivers/target/target_core_alua.c
index 49aba4a31747..4c82bbe19003 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -932,7 +932,7 @@ static int core_alua_update_tpg_primary_metadata(
tg_pt_gp->tg_pt_gp_alua_access_status);
snprintf(path, ALUA_METADATA_PATH_LEN,
- "/var/target/alua/tpgs_%s/%s", >unit_serial[0],
+ "%s/alua/tpgs_%s/%s", db_root, >unit_serial[0],
config_item_name(_pt_gp->tg_pt_gp_group.cg_item));
rc = core_alua_write_tpg_metadata(path, md_buf, len);
@@ -1275,8 +1275,8 @@ static int core_alua_update_tpg_secondary_metadata(struct
se_lun *lun)
atomic_read(>lun_tg_pt_secondary_offline),
lun->lun_tg_pt_secondary_stat);
- snprintf(path, ALUA_METADATA_PATH_LEN,
"/var/target/alua/%s/%s/lun_%llu",
- se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
+ snprintf(path, ALUA_METADATA_PATH_LEN, "%s/alua/%s/%s/lun_%llu",
+ db_root, se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
lun->unpacked_lun);
rc = core_alua_write_tpg_metadata(path, md_buf, len);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index b1795735eafc..47463c99c318 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1985,7 +1985,7 @@ static int __core_scsi3_write_aptpl_to_file(
return -EMSGSIZE;
}
- snprintf(path, 512, "/var/target/pr/aptpl_%s", >unit_serial[0]);
+ snprintf(path, 512, "%s/pr/aptpl_%s", db_root, >unit_serial[0]);
file = filp_open(path, flags, 0600);
if (IS_ERR(file)) {
pr_err("filp_open(%s) for APTPL metadata"
--
2.1.4
--
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
Re: [PATCH] sd: fixup capacity calculation for 4k drives
tor_size < 0)
> - sector_size = read_capacity_10(sdkp, sdp, buffer);
> + sector_size = read_capacity_10(sdkp, sdp, buffer,
> +_capacity);
> if (sector_size < 0)
> return;
> } else {
> - sector_size = read_capacity_10(sdkp, sdp, buffer);
> + sector_size = read_capacity_10(sdkp, sdp, buffer,
> +_capacity);
> if (sector_size == -EOVERFLOW)
> goto got_data;
> if (sector_size < 0)
> return;
> if ((sizeof(sdkp->capacity) > 4) &&
> - (sdkp->capacity > 0xULL)) {
> + (new_capacity > 0xULL)) {
> int old_sector_size = sector_size;
> sd_printk(KERN_NOTICE, sdkp, "Very big device. "
> "Trying to use READ CAPACITY(16).\n");
> - sector_size = read_capacity_16(sdkp, sdp, buffer);
> + sector_size = read_capacity_16(sdkp, sdp, buffer,
> +_capacity);
> if (sector_size < 0) {
> sd_printk(KERN_NOTICE, sdkp,
> "Using 0x as device size\n");
> - sdkp->capacity = 1 + (sector_t) 0x;
> + new_capacity = 1 + (u64) 0x;
> sector_size = old_sector_size;
> goto got_data;
> }
> @@ -2275,11 +2279,11 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned
> char *buffer)
>* the capacity.
>*/
> if (sdp->fix_capacity ||
> - (sdp->guess_capacity && (sdkp->capacity & 0x01))) {
> + (sdp->guess_capacity && (new_capacity & 0x01))) {
> sd_printk(KERN_INFO, sdkp, "Adjusting the sector count "
> "from its reported value: %llu\n",
> - (unsigned long long) sdkp->capacity);
> - --sdkp->capacity;
> + (unsigned long long) new_capacity);
> + --new_capacity;
> }
>
> got_data:
> @@ -2301,7 +2305,7 @@ got_data:
>* would be relatively trivial to set the thing up.
>* For this reason, we leave the thing in the table.
>*/
> - sdkp->capacity = 0;
> + new_capacity = 0;
> /*
>* set a bogus sector size so the normal read/write
>* logic in the block layer will eventually refuse any
> @@ -2312,6 +2316,19 @@ got_data:
> }
> blk_queue_logical_block_size(sdp->request_queue, sector_size);
>
> + /*
> + * Note: up to this point new_capacity carries the
> + * _unscaled_ capacity. So rescale capacity to 512-byte units.
> + */
> + if (sector_size == 4096)
> + sdkp->capacity = new_capacity << 3;
> + else if (sector_size == 2048)
> + sdkp->capacity = new_capacity << 2;
> + else if (sector_size == 1024)
> + sdkp->capacity = new_capacity << 1;
> + else
> + sdkp->capacity = new_capacity;
> +
> {
> char cap_str_2[10], cap_str_10[10];
>
> @@ -2337,14 +2354,6 @@ got_data:
> if (sdkp->capacity > 0x)
> sdp->use_16_for_rw = 1;
>
> - /* Rescale capacity to 512-byte units */
> - if (sector_size == 4096)
> - sdkp->capacity <<= 3;
> - else if (sector_size == 2048)
> - sdkp->capacity <<= 2;
> - else if (sector_size == 1024)
> - sdkp->capacity <<= 1;
> -
> blk_queue_physical_block_size(sdp->request_queue,
> sdkp->physical_block_size);
> sdkp->device->sector_size = sector_size;
>
Reviewed-by: Lee Duncan <ldun...@suse.com>
--
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
Re: [PATCH 0/2] target: make location of /var/target configurable
On 04/02/2016 08:36 PM, Nicholas A. Bellinger wrote: > On Thu, 2016-03-31 at 11:05 -0700, Lee Duncan wrote: >> These patches make the location of "/var/target" configurable, >> though it still defauls to "/var/target". >> >> This configuration is accomplished via the configfs >> top-level target attribute "dbroot", i.e. dumping >> out "/sys/kernel/config/target/dbroot" will normally >> return "/var/target". Writing to this attribute >> changes the loation where the kernel looks for the >> target database. >> >> ** NOTE/QUESTION: no sanity checks are done on the path passed in, >>but it seems like *some* should be done. At least checking that >>it's an abosolute path (i.e. starts with '/')? Opinions? >> > > Wrt to sanity checking db_root at configfs attribute store time, how > about doing a filp_open() + S_DIR(f_inode->imode) + filp_close() of the > requested path to verify it's really a directory..? That seems reasonable. I will try that out and add it assuming it works. :) > > Also, it would probably be a good idea to limit when db_root can be > changed. Eg, only allow db_root to be changed when no active target > fabric drivers have been registered (list_empty(g_tf_list)), and require > userspace to set a different db_root after modprobe target_core_mod > completes, but before any fabric drivers are loaded. > I will also try that. Would it be worthwhile to have a module parameter for target_core_mod to set the root, so it could be accomplished at the right time? (As much as I like playing with configfs.) > > > > -- > 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 > -- Lee Duncan SUSE Labs -- 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
Re: [PATCH 2/2] target: use new "dbroot" target attribute
On 04/01/2016 11:18 AM, Andy Grover wrote: > On 04/01/2016 11:01 AM, Lee Duncan wrote: >> On 04/01/2016 12:58 AM, Johannes Thumshirn wrote: >>> On 2016-03-31 20:05, Lee Duncan wrote: >>>> This commit updates the target core ALUA and PR >>>> modules to use the new "dbroot" attribute instead >>>> of assuming the target database is in "/var/target". >>> >>> Same goes for this one, >>> >>> Reviewed-by: Johannes Thumshirn <jthumsh...@suse.de> >>> as soon as it has a Signed-off-by line >> >> Thanks Johannes. >> >> I will wait to see if there are any other comments, then resubmit v2. > > Seems fine to me, too. Thank you for your review. > > So, if not /var/target, where do you recommend we be pointing this to? > Good question! For testing, I put it in /etc/target/target_db. But /etc is supposed to be for configuration data. Part of my problem in picking a place was that there seems to be two different kinds of data there: policy, and state. Since it was not my intention to sort that out, I just picked the /etc location mentioned and verified it could work. -- 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
Re: [PATCH 2/2] target: use new "dbroot" target attribute
On 04/01/2016 12:58 AM, Johannes Thumshirn wrote: > On 2016-03-31 20:05, Lee Duncan wrote: >> This commit updates the target core ALUA and PR >> modules to use the new "dbroot" attribute instead >> of assuming the target database is in "/var/target". > > Same goes for this one, > > Reviewed-by: Johannes Thumshirn <jthumsh...@suse.de> > as soon as it has a Signed-off-by line Thanks Johannes. I will wait to see if there are any other comments, then resubmit v2. -- Lee Duncan SUSE Labs -- 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
[PATCH 0/2] target: make location of /var/target configurable
These patches make the location of "/var/target" configurable, though it still defauls to "/var/target". This configuration is accomplished via the configfs top-level target attribute "dbroot", i.e. dumping out "/sys/kernel/config/target/dbroot" will normally return "/var/target". Writing to this attribute changes the loation where the kernel looks for the target database. ** NOTE/QUESTION: no sanity checks are done on the path passed in, but it seems like *some* should be done. At least checking that it's an abosolute path (i.e. starts with '/')? Opinions? Lee Duncan (2): Make target db location /var/targets configurable. Use the new "dbroot" configfs target attribute. drivers/target/target_core_alua.c | 6 +++--- drivers/target/target_core_configfs.c | 31 +++ drivers/target/target_core_internal.h | 6 ++ drivers/target/target_core_pr.c | 2 +- 4 files changed, 41 insertions(+), 4 deletions(-) -- 2.1.4 -- 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
[PATCH 2/2] target: use new "dbroot" target attribute
This commit updates the target core ALUA and PR
modules to use the new "dbroot" attribute instead
of assuming the target database is in "/var/target".
---
drivers/target/target_core_alua.c | 6 +++---
drivers/target/target_core_pr.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/target/target_core_alua.c
b/drivers/target/target_core_alua.c
index 49aba4a31747..22a6a9b18a86 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -932,7 +932,7 @@ static int core_alua_update_tpg_primary_metadata(
tg_pt_gp->tg_pt_gp_alua_access_status);
snprintf(path, ALUA_METADATA_PATH_LEN,
- "/var/target/alua/tpgs_%s/%s", >unit_serial[0],
+ "%s/alua/tpgs_%s/%s", db_root, >unit_serial[0],
config_item_name(_pt_gp->tg_pt_gp_group.cg_item));
rc = core_alua_write_tpg_metadata(path, md_buf, len);
@@ -1275,8 +1275,8 @@ static int core_alua_update_tpg_secondary_metadata(struct
se_lun *lun)
atomic_read(>lun_tg_pt_secondary_offline),
lun->lun_tg_pt_secondary_stat);
- snprintf(path, ALUA_METADATA_PATH_LEN,
"/var/target/alua/%s/%s/lun_%llu",
- se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
+ snprintf(path, ALUA_METADATA_PATH_LEN, "%s/alua/%s/%s/lun_%llu",
+ db_root, se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
lun->unpacked_lun);
rc = core_alua_write_tpg_metadata(path, md_buf, len);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index b1795735eafc..47463c99c318 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1985,7 +1985,7 @@ static int __core_scsi3_write_aptpl_to_file(
return -EMSGSIZE;
}
- snprintf(path, 512, "/var/target/pr/aptpl_%s", >unit_serial[0]);
+ snprintf(path, 512, "%s/pr/aptpl_%s", db_root, >unit_serial[0]);
file = filp_open(path, flags, 0600);
if (IS_ERR(file)) {
pr_err("filp_open(%s) for APTPL metadata"
--
2.1.4
--
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
[PATCH 1/2] target: Make target db location configurable
This commit adds the read-write attribute "dbroot", in
the top-level CONFIGFS (core) target directory,
normally /sys/kernel/config/target. This attribute
defaults to "/var/target" but can be changed by
writing a new pathname string to it.
Target modules that care about the target database
root directory will be modified to use this
attribute in a future commit.
---
drivers/target/target_core_configfs.c | 31 +++
drivers/target/target_core_internal.h | 6 ++
2 files changed, 37 insertions(+)
diff --git a/drivers/target/target_core_configfs.c
b/drivers/target/target_core_configfs.c
index 713c63d9681b..bfc5a8bb5778 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -99,6 +99,36 @@ static ssize_t target_core_item_version_show(struct
config_item *item,
CONFIGFS_ATTR_RO(target_core_item_, version);
+char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
+
+static ssize_t target_core_item_dbroot_show(struct config_item *item,
+ char *page)
+{
+ return sprintf(page, "%s\n", db_root);
+}
+
+static ssize_t target_core_item_dbroot_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ ssize_t read_bytes;
+
+ if (count > (DB_ROOT_LEN - 1)) {
+ pr_err("db_root count: %d exceeds DB_ROOT_LEN-1: %u\n",
+ (int)count, DB_ROOT_LEN - 1);
+ return -EINVAL;
+ }
+
+ read_bytes = snprintf(db_root, DB_ROOT_LEN, "%s", page);
+ if (!read_bytes)
+ return -EINVAL;
+ if (db_root[read_bytes - 1] == '\n')
+ db_root[read_bytes - 1] = '\0';
+
+ return read_bytes;
+}
+
+CONFIGFS_ATTR(target_core_item_, dbroot);
+
static struct target_fabric_configfs *target_core_get_fabric(
const char *name)
{
@@ -249,6 +279,7 @@ static struct configfs_group_operations
target_core_fabric_group_ops = {
*/
static struct configfs_attribute *target_core_fabric_item_attrs[] = {
_core_item_attr_version,
+ _core_item_attr_dbroot,
NULL,
};
diff --git a/drivers/target/target_core_internal.h
b/drivers/target/target_core_internal.h
index 040cf5202e54..c2a18b960c5d 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -156,4 +156,10 @@ void
target_stat_setup_mappedlun_default_groups(struct se_lun_acl *);
/* target_core_xcopy.c */
extern struct se_portal_group xcopy_pt_tpg;
+/* target_core_configfs.c */
+#define DB_ROOT_LEN4096
+#defineDB_ROOT_DEFAULT "/var/target"
+
+extern char db_root[];
+
#endif /* TARGET_CORE_INTERNAL_H */
--
2.1.4
--
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
Re: [PATCH] scsi_common: do not clobber fixed sense information
On 03/18/2016 01:01 AM, Hannes Reinecke wrote:
> For fixed sense the information field is 32 bits, to we need
> to truncate the information field to avoid clobbering the
> sense code.
>
> Signed-off-by: Hannes Reinecke <h...@suse.com>
> ---
> drivers/scsi/scsi_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
> index c126966..3459009 100644
> --- a/drivers/scsi/scsi_common.c
> +++ b/drivers/scsi/scsi_common.c
> @@ -279,7 +279,7 @@ int scsi_set_sense_information(u8 *buf, int buf_len, u64
> info)
> put_unaligned_be64(info, [4]);
> } else if ((buf[0] & 0x7f) == 0x70) {
> buf[0] |= 0x80;
> - put_unaligned_be64(info, [3]);
> + put_unaligned_be32((u32)info, [3]);
> }
>
> return 0;
>
Reviewed-by: Lee Duncan <ldun...@suse.com>
--
Lee
--
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
Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
On 02/12/2016 09:54 AM, James Bottomley wrote: > On Fri, 2016-02-12 at 09:38 -0800, Lee Duncan wrote: >> The scsi_transport_iscsi module already uses the ida_simple >> routines for managing the target ID, if requested to do >> so. This change replaces an ever-increasing atomic integer >> that tracks the session ID itself with the ida_simple >> family of routines. This means that the session ID >> will be reclaimed and can be reused when the session >> is freed. > > Is reusing session ID's really a good idea? For sequential sessions it > means that the ID of the next session will be re-used, i.e. the same as > the previous sessions, which could lead to target confusion. I think > local uniqueness of session IDs is more important than wrap around > because sessions are short lived entities and the chances of the same > session being alive by the time we've wrapped is pretty tiny. > > If you can demostrate a multi-target problem, perhaps we should rather > fix this by making the next session id a target local quantity? > > James > It looks like Mike and Chris are good with it. And I'd really like to get rid of yet another atomic int. Are you satisfied with this one? -- Lee -- 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
Re: [PATCHv2] iscsi_ibft: Add prefix-len attr and display netmask
On 02/29/2016 01:04 PM, Konrad Rzeszutek Wilk wrote: > On Mon, Feb 29, 2016 at 1:45 PM, Mike Christie <micha...@cs.wisc.edu> wrote: >> On 02/25/2016 12:15 PM, Lee Duncan wrote: >>> From: Hannes Reinecke <h...@suse.de> >>> >>> The iBFT table only specifies a prefix length, not a netmask. >>> And the netmask is pretty much pointless for IPv6. >>> So introduce a new attribute 'prefix-len' and display the >>> netmask attribute only for IPv4 addresses. >> >> The code looks ok to me, but I think this ipv4 comment was left over >> from the last posting since it conflicts with the comment below about >> always displaying it. Maybe it can just be edited when the patch is >> merged to avoid having to resend again. If so, >> >> Reviewed-by: Mike Christie <micha...@cs.wisc.edu> >> > > Cool. Let me roll it up and send the git pull to Linus. I actually just resent it, but the only change was to truncate the last line of first paragraph of the comment to: So introduce a new attribute 'prefix-len'. > > This does not seem be super-urgent so it can wait for the upcoming merge > window? > >> >>> >>> Some older user-space code might rely on the netmask attribute >>> being present, so we should always display it. >>> >>> Changes from v1: >>> - Combined two patches into one >>> >>> Signed-off-by: Hannes Reinecke <h...@suse.de> >>> Signed-off-by: Lee Duncan <ldun...@suse.com> > -- Lee Duncan SUSE Labs -- 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
Re: [PATCHv2] iscsi_ibft: Add prefix-len attr and display netmask
On 02/29/2016 10:45 AM, Mike Christie wrote: > On 02/25/2016 12:15 PM, Lee Duncan wrote: >> From: Hannes Reinecke <h...@suse.de> >> >> The iBFT table only specifies a prefix length, not a netmask. >> And the netmask is pretty much pointless for IPv6. >> So introduce a new attribute 'prefix-len' and display the >> netmask attribute only for IPv4 addresses. > > The code looks ok to me, but I think this ipv4 comment was left over > from the last posting since it conflicts with the comment below about > always displaying it. Maybe it can just be edited when the patch is > merged to avoid having to resend again. If so, > > Reviewed-by: Mike Christie <micha...@cs.wisc.edu> > > ... Since there are no other comments, I will resubmit it with the comment fixed. -- Lee Duncan -- 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
Re: [LSF/MM TOPIC][LSF/MM ATTEND] LIO Target Synchronization with sysfs [resend]
On 02/27/2016 01:54 PM, Nicholas A. Bellinger wrote: > On Sat, 2016-02-27 at 08:19 -0800, Lee Duncan wrote: >> [Apologies for the resend.] >> >> I would like to attend LSF/MM this year. I would like to discuss >> problems I've had dealing with LIO targets and their inherent >> asynchronicity in sysfs. I believe that with judicious use of kernel >> events we can help user space handle target changes more cleanly. >> > > I assume you mean configfs, and not sysfs. ;) Apologies. Yes. > >> As for other topics: >> >> As one of the maintainers of LIO at SUSE, I'd dearly love to see our >> iSCSI targets merged. >> > > You're talking about Mike's (CC'ed) original target_core_rbd.c driver, > right..? > > He's been working on porting the PR related changes to a generic > interface that can be consumed by multiple backend using > target_core_iblock.c. > Actually, no. I'm aware of that work because one of the groups I'm in is working in that area. And that is of interest to me to. But I meant the scst/lio merge, one of the proposed LSF/MM topics. -- Lee Duncan -- 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
[LSF/MM TOPIC][LSF/MM ATTEND] LIO Target Synchronization with sysfs [resend]
[Apologies for the resend.] I would like to attend LSF/MM this year. I would like to discuss problems I've had dealing with LIO targets and their inherent asynchronicity in sysfs. I believe that with judicious use of kernel events we can help user space handle target changes more cleanly. As for other topics: As one of the maintainers of LIO at SUSE, I'd dearly love to see our iSCSI targets merged. I also would love to see multipath work in a sane way, and *with* the SCSI subsystem, not on top of it, so I'd like to join that discussion. -- Lee Duncan -- 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
[PATCHv2] iscsi_ibft: Add prefix-len attr and display netmask
From: Hannes Reinecke <h...@suse.de>
The iBFT table only specifies a prefix length, not a netmask.
And the netmask is pretty much pointless for IPv6.
So introduce a new attribute 'prefix-len' and display the
netmask attribute only for IPv4 addresses.
Some older user-space code might rely on the netmask attribute
being present, so we should always display it.
Changes from v1:
- Combined two patches into one
Signed-off-by: Hannes Reinecke <h...@suse.de>
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/firmware/iscsi_ibft.c| 4
drivers/scsi/iscsi_boot_sysfs.c | 5 +
include/linux/iscsi_boot_sysfs.h | 1 +
3 files changed, 10 insertions(+)
diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c
index 72791232e46b..81037e5fe301 100644
--- a/drivers/firmware/iscsi_ibft.c
+++ b/drivers/firmware/iscsi_ibft.c
@@ -319,6 +319,9 @@ static ssize_t ibft_attr_show_nic(void *data, int type,
char *buf)
val = cpu_to_be32(~((1 << (32-nic->subnet_mask_prefix))-1));
str += sprintf(str, "%pI4", );
break;
+ case ISCSI_BOOT_ETH_PREFIX_LEN:
+ str += sprintf(str, "%d\n", nic->subnet_mask_prefix);
+ break;
case ISCSI_BOOT_ETH_ORIGIN:
str += sprintf(str, "%d\n", nic->origin);
break;
@@ -460,6 +463,7 @@ static umode_t ibft_check_nic_for(void *data, int type)
if (address_not_null(nic->ip_addr))
rc = S_IRUGO;
break;
+ case ISCSI_BOOT_ETH_PREFIX_LEN:
case ISCSI_BOOT_ETH_SUBNET_MASK:
if (nic->subnet_mask_prefix)
rc = S_IRUGO;
diff --git a/drivers/scsi/iscsi_boot_sysfs.c b/drivers/scsi/iscsi_boot_sysfs.c
index 680bf6f0ce76..8f0ea97cf31f 100644
--- a/drivers/scsi/iscsi_boot_sysfs.c
+++ b/drivers/scsi/iscsi_boot_sysfs.c
@@ -166,6 +166,7 @@ static struct attribute_group iscsi_boot_target_attr_group
= {
iscsi_boot_rd_attr(eth_index, index, ISCSI_BOOT_ETH_INDEX);
iscsi_boot_rd_attr(eth_flags, flags, ISCSI_BOOT_ETH_FLAGS);
iscsi_boot_rd_attr(eth_ip, ip-addr, ISCSI_BOOT_ETH_IP_ADDR);
+iscsi_boot_rd_attr(eth_prefix, prefix-len, ISCSI_BOOT_ETH_PREFIX_LEN);
iscsi_boot_rd_attr(eth_subnet, subnet-mask, ISCSI_BOOT_ETH_SUBNET_MASK);
iscsi_boot_rd_attr(eth_origin, origin, ISCSI_BOOT_ETH_ORIGIN);
iscsi_boot_rd_attr(eth_gateway, gateway, ISCSI_BOOT_ETH_GATEWAY);
@@ -181,6 +182,7 @@ static struct attribute *ethernet_attrs[] = {
_boot_attr_eth_index.attr,
_boot_attr_eth_flags.attr,
_boot_attr_eth_ip.attr,
+ _boot_attr_eth_prefix.attr,
_boot_attr_eth_subnet.attr,
_boot_attr_eth_origin.attr,
_boot_attr_eth_gateway.attr,
@@ -208,6 +210,9 @@ static umode_t iscsi_boot_eth_attr_is_visible(struct
kobject *kobj,
else if (attr == _boot_attr_eth_ip.attr)
return boot_kobj->is_visible(boot_kobj->data,
ISCSI_BOOT_ETH_IP_ADDR);
+ else if (attr == _boot_attr_eth_prefix.attr)
+ return boot_kobj->is_visible(boot_kobj->data,
+ISCSI_BOOT_ETH_PREFIX_LEN);
else if (attr == _boot_attr_eth_subnet.attr)
return boot_kobj->is_visible(boot_kobj->data,
ISCSI_BOOT_ETH_SUBNET_MASK);
diff --git a/include/linux/iscsi_boot_sysfs.h b/include/linux/iscsi_boot_sysfs.h
index 2a8b1659bf35..548d55395488 100644
--- a/include/linux/iscsi_boot_sysfs.h
+++ b/include/linux/iscsi_boot_sysfs.h
@@ -23,6 +23,7 @@ enum iscsi_boot_eth_properties_enum {
ISCSI_BOOT_ETH_INDEX,
ISCSI_BOOT_ETH_FLAGS,
ISCSI_BOOT_ETH_IP_ADDR,
+ ISCSI_BOOT_ETH_PREFIX_LEN,
ISCSI_BOOT_ETH_SUBNET_MASK,
ISCSI_BOOT_ETH_ORIGIN,
ISCSI_BOOT_ETH_GATEWAY,
--
1.8.5.2
--
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
[PATCHv2] Enable iBFT IPv6 booting with prefix length
It turns out that IPv6 doesn't care about netmask, but instead uses "prefix length" to determine the subnet, but the iBFT subsystem still just supports netmask. These two patches enable IPv6 iBFT (iscsi booting) by adding support for IPv6 prefix and prefix length. Open-iscsi changes to use these new values will be introduced once these changes are present. Hannes Reinecke (1): iscsi_ibft: Add prefix-len attr and display netmask drivers/firmware/iscsi_ibft.c| 4 drivers/scsi/iscsi_boot_sysfs.c | 5 + include/linux/iscsi_boot_sysfs.h | 1 + 3 files changed, 10 insertions(+) -- 1.8.5.2 -- 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
Re: [PATCH 2/2] iscsi_ibft: Always display netmask
On 02/01/2016 10:45 PM, Mike Christie wrote: > On 01/22/2016 01:49 PM, Lee Duncan wrote: >> From: Hannes Reinecke <h...@suse.de> >> >> Some older user-space code might rely on the netmask attribute >> being present, so we should always display it. >> This fixes a regression introduced by commit >> 0b2eb3c4060a16f3ec11a4d6d4c934e7e5d5334f. >> >> Signed-off-by: Hannes Reinecke <h...@suse.de> >> Signed-off-by: Lee Duncan <ldun...@suse.com> >> --- >> drivers/firmware/iscsi_ibft.c | 8 +--- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c >> index 2dd1fbb8..81037e5fe301 100644 >> --- a/drivers/firmware/iscsi_ibft.c >> +++ b/drivers/firmware/iscsi_ibft.c >> @@ -464,14 +464,8 @@ static umode_t ibft_check_nic_for(void *data, int type) >> rc = S_IRUGO; >> break; >> case ISCSI_BOOT_ETH_PREFIX_LEN: >> -if (nic->subnet_mask_prefix) >> -rc = S_IRUGO; >> -break; >> case ISCSI_BOOT_ETH_SUBNET_MASK: >> -if (!memcmp(nic->ip_addr, nulls, 10) && >> -(nic->ip_addr[10] == 0xff) && >> -(nic->ip_addr[11] == 0xff) && >> -nic->subnet_mask_prefix) >> +if (nic->subnet_mask_prefix) >> rc = S_IRUGO; >> break; >> case ISCSI_BOOT_ETH_ORIGIN: >> > > Sorry. I thought I sent this mail already. > > Is the commit id above supposed to be referencing the first patch? I > could not match it to anything. If so, then shouldn't this patch just be > combined with the second patch and some comment about us always > displaying it for compat reasons added to the code? > > Also, you should normally cc Konrad for iscsi_ibft.c patches, because he > is actually the maintainer. Hi Mike: I'm sorry I didn't reply sooner. I let this get buried in a side folder and missed it. The commit ID in Patch 2 was from the SUSE repository. The bottom line is that I think you are correct, these two patches could easily be combined. I will resubmit them as one combined patch. I submitted them as two because I was just feeding patching that Hannes had done upstream, but I should have noticed they could be combined. -- Lee Duncan SUSE Labs -- 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
[PATCH] Use ida_simple for SCSI iSCSI transport session id
The scsi_transport_iscsi module already uses the ida_simple
routines for managing the target ID, if requested to do
so. This change replaces an ever-increasing atomic integer
that tracks the session ID itself with the ida_simple
family of routines. This means that the session ID
will be reclaimed and can be reused when the session
is freed.
Note that no maximum is placed on this value, though
user-space currently only seems to use the lower 24-bits.
It seems better to handle this in user space, though,
than to limit the value range for the session ID here.
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/scsi/scsi_transport_iscsi.c | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c
b/drivers/scsi/scsi_transport_iscsi.c
index 441481623fb9..50a10bf214a8 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -79,7 +79,8 @@ struct iscsi_internal {
struct transport_container session_cont;
};
-static atomic_t iscsi_session_nr; /* sysfs session id for next new session */
+static DEFINE_IDA(iscsi_session_id_ida);
+
static struct workqueue_struct *iscsi_eh_timer_workq;
static DEFINE_IDA(iscsi_sess_ida);
@@ -2074,7 +2075,12 @@ int iscsi_add_session(struct iscsi_cls_session *session,
unsigned int target_id)
int err;
ihost = shost->shost_data;
- session->sid = atomic_add_return(1, _session_nr);
+ session->sid = ida_simple_get(_session_id_ida, 0, 0, GFP_KERNEL);
+ if (session->sid < 0) {
+ iscsi_cls_session_printk(KERN_ERR, session,
+"Failure in Session ID Allocation\n");
+ return session->sid;
+ }
if (target_id == ISCSI_MAX_TARGET) {
id = ida_simple_get(_sess_ida, 0, 0, GFP_KERNEL);
@@ -2082,7 +2088,8 @@ int iscsi_add_session(struct iscsi_cls_session *session,
unsigned int target_id)
if (id < 0) {
iscsi_cls_session_printk(KERN_ERR, session,
"Failure in Target ID Allocation\n");
- return id;
+ err = id;
+ goto release_session_id_ida;
}
session->target_id = (unsigned int)id;
session->ida_used = true;
@@ -2109,6 +2116,8 @@ int iscsi_add_session(struct iscsi_cls_session *session,
unsigned int target_id)
release_ida:
if (session->ida_used)
ida_simple_remove(_sess_ida, session->target_id);
+release_session_id_ida:
+ ida_simple_remove(_session_id_ida, session->target_id);
return err;
}
@@ -2214,6 +2223,7 @@ void iscsi_free_session(struct iscsi_cls_session *session)
{
ISCSI_DBG_TRANS_SESSION(session, "Freeing session\n");
iscsi_session_event(session, ISCSI_KEVENT_DESTROY_SESSION);
+ ida_simple_remove(_session_id_ida, session->target_id);
put_device(>dev);
}
EXPORT_SYMBOL_GPL(iscsi_free_session);
@@ -4524,8 +4534,6 @@ static __init int iscsi_transport_init(void)
printk(KERN_INFO "Loading iSCSI transport class v%s.\n",
ISCSI_TRANSPORT_VERSION);
- atomic_set(_session_nr, 0);
-
err = class_register(_transport_class);
if (err)
return err;
@@ -4598,6 +4606,7 @@ static void __exit iscsi_transport_exit(void)
class_unregister(_endpoint_class);
class_unregister(_iface_class);
class_unregister(_transport_class);
+ ida_destroy(_session_id_ida);
}
module_init(iscsi_transport_init);
--
2.1.4
--
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
[PATCH 2/2] iscsi_ibft: Always display netmask
From: Hannes Reinecke <h...@suse.de> Some older user-space code might rely on the netmask attribute being present, so we should always display it. This fixes a regression introduced by commit 0b2eb3c4060a16f3ec11a4d6d4c934e7e5d5334f. Signed-off-by: Hannes Reinecke <h...@suse.de> Signed-off-by: Lee Duncan <ldun...@suse.com> --- drivers/firmware/iscsi_ibft.c | 8 +--- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c index 2dd1fbb8..81037e5fe301 100644 --- a/drivers/firmware/iscsi_ibft.c +++ b/drivers/firmware/iscsi_ibft.c @@ -464,14 +464,8 @@ static umode_t ibft_check_nic_for(void *data, int type) rc = S_IRUGO; break; case ISCSI_BOOT_ETH_PREFIX_LEN: - if (nic->subnet_mask_prefix) - rc = S_IRUGO; - break; case ISCSI_BOOT_ETH_SUBNET_MASK: - if (!memcmp(nic->ip_addr, nulls, 10) && - (nic->ip_addr[10] == 0xff) && - (nic->ip_addr[11] == 0xff) && - nic->subnet_mask_prefix) + if (nic->subnet_mask_prefix) rc = S_IRUGO; break; case ISCSI_BOOT_ETH_ORIGIN: -- 1.8.5.2 -- 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
Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management
On 01/05/2016 03:53 PM, Martin K. Petersen wrote: "Lee" == Lee Duncan <ldun...@suse.com> writes: Lee> Do you need me to resubmit this patch now that it's accepted? Please resend. Thanks! Done, submitted against scsi tree, misc branch. -- Lee Duncan -- 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
[PATCHv2] SCSI: usd ida for host number management
Update the SCSI hosts module to use ida to manage its
host_no index instead of an ATOMIC integer. This means
that the SCSI host number will now be reclaimable.
Use the ida "simple" mechanism, since there should be no
need for a separate spin lock current usage. Ida was chosen
over idr because the hosts module already has its
own instance and locking mechanisms that aren't easily
changed.
Changes from v1:
* First version used regular ida routines
Reviewed-by: Hannes Reinecke <h...@suse.de>
Reviewed-by: Johannes Thumshirn <jthumsh...@suse.de>
Signed-off-by: Lee Duncan <ldun...@suse.com>
---
drivers/scsi/hosts.c | 22 ++
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 82ac1cd818ac..94025c5cf797 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -33,7 +33,7 @@
#include
#include
#include
-
+#include
#include
#include
#include
@@ -42,7 +42,7 @@
#include "scsi_logging.h"
-static atomic_t scsi_host_next_hn = ATOMIC_INIT(0);/* host_no for next new
host */
+static DEFINE_IDA(host_index_ida);
static void scsi_host_cls_release(struct device *dev)
@@ -355,6 +355,8 @@ static void scsi_host_dev_release(struct device *dev)
kfree(shost->shost_data);
+ ida_simple_remove(_index_ida, shost->host_no);
+
if (parent)
put_device(parent);
kfree(shost);
@@ -388,6 +390,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template
*sht, int privsize)
{
struct Scsi_Host *shost;
gfp_t gfp_mask = GFP_KERNEL;
+ int index;
if (sht->unchecked_isa_dma && privsize)
gfp_mask |= __GFP_DMA;
@@ -406,11 +409,11 @@ struct Scsi_Host *scsi_host_alloc(struct
scsi_host_template *sht, int privsize)
init_waitqueue_head(>host_wait);
mutex_init(>scan_mutex);
- /*
-* subtract one because we increment first then return, but we need to
-* know what the next host number was before increment
-*/
- shost->host_no = atomic_inc_return(_host_next_hn) - 1;
+ index = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
+ if (index < 0)
+ goto fail_kfree;
+ shost->host_no = index;
+
shost->dma_channel = 0xff;
/* These three are default values which can be overridden */
@@ -495,7 +498,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template
*sht, int privsize)
shost_printk(KERN_WARNING, shost,
"error handler thread failed to spawn, error = %ld\n",
PTR_ERR(shost->ehandler));
- goto fail_kfree;
+ goto fail_index_remove;
}
shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
@@ -511,6 +514,8 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template
*sht, int privsize)
fail_kthread:
kthread_stop(shost->ehandler);
+ fail_index_remove:
+ ida_simple_remove(_index_ida, shost->host_no);
fail_kfree:
kfree(shost);
return NULL;
@@ -606,6 +611,7 @@ int scsi_init_hosts(void)
void scsi_exit_hosts(void)
{
class_unregister(_class);
+ ida_destroy(_index_ida);
}
int scsi_is_host_device(const struct device *dev)
--
2.1.4
--
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
Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management
On 12/17/2015 11:24 AM, Lee Duncan wrote: > On 12/14/2015 05:55 PM, Martin K. Petersen wrote: >>>>>>> "Hannes" == Hannes Reinecke <h...@suse.de> writes: >> >>>> I'm not opposed to having the module option if others (Martin?) feel >>>> they need it, but generally I think it's better to keep things as >>>> simple as possible. So, unless there are strong objections, I would >>>> say no. >> >> Hannes> Agreeing with Ewan here. >> >> Hannes> I guess it's up to you to tell us whether you absolutely need a >> Hannes> module parameter ... >> >> Still not a big ida fan but since the most people seem to be in favor of >> this I guess I'll have to bite the bullet. >> >> I don't see much value in the module parameter since it will require >> customers to tweak their configs and reproduce. Not worth the hassle. >> > > Thank you Martin. I'll look at further cleaning up the host module, but > I think this still much better than leaving the code as is. > James: Do you need me to resubmit this patch now that it's accepted? -- Lee Duncan -- 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
Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management
On 12/14/2015 05:55 PM, Martin K. Petersen wrote: >>>>>> "Hannes" == Hannes Reinecke <h...@suse.de> writes: > >>> I'm not opposed to having the module option if others (Martin?) feel >>> they need it, but generally I think it's better to keep things as >>> simple as possible. So, unless there are strong objections, I would >>> say no. > > Hannes> Agreeing with Ewan here. > > Hannes> I guess it's up to you to tell us whether you absolutely need a > Hannes> module parameter ... > > Still not a big ida fan but since the most people seem to be in favor of > this I guess I'll have to bite the bullet. > > I don't see much value in the module parameter since it will require > customers to tweak their configs and reproduce. Not worth the hassle. > Thank you Martin. I'll look at further cleaning up the host module, but I think this still much better than leaving the code as is. -- Lee Duncan -- 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
