Skip to contentSkip to conversation

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

PATCH New Sagi Grimberg 3 participants 8 messages Aug 1, 2016 – Aug 4, 2016

Subsystems (3)
InfiniBand NVMe NVMe target
lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

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

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(-)
drivers/nvme/target/rdma.c +4 -9
index e06d504bdf0c..4e83d92d6bdd 100644
@@ -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,
Signature · 1 line
1.9.1
maintainer
#1 lore raw

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

Quoted 7 lines · 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. I fully agree on the SRQ case, but I think we should offer it for the non-SRP case.

author maintainer
#2 lore raw

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

Quoted 11 lines
Quoted 8 lines

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...

Quoted 1 line

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?

#3 lore raw

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

Quoted 3 lines · 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.

Jason

author maintainer
#4 lore raw

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

Quoted 7 lines
Quoted 4 lines

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...)

maintainer
#5 lore raw

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

In reply to Jason Gunthorpe
Quoted 7 lines · Jason Gunthorpe
Quoted 3 lines · 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.

author maintainer
#6 lore raw

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

Quoted 10 lines
Quoted 7 lines
Quoted 4 lines

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.

maintainer
#7 lore raw

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

In reply to Sagi Grimberg
Quoted 7 lines · 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.

Allright, after the discussion:

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