[PATCH] nvmet-rdma: Don't use the inline buffer in order to avoid allocation for small reads

[PATCH] nvmet-rdma: Don't use the inline buffer in order to avoid allocation for small reads

[Sagi Grimberg]

Under extreme conditions this might cause data corruptions. By doing that
we we repost the buffer and then post this buffer for the device to send.
If we happen to use shared receive queues the device might write to the
buffer before it sends it (there is no ordering between send and recv
queues). Without SRQs we probably won't get that if the host doesn't
mis-behave and send more than we allowed it, but relying on that is not
really a good idea.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/rdma.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index e06d504bdf0c..4e83d92d6bdd 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -615,15 +615,10 @@ static u16 nvmet_rdma_map_sgl_keyed(struct nvmet_rdma_rsp *rsp,
 	if (!len)
 		return 0;
 
-	/* use the already allocated data buffer if possible */
-	if (len <= NVMET_RDMA_INLINE_DATA_SIZE && rsp->queue->host_qid) {
-		nvmet_rdma_use_inline_sg(rsp, len, 0);
-	} else {
-		status = nvmet_rdma_alloc_sgl(&rsp->req.sg, &rsp->req.sg_cnt,
-				len);
-		if (status)
-			return status;
-	}
+	status = nvmet_rdma_alloc_sgl(&rsp->req.sg, &rsp->req.sg_cnt,
+			len);
+	if (status)
+		return status;
 
 	ret = rdma_rw_ctx_init(&rsp->rw, cm_id->qp, cm_id->port_num,
 			rsp->req.sg, rsp->req.sg_cnt, 0, addr, key,
-- 
1.9.1

[PATCH] nvmet-rdma: Don't use the inline buffer in order to avoid allocation for small reads

[Christoph Hellwig]

On Mon, Aug 01, 2016@11:36:39AM +0300, Sagi Grimberg wrote:
> Under extreme conditions this might cause data corruptions. By doing that
> we we repost the buffer and then post this buffer for the device to send.
> If we happen to use shared receive queues the device might write to the
> buffer before it sends it (there is no ordering between send and recv
> queues). Without SRQs we probably won't get that if the host doesn't
> mis-behave and send more than we allowed it, but relying on that is not
> really a good idea.

Pitty - it seems so wasteful not being able to use these buffers for
anything that isn't an inline write.  I fully agree on the SRQ case,
but I think we should offer it for the non-SRP case.

[PATCH] nvmet-rdma: Don't use the inline buffer in order to avoid allocation for small reads

[Sagi Grimberg]

>> Under extreme conditions this might cause data corruptions. By doing that
>> we we repost the buffer and then post this buffer for the device to send.
>> If we happen to use shared receive queues the device might write to the
>> buffer before it sends it (there is no ordering between send and recv
>> queues). Without SRQs we probably won't get that if the host doesn't
>> mis-behave and send more than we allowed it, but relying on that is not
>> really a good idea.
>
> Pitty - it seems so wasteful not being able to use these buffers for
> anything that isn't an inline write.

Totally agree, I'm open to smart ideas on this...

> I fully agree on the SRQ case, but I think we should offer it for the non-SRP case.

As I wrote, even in the non-srq case, if the host is sending a single
write over the negotiated queue size, the data can land in the buffer
that is currently being sent (its a rare race condition, but
theoretically possible). The reason is that we repost the inline data
buffer for receive before we post the send request. We used to have
it the other way around (which eliminates the issue) but we then saw
some latency bubbles due to the HW sending rnr-naks to the host in
the lack of a receive buffer (in iWARP the problem was even worse
because there is no flow-control).

Do you think it's OK to risk data corruption if the host is
misbehaving?

[PATCH] nvmet-rdma: Don't use the inline buffer in order to avoid allocation for small reads

[Jason Gunthorpe]

On Tue, Aug 02, 2016@04:38:58PM +0300, Sagi Grimberg wrote:
> that is currently being sent (its a rare race condition, but
> theoretically possible). The reason is that we repost the inline data
> buffer for receive before we post the send request. We used to have

?? The same buffer is posted at the same time for send and recv? That
is never OK, SRQ or not.

Jason

[PATCH] nvmet-rdma: Don't use the inline buffer in order to avoid allocation for small reads

[Sagi Grimberg]

>> that is currently being sent (its a rare race condition, but
>> theoretically possible). The reason is that we repost the inline data
>> buffer for receive before we post the send request. We used to have
>
> ?? The same buffer is posted at the same time for send and recv? That
> is never OK, SRQ or not.

I agree.

But I agree its a shame to lose this. Maybe if we over-allocate cmds
and restore the receive repost to be after the send completion? I'm not
too fond of that either (it's not just commands but also inline
pages...)

[PATCH] nvmet-rdma: Don't use the inline buffer in order to avoid allocation for small reads

[Christoph Hellwig]

On Tue, Aug 02, 2016@10:15:26AM -0600, Jason Gunthorpe wrote:
> On Tue, Aug 02, 2016@04:38:58PM +0300, Sagi Grimberg wrote:
> > that is currently being sent (its a rare race condition, but
> > theoretically possible). The reason is that we repost the inline data
> > buffer for receive before we post the send request. We used to have
> 
> ?? The same buffer is posted at the same time for send and recv? That
> is never OK, SRQ or not.

We will never POST it for a SEND, but it would be used as the target
of RDMA READ / WRITE operations.

[PATCH] nvmet-rdma: Don't use the inline buffer in order to avoid allocation for small reads

[Sagi Grimberg]

>>> that is currently being sent (its a rare race condition, but
>>> theoretically possible). The reason is that we repost the inline data
>>> buffer for receive before we post the send request. We used to have
>>
>> ?? The same buffer is posted at the same time for send and recv? That
>> is never OK, SRQ or not.
>
> We will never POST it for a SEND, but it would be used as the target
> of RDMA READ / WRITE operations.

Jason's comment still holds.

[PATCH] nvmet-rdma: Don't use the inline buffer in order to avoid allocation for small reads

[Christoph Hellwig]

On Mon, Aug 01, 2016@11:36:39AM +0300, Sagi Grimberg wrote:
> Under extreme conditions this might cause data corruptions. By doing that
> we we repost the buffer and then post this buffer for the device to send.
> If we happen to use shared receive queues the device might write to the
> buffer before it sends it (there is no ordering between send and recv
> queues). Without SRQs we probably won't get that if the host doesn't
> mis-behave and send more than we allowed it, but relying on that is not
> really a good idea.

Allright, after the discussion:

Reviewed-by: Christoph Hellwig <hch at lst.de>