* [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU @ 2025-02-14 19:00 Maurizio Lombardi 2025-02-17 8:14 ` Sagi Grimberg 2025-02-17 10:56 ` Hannes Reinecke 0 siblings, 2 replies; 10+ messages in thread From: Maurizio Lombardi @ 2025-02-14 19:00 UTC (permalink / raw) To: kbusch; +Cc: axboe, hch, sagi, linux-nvme, hare, mlombard Previously, the NVMe/TCP host driver did not handle the C2HTermReq PDU, instead printing "unsupported pdu type (3)" when received. This patch adds support for processing the C2HTermReq PDU, allowing the driver to print the Fatal Error Status field. Example of output: nvme nvme4: Received C2HTermReq (FES = Invalid PDU Header Field) Signed-off-by: Maurizio Lombardi <mlombard@redhat.com> --- drivers/nvme/host/tcp.c | 37 +++++++++++++++++++++++++++++++++++++ include/linux/nvme-tcp.h | 2 ++ 2 files changed, 39 insertions(+) diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c index 841238f38fdd..8f783185575d 100644 --- a/drivers/nvme/host/tcp.c +++ b/drivers/nvme/host/tcp.c @@ -763,6 +763,40 @@ static int nvme_tcp_handle_r2t(struct nvme_tcp_queue *queue, return 0; } +static void nvme_tcp_handle_c2h_term(struct nvme_tcp_queue *queue, + struct nvme_tcp_term_pdu *pdu) +{ + u16 fes; + const char *msg; + u32 plen = le32_to_cpu(pdu->hdr.plen); + + static const char * const msg_table[] = { + [NVME_TCP_FES_INVALID_PDU_HDR] = "Invalid PDU Header Field", + [NVME_TCP_FES_PDU_SEQ_ERR] = "PDU Sequence Error", + [NVME_TCP_FES_HDR_DIGEST_ERR] = "Header Digest Error", + [NVME_TCP_FES_DATA_OUT_OF_RANGE] = "Data Transfer Out Of Range", + [NVME_TCP_FES_R2T_LIMIT_EXCEEDED] = "R2T Limit Exceeded", + [NVME_TCP_FES_UNSUPPORTED_PARAM] = "Unsupported Parameter", + }; + + if (plen < NVME_TCP_MIN_C2HTERM_PLEN || + plen > NVME_TCP_MAX_C2HTERM_PLEN) { + dev_err(queue->ctrl->ctrl.device, + "Received a malformed C2HTermReq PDU (plen = %u)\n", + plen); + return; + } + + fes = le16_to_cpu(pdu->fes); + if (fes && fes < ARRAY_SIZE(msg_table)) + msg = msg_table[fes]; + else + msg = "N/A"; + + dev_err(queue->ctrl->ctrl.device, + "Received C2HTermReq (FES = %s)\n", msg); +} + static int nvme_tcp_recv_pdu(struct nvme_tcp_queue *queue, struct sk_buff *skb, unsigned int *offset, size_t *len) { @@ -806,6 +840,9 @@ static int nvme_tcp_recv_pdu(struct nvme_tcp_queue *queue, struct sk_buff *skb, case nvme_tcp_r2t: nvme_tcp_init_recv_ctx(queue); return nvme_tcp_handle_r2t(queue, (void *)queue->pdu); + case nvme_tcp_c2h_term: + nvme_tcp_handle_c2h_term(queue, (void *)queue->pdu); + return -EINVAL; default: dev_err(queue->ctrl->ctrl.device, "unsupported pdu type (%d)\n", hdr->type); diff --git a/include/linux/nvme-tcp.h b/include/linux/nvme-tcp.h index e07e8978d691..e435250fcb4d 100644 --- a/include/linux/nvme-tcp.h +++ b/include/linux/nvme-tcp.h @@ -13,6 +13,8 @@ #define NVME_TCP_ADMIN_CCSZ SZ_8K #define NVME_TCP_DIGEST_LENGTH 4 #define NVME_TCP_MIN_MAXH2CDATA 4096 +#define NVME_TCP_MIN_C2HTERM_PLEN 24 +#define NVME_TCP_MAX_C2HTERM_PLEN 152 enum nvme_tcp_pfv { NVME_TCP_PFV_1_0 = 0x0, -- 2.43.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU 2025-02-14 19:00 [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU Maurizio Lombardi @ 2025-02-17 8:14 ` Sagi Grimberg 2025-02-17 13:26 ` Maurizio Lombardi 2025-02-17 10:56 ` Hannes Reinecke 1 sibling, 1 reply; 10+ messages in thread From: Sagi Grimberg @ 2025-02-17 8:14 UTC (permalink / raw) To: Maurizio Lombardi, kbusch; +Cc: axboe, hch, linux-nvme, hare, mlombard On 14/02/2025 21:00, Maurizio Lombardi wrote: > Previously, the NVMe/TCP host driver did not handle the C2HTermReq PDU, > instead printing "unsupported pdu type (3)" when received. This patch adds > support for processing the C2HTermReq PDU, allowing the driver > to print the Fatal Error Status field. > > Example of output: > nvme nvme4: Received C2HTermReq (FES = Invalid PDU Header Field) > > Signed-off-by: Maurizio Lombardi <mlombard@redhat.com> > --- > drivers/nvme/host/tcp.c | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/nvme-tcp.h | 2 ++ > 2 files changed, 39 insertions(+) > > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c > index 841238f38fdd..8f783185575d 100644 > --- a/drivers/nvme/host/tcp.c > +++ b/drivers/nvme/host/tcp.c > @@ -763,6 +763,40 @@ static int nvme_tcp_handle_r2t(struct nvme_tcp_queue *queue, > return 0; > } > > +static void nvme_tcp_handle_c2h_term(struct nvme_tcp_queue *queue, > + struct nvme_tcp_term_pdu *pdu) > +{ > + u16 fes; > + const char *msg; > + u32 plen = le32_to_cpu(pdu->hdr.plen); > + > + static const char * const msg_table[] = { > + [NVME_TCP_FES_INVALID_PDU_HDR] = "Invalid PDU Header Field", > + [NVME_TCP_FES_PDU_SEQ_ERR] = "PDU Sequence Error", > + [NVME_TCP_FES_HDR_DIGEST_ERR] = "Header Digest Error", > + [NVME_TCP_FES_DATA_OUT_OF_RANGE] = "Data Transfer Out Of Range", > + [NVME_TCP_FES_R2T_LIMIT_EXCEEDED] = "R2T Limit Exceeded", > + [NVME_TCP_FES_UNSUPPORTED_PARAM] = "Unsupported Parameter", > + }; > + > + if (plen < NVME_TCP_MIN_C2HTERM_PLEN || > + plen > NVME_TCP_MAX_C2HTERM_PLEN) { > + dev_err(queue->ctrl->ctrl.device, > + "Received a malformed C2HTermReq PDU (plen = %u)\n", > + plen); > + return; > + } > + > + fes = le16_to_cpu(pdu->fes); > + if (fes && fes < ARRAY_SIZE(msg_table)) > + msg = msg_table[fes]; > + else > + msg = "N/A"; msg = "unknown" ? we shouldn't suggest N/A because it should be very much applicable. > + > + dev_err(queue->ctrl->ctrl.device, > + "Received C2HTermReq (FES = %s)\n", msg); > +} > + AFAIR, the goal with C2HTerm was to also log the offending PDU itself. However I don't think its really necessary looking back. Can you test this also with header digest enabled? Conditioned that the above passes just fine, plus one minor nit, Reviewed-by: Sagi Grimberg <sagi@grimberg.me> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU 2025-02-17 8:14 ` Sagi Grimberg @ 2025-02-17 13:26 ` Maurizio Lombardi 0 siblings, 0 replies; 10+ messages in thread From: Maurizio Lombardi @ 2025-02-17 13:26 UTC (permalink / raw) To: Sagi Grimberg, Maurizio Lombardi, kbusch; +Cc: axboe, hch, linux-nvme, hare On Mon Feb 17, 2025 at 9:14 AM CET, Sagi Grimberg wrote: > >> + fes = le16_to_cpu(pdu->fes); >> + if (fes && fes < ARRAY_SIZE(msg_table)) >> + msg = msg_table[fes]; >> + else >> + msg = "N/A"; > > msg = "unknown" ? we shouldn't suggest N/A because it should be very > much applicable. Changed it to "Unknown". > >> + >> + dev_err(queue->ctrl->ctrl.device, >> + "Received C2HTermReq (FES = %s)\n", msg); >> +} >> + > > AFAIR, the goal with C2HTerm was to also log the offending PDU itself. > However I don't think > its really necessary looking back. Yes, it also provides this information, I just didn't want to make it too complicated to start with, I can improve it later. > > Can you test this also with header digest enabled? Indeed there is a small change to do. C2HTermReq is the only PDU coming from the controller that never includes Header or Data digests, so the driver must skip the checks. Will submit a V2 today. Thanks, Maurizio ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU 2025-02-14 19:00 [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU Maurizio Lombardi 2025-02-17 8:14 ` Sagi Grimberg @ 2025-02-17 10:56 ` Hannes Reinecke 2025-02-17 13:29 ` Maurizio Lombardi 2025-02-24 14:38 ` Maurizio Lombardi 1 sibling, 2 replies; 10+ messages in thread From: Hannes Reinecke @ 2025-02-17 10:56 UTC (permalink / raw) To: Maurizio Lombardi, kbusch; +Cc: axboe, hch, sagi, linux-nvme, hare, mlombard On 2/14/25 20:00, Maurizio Lombardi wrote: > Previously, the NVMe/TCP host driver did not handle the C2HTermReq PDU, > instead printing "unsupported pdu type (3)" when received. This patch adds > support for processing the C2HTermReq PDU, allowing the driver > to print the Fatal Error Status field. > > Example of output: > nvme nvme4: Received C2HTermReq (FES = Invalid PDU Header Field) > > Signed-off-by: Maurizio Lombardi <mlombard@redhat.com> > --- > drivers/nvme/host/tcp.c | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/nvme-tcp.h | 2 ++ > 2 files changed, 39 insertions(+) > > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c > index 841238f38fdd..8f783185575d 100644 > --- a/drivers/nvme/host/tcp.c > +++ b/drivers/nvme/host/tcp.c > @@ -763,6 +763,40 @@ static int nvme_tcp_handle_r2t(struct nvme_tcp_queue *queue, > return 0; > } > > +static void nvme_tcp_handle_c2h_term(struct nvme_tcp_queue *queue, > + struct nvme_tcp_term_pdu *pdu) > +{ > + u16 fes; > + const char *msg; > + u32 plen = le32_to_cpu(pdu->hdr.plen); > + > + static const char * const msg_table[] = { > + [NVME_TCP_FES_INVALID_PDU_HDR] = "Invalid PDU Header Field", > + [NVME_TCP_FES_PDU_SEQ_ERR] = "PDU Sequence Error", > + [NVME_TCP_FES_HDR_DIGEST_ERR] = "Header Digest Error", > + [NVME_TCP_FES_DATA_OUT_OF_RANGE] = "Data Transfer Out Of Range", > + [NVME_TCP_FES_R2T_LIMIT_EXCEEDED] = "R2T Limit Exceeded", > + [NVME_TCP_FES_UNSUPPORTED_PARAM] = "Unsupported Parameter", > + }; > + > + if (plen < NVME_TCP_MIN_C2HTERM_PLEN || > + plen > NVME_TCP_MAX_C2HTERM_PLEN) { > + dev_err(queue->ctrl->ctrl.device, > + "Received a malformed C2HTermReq PDU (plen = %u)\n", > + plen); > + return; > + } > + > + fes = le16_to_cpu(pdu->fes); > + if (fes && fes < ARRAY_SIZE(msg_table)) > + msg = msg_table[fes]; > + else > + msg = "N/A"; > + > + dev_err(queue->ctrl->ctrl.device, > + "Received C2HTermReq (FES = %s)\n", msg); > +} > + > static int nvme_tcp_recv_pdu(struct nvme_tcp_queue *queue, struct sk_buff *skb, > unsigned int *offset, size_t *len) > { > @@ -806,6 +840,9 @@ static int nvme_tcp_recv_pdu(struct nvme_tcp_queue *queue, struct sk_buff *skb, > case nvme_tcp_r2t: > nvme_tcp_init_recv_ctx(queue); > return nvme_tcp_handle_r2t(queue, (void *)queue->pdu); > + case nvme_tcp_c2h_term: > + nvme_tcp_handle_c2h_term(queue, (void *)queue->pdu); > + return -EINVAL; > default: > dev_err(queue->ctrl->ctrl.device, > "unsupported pdu type (%d)\n", hdr->type); > diff --git a/include/linux/nvme-tcp.h b/include/linux/nvme-tcp.h > index e07e8978d691..e435250fcb4d 100644 > --- a/include/linux/nvme-tcp.h > +++ b/include/linux/nvme-tcp.h > @@ -13,6 +13,8 @@ > #define NVME_TCP_ADMIN_CCSZ SZ_8K > #define NVME_TCP_DIGEST_LENGTH 4 > #define NVME_TCP_MIN_MAXH2CDATA 4096 > +#define NVME_TCP_MIN_C2HTERM_PLEN 24 > +#define NVME_TCP_MAX_C2HTERM_PLEN 152 > > enum nvme_tcp_pfv { > NVME_TCP_PFV_1_0 = 0x0, Can you add support for nvmet, too, such that we can test the patch? (And maybe even a blktest script for it?) Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU 2025-02-17 10:56 ` Hannes Reinecke @ 2025-02-17 13:29 ` Maurizio Lombardi 2025-02-24 14:38 ` Maurizio Lombardi 1 sibling, 0 replies; 10+ messages in thread From: Maurizio Lombardi @ 2025-02-17 13:29 UTC (permalink / raw) To: Hannes Reinecke, Maurizio Lombardi, kbusch Cc: axboe, hch, sagi, linux-nvme, hare On Mon Feb 17, 2025 at 11:56 AM CET, Hannes Reinecke wrote: > > Can you add support for nvmet, too, such that we can test the patch? > (And maybe even a blktest script for it?) Ok, going to look at this. Thanks, Maurizio ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU 2025-02-17 10:56 ` Hannes Reinecke 2025-02-17 13:29 ` Maurizio Lombardi @ 2025-02-24 14:38 ` Maurizio Lombardi 2025-02-24 16:04 ` Hannes Reinecke 1 sibling, 1 reply; 10+ messages in thread From: Maurizio Lombardi @ 2025-02-24 14:38 UTC (permalink / raw) To: Hannes Reinecke, Maurizio Lombardi, kbusch Cc: axboe, hch, sagi, linux-nvme, hare On Mon Feb 17, 2025 at 11:56 AM CET, Hannes Reinecke wrote: > Can you add support for nvmet, too, such that we can test the patch? > (And maybe even a blktest script for it?) Possible target-side implementation here. Question is if it's acceptable to send the packet in blocking mode (MSG_DONTWAIT unset). Errors in nvmet_send_c2h_term() can be safely ignored because in any case we are going to perform a fatal error recovery immediately after. Example of dmesg in the target: nvmet_tcp: queue 2: header digest error: recv 0xcf5f1cf7 expected 0x751607d3 nvmet: ctrl 1 fatal error occurred! Example of dmesg in the host: nvme nvme0: Received C2HTermReq (FES = Header Digest Error) nvme nvme0: C2HTermReq: invalid digest = 0xcf5f1cf7 diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c index 7c51c2a8c109..16438f2a624d 100644 --- a/drivers/nvme/target/tcp.c +++ b/drivers/nvme/target/tcp.c @@ -217,6 +217,9 @@ static struct workqueue_struct *nvmet_tcp_wq; static const struct nvmet_fabrics_ops nvmet_tcp_ops; static void nvmet_tcp_free_cmd(struct nvmet_tcp_cmd *c); static void nvmet_tcp_free_cmd_buffers(struct nvmet_tcp_cmd *cmd); +static void nvmet_send_c2h_term(struct nvmet_tcp_queue *queue, + enum nvme_tcp_fatal_error_status fes, + u32 field_offset); static inline u16 nvmet_tcp_cmd_tag(struct nvmet_tcp_queue *queue, struct nvmet_tcp_cmd *cmd) @@ -322,6 +325,8 @@ static int nvmet_tcp_verify_hdgst(struct nvmet_tcp_queue *queue, nvmet_tcp_hdgst(queue->rcv_hash, pdu, len); exp_digest = *(__le32 *)(pdu + hdr->hlen); if (recv_digest != exp_digest) { + /* Restore the original value for C2HTermReq */ + *(__le32 *)(pdu + hdr->hlen) = recv_digest; pr_err("queue %d: header digest error: recv %#x expected %#x\n", queue->idx, le32_to_cpu(recv_digest), le32_to_cpu(exp_digest)); @@ -999,6 +1004,7 @@ static int nvmet_tcp_handle_h2c_data_pdu(struct nvmet_tcp_queue *queue) pr_err("ttag %u unexpected data offset %u (expected %u)\n", data->ttag, le32_to_cpu(data->data_offset), cmd->rbytes_done); + nvmet_send_c2h_term(queue, NVME_TCP_FES_DATA_OUT_OF_RANGE, 0); goto err_proto; } @@ -1012,6 +1018,8 @@ static int nvmet_tcp_handle_h2c_data_pdu(struct nvmet_tcp_queue *queue) cmd->pdu_len == 0 || cmd->pdu_len > NVMET_TCP_MAXH2CDATA)) { pr_err("H2CData PDU len %u is invalid\n", cmd->pdu_len); + if (cmd->pdu_len > NVMET_TCP_MAXH2CDATA) + nvmet_send_c2h_term(queue, NVME_TCP_FES_DATA_LIMIT_EXCEEDED, 0); goto err_proto; } cmd->pdu_recv = 0; @@ -1173,6 +1181,55 @@ static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue, return ret; } +static void nvmet_send_c2h_term(struct nvmet_tcp_queue *queue, + enum nvme_tcp_fatal_error_status fes, + u32 field_offset) +{ + struct nvme_tcp_cmd_pdu *cmd = &queue->pdu.cmd; + struct nvme_tcp_term_pdu *term_pdu; + size_t cmd_size = nvmet_tcp_pdu_size(cmd->hdr.type); + size_t pdu_size = sizeof(*term_pdu) + cmd_size; + struct msghdr msg = { .msg_flags = MSG_EOR }; + __le32 fei; + struct bio_vec bvec; + + if (!cmd_size) + return; + + term_pdu = kzalloc(pdu_size, GFP_KERNEL); + if (!term_pdu) + return; + + switch (fes) { + case NVME_TCP_FES_INVALID_PDU_HDR: + case NVME_TCP_FES_UNSUPPORTED_PARAM: + fei = field_offset; + break; + case NVME_TCP_FES_HDR_DIGEST_ERR: + fei = le32_to_cpu(*(__le32 *)((u8 *)cmd + cmd->hdr.hlen)); + break; + default: + fei = 0; + break; + } + term_pdu->feil = cpu_to_le16(lower_16_bits(fei)); + term_pdu->feiu = cpu_to_le16(upper_16_bits(fei)); + term_pdu->fes = cpu_to_le16(fes); + + memcpy((u8 *)term_pdu + sizeof(*term_pdu), cmd, cmd_size); + + term_pdu->hdr.type = nvme_tcp_c2h_term; + term_pdu->hdr.flags = 0; + term_pdu->hdr.hlen = sizeof(*term_pdu); + term_pdu->hdr.plen = cpu_to_le32(pdu_size); + + bvec_set_virt(&bvec, (void *)term_pdu, pdu_size); + iov_iter_bvec(&msg.msg_iter, ITER_SOURCE, &bvec, 1, pdu_size); + sock_sendmsg(queue->sock, &msg); + + kfree(term_pdu); +} + static int nvmet_tcp_try_recv_pdu(struct nvmet_tcp_queue *queue) { struct nvme_tcp_hdr *hdr = &queue->pdu.cmd.hdr; @@ -1223,6 +1280,7 @@ static int nvmet_tcp_try_recv_pdu(struct nvmet_tcp_queue *queue) if (queue->hdr_digest && nvmet_tcp_verify_hdgst(queue, &queue->pdu, hdr->hlen)) { + nvmet_send_c2h_term(queue, NVME_TCP_FES_HDR_DIGEST_ERR, 0); nvmet_tcp_fatal_error(queue); /* fatal */ return -EPROTO; } -- 2.43.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU 2025-02-24 14:38 ` Maurizio Lombardi @ 2025-02-24 16:04 ` Hannes Reinecke 2025-02-24 16:48 ` Maurizio Lombardi 0 siblings, 1 reply; 10+ messages in thread From: Hannes Reinecke @ 2025-02-24 16:04 UTC (permalink / raw) To: Maurizio Lombardi, Maurizio Lombardi, kbusch Cc: axboe, hch, sagi, linux-nvme, hare On 2/24/25 15:38, Maurizio Lombardi wrote: > On Mon Feb 17, 2025 at 11:56 AM CET, Hannes Reinecke wrote: >> Can you add support for nvmet, too, such that we can test the patch? >> (And maybe even a blktest script for it?) > > Possible target-side implementation here. > > Question is if it's acceptable to send the packet in > blocking mode (MSG_DONTWAIT unset). > I really wouldn't be doing that, as we need to terminate the queue upon fatal error and don't really want to delay that (eg by waiting for the transfer to finish) and > Errors in nvmet_send_c2h_term() can be safely ignored because in any > case we are going to perform a fatal error recovery immediately after. > we don't really care about the error on sending anyway, so this is just a 'best effort' thingie. If it works, fine, if not, fine too. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU 2025-02-24 16:04 ` Hannes Reinecke @ 2025-02-24 16:48 ` Maurizio Lombardi 2025-02-24 17:03 ` Hannes Reinecke 0 siblings, 1 reply; 10+ messages in thread From: Maurizio Lombardi @ 2025-02-24 16:48 UTC (permalink / raw) To: Hannes Reinecke, Maurizio Lombardi, kbusch Cc: axboe, hch, sagi, linux-nvme, hare On Mon Feb 24, 2025 at 5:04 PM CET, Hannes Reinecke wrote: > I really wouldn't be doing that, as we need to terminate the queue upon > fatal error and don't really want to delay that (eg by waiting for the > transfer to finish) and I did some tests and it seems to work with MSG_DONTWAIT as well. I am just wondering if calling kernel_sock_shutdown() could in some cases race with MSG_DONTWAIT, preventing the packet from being effectively sent. Not a big problem anyway because, as you said, it's a kind of best-effort thing. Maurizio ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU 2025-02-24 16:48 ` Maurizio Lombardi @ 2025-02-24 17:03 ` Hannes Reinecke 2025-02-25 7:41 ` Sagi Grimberg 0 siblings, 1 reply; 10+ messages in thread From: Hannes Reinecke @ 2025-02-24 17:03 UTC (permalink / raw) To: Maurizio Lombardi, Maurizio Lombardi, kbusch Cc: axboe, hch, sagi, linux-nvme, hare On 2/24/25 17:48, Maurizio Lombardi wrote: > On Mon Feb 24, 2025 at 5:04 PM CET, Hannes Reinecke wrote: >> I really wouldn't be doing that, as we need to terminate the queue upon >> fatal error and don't really want to delay that (eg by waiting for the >> transfer to finish) and > > I did some tests and it seems to work with MSG_DONTWAIT as well. > > I am just wondering if calling kernel_sock_shutdown() could > in some cases race with MSG_DONTWAIT, preventing the packet from > being effectively sent. > Not a big problem anyway because, as you said, it's a kind > of best-effort thing. > 'xactly. If in doubt we should prefer latency over correctness; and especially when running over a more complicated network setup (or doing TLS) it's anyone's guess if the entire stack is still operational by the time we're trying to send the c2hterm. Part of the reason why it wasn't implement initially I guess :-) Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU 2025-02-24 17:03 ` Hannes Reinecke @ 2025-02-25 7:41 ` Sagi Grimberg 0 siblings, 0 replies; 10+ messages in thread From: Sagi Grimberg @ 2025-02-25 7:41 UTC (permalink / raw) To: Hannes Reinecke, Maurizio Lombardi, Maurizio Lombardi, kbusch Cc: axboe, hch, linux-nvme, hare On 24/02/2025 19:03, Hannes Reinecke wrote: > On 2/24/25 17:48, Maurizio Lombardi wrote: >> On Mon Feb 24, 2025 at 5:04 PM CET, Hannes Reinecke wrote: >>> I really wouldn't be doing that, as we need to terminate the queue upon >>> fatal error and don't really want to delay that (eg by waiting for the >>> transfer to finish) and >> >> I did some tests and it seems to work with MSG_DONTWAIT as well. >> >> I am just wondering if calling kernel_sock_shutdown() could >> in some cases race with MSG_DONTWAIT, preventing the packet from >> being effectively sent. >> Not a big problem anyway because, as you said, it's a kind >> of best-effort thing. >> > 'xactly. > If in doubt we should prefer latency over correctness; and especially > when running over a more complicated network setup (or doing TLS) it's > anyone's guess if the entire stack is still operational by the time > we're trying to send the c2hterm. > Part of the reason why it wasn't implement initially I guess :-) Agree, this is a best effort thing. No need to jump through hoops to get it... ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-02-25 7:43 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-02-14 19:00 [PATCH] nvme-tcp: add basic support for the C2HTermReq PDU Maurizio Lombardi 2025-02-17 8:14 ` Sagi Grimberg 2025-02-17 13:26 ` Maurizio Lombardi 2025-02-17 10:56 ` Hannes Reinecke 2025-02-17 13:29 ` Maurizio Lombardi 2025-02-24 14:38 ` Maurizio Lombardi 2025-02-24 16:04 ` Hannes Reinecke 2025-02-24 16:48 ` Maurizio Lombardi 2025-02-24 17:03 ` Hannes Reinecke 2025-02-25 7:41 ` Sagi Grimberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox