Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

Sagi, do we still do IN, OUT, both or none? if not, where this stopped
to be supported? how large would be the fix?


Or, it's hard for me to say where exactly it stopped being supported
because I've never tested it and I'm not convinced it was ever
supported.

I'm exhausted with this discussion so I'll change the tiny condition
and bidi will remain unsupported until we'll decide we want to get it
working.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Christoph Hellwig]

On Thu, Nov 19, 2015 at 09:12:20AM +0200, Or Gerlitz wrote:
> This is wrong assertion.
> 
> Look on the code throughout the iser path done from iser_send_command, we
> allowed the command associated with the
> iscsi task to be IN, OUT, both or none, when we do all the dma-mapping,
> memory registrations and such for either of the
> needed directions and same on the completion path.

Internal code structure is one thing, ever supporting bidi is another
one.  Without QUEUE_FLAG_BIDI a driver has never supported bidirectional
requests.  And iSER never had that flag set in mainline.  So even if you
spent of time supporting bidi in iSER it never was supported and all
that great support was entirely untested the last 10 years.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On Thu, Nov 19, 2015 at 12:01 PM, Christoph Hellwig  wrote:

> Internal code structure is one thing, ever supporting bidi is another
> one.  Without QUEUE_FLAG_BIDI a driver has never supported bidirectional
> requests.  And iSER never had that flag set in mainline.  So even if you
> spent of time supporting bidi in iSER it never was supported and all
> that great support was entirely untested the last 10 years.

Christoph,

To make it clear iser's role is very well defined and strict w.r.t
supporting bidis, we should

on IO submission path

1. dma map the SC properly in, out, both or none
2. memory register what's needed
3. put in the iser header 1,2 or 0 keys

on IO completion path

4. set a pointer to the iscsi response header etc
5. memory unregister what's needed
6. dma unmap properly
7. call up to the iscsi/scsi stack with that pointer

anything else that I missed?

AFAIK steps 1..7 were supported ok in the past, probably now not, we
should fix at some point



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

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

AFAIR, in the past there weren't explicit means to do that.

What's makes iscsi tcp eligible to support bidi's that we don't have @ iser?


In theory, nothing.
In practice, iser is missing customer demands, iser targets that
support bidi and testing...

If someone cared enough about it then I assume we'd hear about it by
now...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Christoph Hellwig]

On Wed, Nov 18, 2015 at 03:33:01PM +0200, Or Gerlitz wrote:
> Sagi, it works in TGT and AFAIR with the initiator too.
> 
> Looking on this paper of Pete Wyckoff  [1] I see that he says that
> few changes to the initiator were needed, not sure which.

Or, can you please stop it?

Fortunately neither the iSER target or initiator ever support the
nightmare called bidi commands, and I'be happy o kill of that cruft
entirely if we could.

Did you buy shares in a defunct T10 OSD vendor or why do you even care?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Christoph Hellwig]

On Wed, Nov 18, 2015 at 03:58:48PM +0200, Or Gerlitz wrote:
> > Fortunately neither the iSER target or initiator ever support the
> > nightmare called bidi commands,
> 
> I honestly don't know why you call it nightmare nor what make you
> make that strong assertion. 

Beause I actually had to deal with block layer code implementing the
bidi semantics.

> I checked with Alex N. the TGT iser
> transport author and he confirmed to me that bidi's worked on TGT,
> as for the Linux upstream initiator, I thought that was the case too, but
> I never saw it my eyes, and Pete's paper states he had to change the code,
> so on that I can't be sure.

No need to trust words, we have git.  There was absolutely nothing
relating to bidi in either the initial iSER checking nor in the
changelogs since, and neither has the initial BIDI checking touched
iSER.

> that's beyond the scope of this thread, I guess..

As is iSER BIDI suppport.  Let's thank Sagi for doing a very important
performance feature for iSER instead of having this endless discussion
about a pointless fringe feature!
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

Sagi, it works in TGT and AFAIR with the initiator too.

Looking on this paper of Pete Wyckoff  [1] I see that he says that
few changes to the initiator were needed, not sure which.


I see. I wasn't aware that TGT supports bidi. However, AFAICT the
initiator support was never fully introduced upstream nor in our mlnx
backports (perhaps in an off-tree implementation). As I see it, bidi
functionality, is broken for a long time (if it was ever supported).

So I'd suggest we (you and me Or) look at bidi separately and try to
find out if someone wants it (not for academic research).

Would you mind if we don't include bidi considerations in this patchset?

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

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On Wed, Nov 18, 2015 at 1:38 PM, Sagi Grimberg  wrote:
>> AFAIR, in the past there weren't explicit means to do that.
>> What's makes iscsi tcp eligible to support bidi's that we don't have @ iser?

> In theory, nothing. In practice, iser is missing customer demands, iser 
> targets that
> support bidi and testing...

> If someone cared enough about it then I assume we'd hear about it by now...

Sagi, it works in TGT and AFAIR with the initiator too.

Looking on this paper of Pete Wyckoff  [1] I see that he says that
few changes to the initiator were needed, not sure which.

Or.


[1] http://pw.padd.com/papers/dalessandro-iser-snapi07.pdf
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On Tue, Nov 17, 2015 at 12:26 PM, Sagi Grimberg
 wrote:
>
>> Why? we don't support and when did we broke it after the initial 2.6.18
>> upstreaming of the driver
>>
>> Also, do we refuse to queuecommand a bidi? where?
>
>
> Or, bidirectional support is not assumed and needs to be actively set
> as a request queue flag (see iscsi_sw_tcp_slave_alloc()).

> AFAICT iser never exposed support for bidirectional commands.

AFAIR, in the past there weren't explicit means to do that.

What's makes iscsi tcp eligible to support bidi's that we don't have @ iser?

> Did things change from back in 2.6.18 that we ever carried bidi
> commands over iser?

AFAIK, Pete Wyckoff had iser working with BIDI commands for few object
storage projects.

It's been a long time, but I was totally unaware that we don't publish
the whatever means
needed by upper layers.

I copied some 2010 email address of his here...

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

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:

--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -847,7 +847,7 @@ static void iser_route_handler(struct rdma_cm_id *cma_id)
conn_param.rnr_retry_count = 6;
  
  	memset(_hdr, 0, sizeof(req_hdr));

-   req_hdr.flags = (ISER_ZBVA_NOT_SUP | ISER_SEND_W_INV_NOT_SUP);
+   req_hdr.flags = ISER_ZBVA_NOT_SUP;


isn't there a property of the **local** device we need to check before 
advertizing that
to the target? to be on the safe side, I would do  that only over 
devices that support
IB_DEVICE_MEM_MGT_EXTENSIONS, as non-local invalidations are part of the 
BMME ext

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

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Christoph Hellwig]

On Tue, Nov 17, 2015 at 12:04:03PM +0200, Or Gerlitz wrote:
> Also, do we refuse to queuecommand a bidi? where?

Or, can you please do the basic research first?  Thanks! (Hint: check
how few drivers support bidi commands, and how it's enabled)
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

On 17/11/2015 10:05, Or Gerlitz wrote:

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:

+if (iser_task->dir[ISER_DIR_IN])
+reg = _task->rdma_reg[ISER_DIR_IN];
+else
+reg = _task->rdma_reg[ISER_DIR_OUT];


and what happens with bidirectional commands?!


It won't work, iSER lacks support for bidirectional commands for as
long as I'm involved with it and until we either decide that we care
enough to implement it both in the target and initiator sides or some
array with iser bidi support shows up I don't know how much its worth
our attention.

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

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

Why? we don't support and when did we broke it after the initial 2.6.18
upstreaming of the driver

Also, do we refuse to queuecommand a bidi? where?


Or, bidirectional support is not assumed and needs to be actively set
as a request queue flag (see iscsi_sw_tcp_slave_alloc()). AFAICT iser
never exposed support for bidirectional commands.

Did things change from back in 2.6.18 that we ever carried bidi
commands over iser?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

On 17/11/2015 10:03, Or Gerlitz wrote:

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:

  void iser_rcv_completion(struct iser_rx_desc *rx_desc,
- unsigned long rx_xfer_len,
+ struct ib_wc *wc,
   struct ib_conn *ib_conn)
  {
  struct iser_conn *iser_conn = container_of(ib_conn, struct
iser_conn,
@@ -575,6 +621,7 @@ void iser_rcv_completion(struct iser_rx_desc
*rx_desc,
  struct iscsi_hdr *hdr;
  u64 rx_dma;
  int rx_buflen, outstanding, count, err;
+unsigned long rx_xfer_len = wc->byte_len;


unrelated small enhancement which has nothing to do with this patch,
remove it from here


It is related, we pass wc to check the invalidate stuff down the road...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

On 17/11/2015 10:08, Or Gerlitz wrote:

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:

--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -847,7 +847,7 @@ static void iser_route_handler(struct rdma_cm_id
*cma_id)
  conn_param.rnr_retry_count = 6;
  memset(_hdr, 0, sizeof(req_hdr));
-req_hdr.flags = (ISER_ZBVA_NOT_SUP | ISER_SEND_W_INV_NOT_SUP);
+req_hdr.flags = ISER_ZBVA_NOT_SUP;


isn't there a property of the **local** device we need to check before
advertizing that
to the target? to be on the safe side, I would do  that only over
devices that support
IB_DEVICE_MEM_MGT_EXTENSIONS, as non-local invalidations are part of the
BMME ext
of IBTA, right?


This was dependent on using fastreg (which depends on
IB_DEVICE_MEM_MGT_EXTENSIONS) at some point but it must have got lost
at some point..

Will fix. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On 11/17/2015 11:53 AM, Sagi Grimberg wrote:
[...] iSER lacks support for bidirectional commands for as long as I'm 
involved with it 



Why? we don't support and when did we broke it after the initial 2.6.18 
upstreaming of the driver


Also, do we refuse to queuecommand a bidi? where?



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

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:

+   if (iser_task->dir[ISER_DIR_IN])
+   reg = _task->rdma_reg[ISER_DIR_IN];
+   else
+   reg = _task->rdma_reg[ISER_DIR_OUT];


and what happens with bidirectional commands?!
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:

  void iser_rcv_completion(struct iser_rx_desc *rx_desc,
-unsigned long rx_xfer_len,
+struct ib_wc *wc,
 struct ib_conn *ib_conn)
  {
struct iser_conn *iser_conn = container_of(ib_conn, struct iser_conn,
@@ -575,6 +621,7 @@ void iser_rcv_completion(struct iser_rx_desc *rx_desc,
struct iscsi_hdr *hdr;
u64 rx_dma;
int rx_buflen, outstanding, count, err;
+   unsigned long rx_xfer_len = wc->byte_len;


unrelated small enhancement which has nothing to do with this patch, 
remove it from here

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

[PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

From: Jenny Derzhavetz 

Declare that we support remote invalidation and be able to detect
the invalidated rkey so we won't invalidate it locally. The spec
mandates that we must not rely on the taget remote invalidate our
rkey so we must check it upon a receive (scsi response) completion.

Signed-off-by: Jenny Derzhavetz 
Signed-off-by: Sagi Grimberg 
---
 drivers/infiniband/ulp/iser/iscsi_iser.h |  3 +-
 drivers/infiniband/ulp/iser/iser_initiator.c | 55 +++-
 drivers/infiniband/ulp/iser/iser_verbs.c | 19 +++---
 3 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h 
b/drivers/infiniband/ulp/iser/iscsi_iser.h
index 096d5234bbea..2e0a24ba18ed 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -519,6 +519,7 @@ struct iser_conn {
u32  num_rx_descs;
unsigned short   scsi_sg_tablesize;
unsigned int scsi_max_sectors;
+   bool snd_w_inv;
 };
 
 /**
@@ -603,7 +604,7 @@ int iser_conn_terminate(struct iser_conn *iser_conn);
 void iser_release_work(struct work_struct *work);
 
 void iser_rcv_completion(struct iser_rx_desc *desc,
-unsigned long dto_xfer_len,
+struct ib_wc *wc,
 struct ib_conn *ib_conn);
 
 void iser_snd_completion(struct iser_tx_desc *desc,
diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c 
b/drivers/infiniband/ulp/iser/iser_initiator.c
index 6a968e350c14..df6a4b70ffc9 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -563,11 +563,57 @@ send_control_error:
return err;
 }
 
+static int
+iser_check_remote_inv(struct iser_conn *iser_conn,
+ struct ib_wc *wc,
+ struct iscsi_hdr *hdr)
+{
+   if (wc->wc_flags & IB_WC_WITH_INVALIDATE) {
+   struct iscsi_task *task;
+   struct iscsi_iser_task *iser_task;
+   u32 rkey = wc->ex.invalidate_rkey;
+
+   iser_dbg("conn %p: remote invalidation\n", iser_conn);
+   if (unlikely(!iser_conn->snd_w_inv)) {
+   iser_err("conn %p: unexepected remote invalidation,"
+"terminating connection\n", iser_conn);
+   return -EPROTO;
+   }
+
+   task = iscsi_itt_to_ctask(iser_conn->iscsi_conn, hdr->itt);
+   if (likely(task)) {
+   struct iser_mem_reg *reg;
+   struct iser_fr_desc *desc;
+
+   iser_task = task->dd_data;
+   if (iser_task->dir[ISER_DIR_IN])
+   reg = _task->rdma_reg[ISER_DIR_IN];
+   else
+   reg = _task->rdma_reg[ISER_DIR_OUT];
+   desc = reg->mem_h;
+
+   if (likely(rkey == desc->rsc.mr->rkey)) {
+   desc->rsc.mr_valid = 0;
+   } else if (likely(rkey == desc->pi_ctx->sig_mr->rkey)) {
+   desc->pi_ctx->sig_mr_valid = 0;
+   } else {
+   iser_err("invalidation of unknown rkey=0x%x\n", 
rkey);
+   return -EINVAL;
+   }
+   } else {
+   iser_err("failed to get task for itt=%d\n", hdr->itt);
+   return -EINVAL;
+   }
+   }
+
+   return 0;
+}
+
 /**
  * iser_rcv_dto_completion - recv DTO completion
  */
 void iser_rcv_completion(struct iser_rx_desc *rx_desc,
-unsigned long rx_xfer_len,
+struct ib_wc *wc,
 struct ib_conn *ib_conn)
 {
struct iser_conn *iser_conn = container_of(ib_conn, struct iser_conn,
@@ -575,6 +621,7 @@ void iser_rcv_completion(struct iser_rx_desc *rx_desc,
struct iscsi_hdr *hdr;
u64 rx_dma;
int rx_buflen, outstanding, count, err;
+   unsigned long rx_xfer_len = wc->byte_len;
 
/* differentiate between login to all other PDUs */
if ((char *)rx_desc == iser_conn->login_resp_buf) {
@@ -593,6 +640,12 @@ void iser_rcv_completion(struct iser_rx_desc *rx_desc,
iser_dbg("op 0x%x itt 0x%x dlen %d\n", hdr->opcode,
hdr->itt, (int)(rx_xfer_len - ISER_HEADERS_LEN));
 
+   if (iser_check_remote_inv(iser_conn, wc, hdr)) {
+   iscsi_conn_failure(iser_conn->iscsi_conn,
+  ISCSI_ERR_CONN_FAILED);
+   return;
+   }
+
iscsi_iser_recv(iser_conn->iscsi_conn, hdr, rx_desc->data,
rx_xfer_len - ISER_HEADERS_LEN);
 
diff --git