Re: [PATCH v3 4/5] scsi: rename SCSI_MAX_{SG, SG_CHAIN}_SEGMENTS

[Ming Lin]

On Tue, Apr 5, 2016 at 7:55 AM, Tejun Heo  wrote:
> On Mon, Apr 04, 2016 at 02:48:10PM -0700, Ming Lin wrote:
>> From: Ming Lin 
>>
>> Rename SCSI_MAX_SG_SEGMENTS to SG_CHUNK_SIZE, which means the amount
>> we fit into a single scatterlist chunk.
>>
>> Rename SCSI_MAX_SG_CHAIN_SEGMENTS to SG_MAX_SEGMENTS.
>>
>> Will move these 2 generic definitions to scatterlist.h later.
>>
>> Reviewed-by: Christoph Hellwig 
>> Acked-by: Bart Van Assche  (for ib_srp changes)
>> Signed-off-by: Ming Lin 
>
> For libata,
>
> Acked-by: Tejun Heo 

Hi James,

Are we ready to merge it?

Thanks,
Ming
--
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] [LSF/MM TOPIC] block-mq issues with FC

[Sagi Grimberg]

Hey Willy,


  - Interrupt steering needs to be controlled by block-mq instead of
the driver.  It's pointless to have each driver implement its own
policies on interrupt steering, irqbalanced remains a source of
end-user frustration, and block-mq can change the queue<->cpu mapping
without the driver's knowledge.


I honestly don't think that block-mq is the right place to
*assign* interrupt steering. Not all HW devices are dedicated
to storage, take RDMA for example, a RNIC is shared by block
storage, networking and even user-space workloads so obviously
block-mq can't understand how a user wants to steer interrupts.

I think that block-mq needs to ask the device driver:
"what is the optimal queue index for cpu X?" and use it
while *someone* will be responsible for optimum interrupt
steering (can be the driver itself or user-space).

From some discussions I had with HCH I think he intends to
use the cpu reverse-mapping API to try and do what's described
above (if I'm not mistaken).
--
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 15/16] iscsi-target: fix seq_end_offset calculation

[Sagi Grimberg]

Fixes should go in the front of the series (probably even better
detached from the set), and this also looks like stable material...
--
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 11/16] iscsi-target: add new offload transport type

[Sagi Grimberg]

+static ssize_t lio_target_np_hw_offload_show(struct config_item *item,
+char *page)
+{
+   struct iscsi_tpg_np *tpg_np = to_iscsi_tpg_np(item);
+   struct iscsi_tpg_np *tpg_np_hw_offload;
+   ssize_t rb;
+
+   tpg_np_hw_offload = iscsit_tpg_locate_child_np(tpg_np,
+  ISCSI_HW_OFFLOAD);
+   if (tpg_np_hw_offload)
+   rb = sprintf(page, "1\n");
+   else
+   rb = sprintf(page, "0\n");
+
+   return rb;
+}
+
+static ssize_t lio_target_np_hw_offload_store(struct config_item *item,
+ const char *page, size_t count)
+{
+   struct iscsi_tpg_np *tpg_np = to_iscsi_tpg_np(item);
+   struct iscsi_np *np;
+   struct iscsi_portal_group *tpg;
+   struct iscsi_tpg_np *tpg_np_hw_offload = NULL;
+   u32 op;
+   int rc = 0;
+
+   rc = kstrtou32(page, 0, );
+   if (rc)
+   return rc;
+
+   if ((op != 1) && (op != 0)) {
+   pr_err("Illegal value for tpg_enable: %u\n", op);
+   return -EINVAL;
+   }
+
+   np = tpg_np->tpg_np;
+   if (!np) {
+   pr_err("Unable to locate struct iscsi_np from"
+  " struct iscsi_tpg_np\n");
+   return -EINVAL;
+   }
+
+   tpg = tpg_np->tpg;
+   if (iscsit_get_tpg(tpg) < 0)
+   return -EINVAL;
+
+   if (op) {
+   tpg_np_hw_offload = iscsit_tpg_add_network_portal(tpg,
+   >np_sockaddr, tpg_np, ISCSI_HW_OFFLOAD);
+
+   if (IS_ERR(tpg_np_hw_offload)) {
+   rc = PTR_ERR(tpg_np_hw_offload);
+   goto out;
+   }
+   } else {
+   tpg_np_hw_offload = iscsit_tpg_locate_child_np(tpg_np,
+   ISCSI_HW_OFFLOAD);
+
+   if (tpg_np_hw_offload) {
+   rc = iscsit_tpg_del_network_portal(tpg,
+  tpg_np_hw_offload);
+   if (rc < 0)
+   goto out;
+   }
+   }
+
+   iscsit_put_tpg(tpg);
+   return count;
+out:
+   iscsit_put_tpg(tpg);
+   return rc;
+}
+
  CONFIGFS_ATTR(lio_target_np_, sctp);
  CONFIGFS_ATTR(lio_target_np_, iser);
+CONFIGFS_ATTR(lio_target_np_, hw_offload);


I'd be happy to see new transports being added with some
initiative to reduce the code duplication here (pretty please :))
--
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 10/16] iscsi-target: use conn->network_transport in text rsp

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg 
--
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 08/16] iscsi-target: add void (*iscsit_get_r2t_ttt)()

[Sagi Grimberg]

Add void (*iscsit_get_r2t_ttt)() to
struct iscsit_transport, iscsi-target
uses this callback to get
r2t->targ_xfer_tag.


Your driver allocates ttt's? That looks like bad
layering to me. This definitely deserves an explanation...
--
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 07/16] iscsi-target: add int (*iscsit_validate_params)()

[Sagi Grimberg]

Add int (*iscsit_validate_params)() to
struct iscsit_transport, iscsi-target
uses this callback for validating
conn operational parameters.


Again, why is this needed?
--
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 05/16] iscsi-target: add void (*iscsit_get_rx_pdu)()

[Sagi Grimberg]

Add void (*iscsit_get_rx_pdu)() to
struct iscsit_transport, iscsi-target
uses this callback to receive and
process Rx iSCSI PDUs.


Same comment on change logs.

The iser bit looks harmless

Acked-by: Sagi Grimberg 

Though I agree with hch that we don't really need
rx threads in iSER, but that's another story...
--
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 04/16] iscsi-target: add void (*iscsit_release_cmd)()

[Sagi Grimberg]

Add void (*iscsit_release_cmd)() to
struct iscsit_transport, iscsi-target
uses this callback to release transport
driver resources associated with an iSCSI cmd.


I'd really like to see some reasoning on why you add
abstraction callouts. It may have a valid reason but
it needs to be documented in the change log...


diff --git a/drivers/target/iscsi/iscsi_target_util.c 
b/drivers/target/iscsi/iscsi_target_util.c
index 428b0d9..a533017 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -725,6 +725,9 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool scsi_cmd,
iscsit_remove_cmd_from_immediate_queue(cmd, conn);
iscsit_remove_cmd_from_response_queue(cmd, conn);
}
+
+   if (conn && conn->conn_transport->iscsit_release_cmd)
+   conn->conn_transport->iscsit_release_cmd(conn, cmd);
  }


Did you verify that you get here with conn = NULL (given that you test
it)? If so, then can you please document why is it expected for this
function to be called twice that we need to make it safe?

If not, then I'd move this check to be a WARN_ON/BUG_ON to hunt
down when is this happening.
--
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 03/16] iscsi-target: add int (*iscsit_xmit_datain_pdu)()

[Sagi Grimberg]

On 09/04/16 16:11, Varun Prakash wrote:

Add int (*iscsit_xmit_datain_pdu)() to
struct iscsit_transport, iscsi-target
uses this callback to transmit a DATAIN
iSCSI PDU.

Signed-off-by: Varun Prakash 
---
  drivers/target/iscsi/iscsi_target.c| 143 +++--
  include/target/iscsi/iscsi_transport.h |   3 +
  2 files changed, 86 insertions(+), 60 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 0e7a481..9e65e5d 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -577,6 +577,84 @@ static int iscsit_xmit_pdu(struct iscsi_conn *conn, struct 
iscsi_cmd *cmd,
return 0;
  }

+static int iscsit_map_iovec(struct iscsi_cmd *, struct kvec *, u32, u32);
+static void iscsit_unmap_iovec(struct iscsi_cmd *);
+static u32 iscsit_do_crypto_hash_sg(struct ahash_request *, struct iscsi_cmd *,
+   u32, u32, u32, u8 *);
+static int
+iscsit_xmit_datain_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
+  struct iscsi_datain_req *dr, struct iscsi_datain *datain)


Looks very similar to xmit_pdu(), if we add a datain pointer that
can be null for normal pdus would that reduce code duplication?
--
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 02/16] iscsi-target: add int (*iscsit_xmit_pdu)()

[Sagi Grimberg]

Nice!

Reviewed-by: Sagi Grimberg 
--
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 01/16] iscsi-target: add callback to alloc and free PDU

[Sagi Grimberg]

On 09/04/16 16:11, Varun Prakash wrote:

Add two callbacks to struct iscsit_transport -

1. void *(*iscsit_alloc_pdu)()
iscsi-target uses this callback for
iSCSI PDU allocation.

2. void (*iscsit_free_pdu)
iscsi-target uses this callback
to free an iSCSI PDU which was
allocated by iscsit_alloc_pdu().


Can you please explain why are you adding two different
callouts? Who (Chelsio T5) will need it, and why they can't
use the in-cmd pdu?



Signed-off-by: Varun Prakash 
---
  drivers/target/iscsi/iscsi_target.c| 76 --
  include/target/iscsi/iscsi_transport.h |  2 +
  2 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 961202f..fdb33ba 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -499,6 +499,11 @@ static void iscsit_aborted_task(struct iscsi_conn *conn, 
struct iscsi_cmd *cmd)
__iscsit_free_cmd(cmd, scsi_cmd, true);
  }

+static void *iscsit_alloc_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
+{
+   return cmd->pdu;
+}
+
  static enum target_prot_op iscsit_get_sup_prot_ops(struct iscsi_conn *conn)
  {
return TARGET_PROT_NORMAL;
@@ -519,6 +524,7 @@ static struct iscsit_transport iscsi_target_transport = {
.iscsit_queue_data_in   = iscsit_queue_rsp,
.iscsit_queue_status= iscsit_queue_rsp,
.iscsit_aborted_task= iscsit_aborted_task,
+   .iscsit_alloc_pdu   = iscsit_alloc_pdu,
.iscsit_get_sup_prot_ops = iscsit_get_sup_prot_ops,
  };

@@ -2537,7 +2543,10 @@ static int iscsit_send_conn_drop_async_message(
cmd->tx_size = ISCSI_HDR_LEN;
cmd->iscsi_opcode = ISCSI_OP_ASYNC_EVENT;

-   hdr = (struct iscsi_async *) cmd->pdu;
+   hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+   if (unlikely(!hdr))
+   return -ENOMEM;
+
hdr->opcode  = ISCSI_OP_ASYNC_EVENT;
hdr->flags   = ISCSI_FLAG_CMD_FINAL;
cmd->init_task_tag   = RESERVED_ITT;
@@ -2630,7 +2639,7 @@ iscsit_build_datain_pdu(struct iscsi_cmd *cmd, struct 
iscsi_conn *conn,

  static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
  {
-   struct iscsi_data_rsp *hdr = (struct iscsi_data_rsp *)>pdu[0];
+   struct iscsi_data_rsp *hdr;
struct iscsi_datain datain;
struct iscsi_datain_req *dr;
struct kvec *iov;
@@ -2675,6 +2684,10 @@ static int iscsit_send_datain(struct iscsi_cmd *cmd, 
struct iscsi_conn *conn)
set_statsn = true;
}

+   hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+   if (unlikely(!hdr))
+   return -ENOMEM;
+
iscsit_build_datain_pdu(cmd, conn, , hdr, set_statsn);

iov = >iov_data[0];
@@ -2843,13 +2856,20 @@ EXPORT_SYMBOL(iscsit_build_logout_rsp);
  static int
  iscsit_send_logout(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
  {
+   struct iscsi_logout_rsp *hdr;
struct kvec *iov;
int niov = 0, tx_size, rc;

-   rc = iscsit_build_logout_rsp(cmd, conn,
-   (struct iscsi_logout_rsp *)>pdu[0]);
-   if (rc < 0)
+   hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+   if (unlikely(!hdr))
+   return -ENOMEM;
+
+   rc = iscsit_build_logout_rsp(cmd, conn, hdr);
+   if (rc < 0) {
+   if (conn->conn_transport->iscsit_free_pdu)
+   conn->conn_transport->iscsit_free_pdu(conn, cmd);


This creates a weird asymmetry where alloc is called unconditionally
while free is conditional, I'd say implement an empty free for iscsit.
Same for the rest of the code...

P.S. I didn't see any non-error path call to free_pdu, is that a
possible leak (for drivers that actually allocate a PDU)?

On another unrelated note, I'd be very happy if we lose the iscsit_
prefix from all the callouts, it clear to everyone that it iscsi, no
need for an explicit reminder...
--
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: [RFC 14/34] iscsi-target: export symbols

[Sagi Grimberg]

Great.  Just curious how -v2 is coming along..?

I've got a few cycles over the weekend, and plan to start reviewing as
the series hits the list.

Btw, I asked Sagi to help out with review as well.


If you did, it's lost in my old email address :)

I'll have a look this week.

Cheers,
Sagi.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html