Re: [PATCH v2 2/2] nvme-tcp: send H2CData PDUs based on MAXH2CDATA

[Varun Prakash <varun@chelsio.com>]

On Mon, Nov 22, 2021 at 12:35:11PM +0200, Sagi Grimberg wrote:
> >@@ -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.

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

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