On Mon, Nov 22, 2021 at 12:35:11PM +0200, Sagi Grimberg wrote:

quoted

@@ -933,6 +934,7 @@ static int nvme_tcp_try_send_data(struct nvme_tcp_request *req)
 {
 	struct nvme_tcp_queue *queue = req->queue;
 	int req_data_len = req->data_len;
+	u32 h2cdata_left = req->h2cdata_left;

No need for the local variable, the reference only happens when the
pdu data transfer is completed.

As this function also executes for inline data it is possible that completion
gets processed and request gets allocated for new cmd before
if (req->h2cdata_left) check, nvme_tcp_setup_cmd_pdu() will set
req->h2cdata_left to 0 so it will work but it does not look correct
to me. IMO we should use local variable here.

quoted

 	while (true) {
 		struct page *page = nvme_tcp_req_cur_page(req);

@@ -977,7 +979,13 @@ static int nvme_tcp_try_send_data(struct nvme_tcp_request *req)
 				req->state = NVME_TCP_SEND_DDGST;
 				req->offset = 0;
 			} else {
-				nvme_tcp_done_send_req(queue);
+				if (h2cdata_left) {
+					nvme_tcp_setup_h2c_data_pdu(req);
+					req->state = NVME_TCP_SEND_H2C_PDU;
+					req->offset = 0;

quoted

@@ -1028,16 +1036,21 @@ static int nvme_tcp_try_send_data_pdu(struct nvme_tcp_request *req)
 {
 	struct nvme_tcp_queue *queue = req->queue;
 	struct nvme_tcp_data_pdu *pdu = req->pdu;
+	struct page *page = virt_to_page(pdu);
+	size_t offset = offset_in_page(pdu) + req->offset;
 	u8 hdgst = nvme_tcp_hdgst_len(queue);
 	int len = sizeof(*pdu) - req->offset + hdgst;
+	int flags = MSG_DONTWAIT | MSG_MORE | MSG_SENDPAGE_NOTLAST;
 	int ret;

Not sure the local variables are needed, but OK, I'm fine with that.

I will remove these local variables in next patch.

quoted

@@ -1057,6 +1070,7 @@ static int nvme_tcp_try_send_ddgst(struct nvme_tcp_request *req)
 {
 	struct nvme_tcp_queue *queue = req->queue;
 	size_t offset = req->offset;
+	u32 h2cdata_left = req->h2cdata_left;

No need for local variable.

Same explanation as above for local variable.