[PATCH v2] nvme/tcp: handle tls partially sent records in write_space()

[PATCH v2] nvme/tcp: handle tls partially sent records in write_space()

[Wilfred Mallawa]

From: Wilfred Mallawa <wilfred.mallawa@wdc.com>

With TLS enabled, records that are encrypted and appended to TLS TX
list can fail to see a retry if the underlying TCP socket is busy, for
example, hitting an EAGAIN from tcp_sendmsg_locked(). This is not known
to the NVMe TCP driver, as the TLS layer successfully generated a record.

Typically, the TLS write_space() callback would ensure such records are
retried, but in the NVMe TCP Host driver, write_space() invokes
nvme_tcp_write_space(). This causes a partially sent record in the TLS TX
list to timeout after not being retried.

This patch fixes the above by calling queue->write_space(), which calls
into the TLS layer to retry any pending records.

Fixes: be8e82caa685 ("nvme-tcp: enable TLS handshake upcall")
Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
---
V2:
    - Unconditionally invoke TLS write_space(). This means we don't need
      to export tls_is_partially_sent_record()
---
drivers/nvme/host/tcp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 1413788ca7d5..6016510577bd 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1079,8 +1079,13 @@ static void nvme_tcp_write_space(struct sock *sk)
 
 	read_lock_bh(&sk->sk_callback_lock);
 	queue = sk->sk_user_data;
+
 	if (likely(queue && sk_stream_is_writeable(sk))) {
 		clear_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
+		/* Ensure pending TLS partial records are retried */
+		if (nvme_tcp_queue_tls(queue))
+			queue->write_space(sk);
+
 		queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue->io_work);
 	}
 	read_unlock_bh(&sk->sk_callback_lock);
-- 
2.51.0

Re: [PATCH v2] nvme/tcp: handle tls partially sent records in write_space()

[Damien Le Moal]

On 2025/10/10 13:25, Wilfred Mallawa wrote:
> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> With TLS enabled, records that are encrypted and appended to TLS TX
> list can fail to see a retry if the underlying TCP socket is busy, for
> example, hitting an EAGAIN from tcp_sendmsg_locked(). This is not known
> to the NVMe TCP driver, as the TLS layer successfully generated a record.
> 
> Typically, the TLS write_space() callback would ensure such records are
> retried, but in the NVMe TCP Host driver, write_space() invokes
> nvme_tcp_write_space(). This causes a partially sent record in the TLS TX
> list to timeout after not being retried.
> 
> This patch fixes the above by calling queue->write_space(), which calls
> into the TLS layer to retry any pending records.
> 
> Fixes: be8e82caa685 ("nvme-tcp: enable TLS handshake upcall")
> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> ---
> V2:
>     - Unconditionally invoke TLS write_space(). This means we don't need
>       to export tls_is_partially_sent_record()
> ---
> drivers/nvme/host/tcp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 1413788ca7d5..6016510577bd 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1079,8 +1079,13 @@ static void nvme_tcp_write_space(struct sock *sk)
>  
>  	read_lock_bh(&sk->sk_callback_lock);
>  	queue = sk->sk_user_data;
> +

White space change... Remove please.

>  	if (likely(queue && sk_stream_is_writeable(sk))) {
>  		clear_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> +		/* Ensure pending TLS partial records are retried */
> +		if (nvme_tcp_queue_tls(queue))
> +			queue->write_space(sk);
> +

Maybe remove this empty line too.

>  		queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue->io_work);
>  	}
>  	read_unlock_bh(&sk->sk_callback_lock);


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] nvme/tcp: handle tls partially sent records in write_space()

[Wilfred Mallawa]

On Fri, 2025-10-10 at 14:25 +0900, Damien Le Moal wrote:
> On 2025/10/10 13:25, Wilfred Mallawa wrote:
> > From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > 
> > With TLS enabled, records that are encrypted and appended to TLS TX
> > list can fail to see a retry if the underlying TCP socket is busy,
> > for
> > example, hitting an EAGAIN from tcp_sendmsg_locked(). This is not
> > known
> > to the NVMe TCP driver, as the TLS layer successfully generated a
> > record.
> > 
> > Typically, the TLS write_space() callback would ensure such records
> > are
> > retried, but in the NVMe TCP Host driver, write_space() invokes
> > nvme_tcp_write_space(). This causes a partially sent record in the
> > TLS TX
> > list to timeout after not being retried.
> > 
> > This patch fixes the above by calling queue->write_space(), which
> > calls
> > into the TLS layer to retry any pending records.
> > 
> > Fixes: be8e82caa685 ("nvme-tcp: enable TLS handshake upcall")
> > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > ---
> > V2:
> >     - Unconditionally invoke TLS write_space(). This means we don't
> > need
> >       to export tls_is_partially_sent_record()
> > ---
> > drivers/nvme/host/tcp.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > index 1413788ca7d5..6016510577bd 100644
> > --- a/drivers/nvme/host/tcp.c
> > +++ b/drivers/nvme/host/tcp.c
> > @@ -1079,8 +1079,13 @@ static void nvme_tcp_write_space(struct sock
> > *sk)
> >  
> >  	read_lock_bh(&sk->sk_callback_lock);
> >  	queue = sk->sk_user_data;
> > +
> 
> White space change... Remove please.
> 
> >  	if (likely(queue && sk_stream_is_writeable(sk))) {
> >  		clear_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> > +		/* Ensure pending TLS partial records are retried
> > */
> > +		if (nvme_tcp_queue_tls(queue))
> > +			queue->write_space(sk);
> > +
> 
> Maybe remove this empty line too.
Okay, Fixed in V3. Thanks!

Wilfred
> 
> >  		queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue-
> > >io_work);
> >  	}
> >  	read_unlock_bh(&sk->sk_callback_lock);
> 

Re: [PATCH v2] nvme/tcp: handle tls partially sent records in write_space()

[Hannes Reinecke]

On 10/10/25 06:25, Wilfred Mallawa wrote:
> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> With TLS enabled, records that are encrypted and appended to TLS TX
> list can fail to see a retry if the underlying TCP socket is busy, for
> example, hitting an EAGAIN from tcp_sendmsg_locked(). This is not known
> to the NVMe TCP driver, as the TLS layer successfully generated a record.
> 
> Typically, the TLS write_space() callback would ensure such records are
> retried, but in the NVMe TCP Host driver, write_space() invokes
> nvme_tcp_write_space(). This causes a partially sent record in the TLS TX
> list to timeout after not being retried.
> 
> This patch fixes the above by calling queue->write_space(), which calls
> into the TLS layer to retry any pending records.
> 
> Fixes: be8e82caa685 ("nvme-tcp: enable TLS handshake upcall")
> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> ---
> V2:
>      - Unconditionally invoke TLS write_space(). This means we don't need
>        to export tls_is_partially_sent_record()
> ---
> drivers/nvme/host/tcp.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 1413788ca7d5..6016510577bd 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1079,8 +1079,13 @@ static void nvme_tcp_write_space(struct sock *sk)
>   
>   	read_lock_bh(&sk->sk_callback_lock);
>   	queue = sk->sk_user_data;
> +
>   	if (likely(queue && sk_stream_is_writeable(sk))) {
>   		clear_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> +		/* Ensure pending TLS partial records are retried */
> +		if (nvme_tcp_queue_tls(queue))
> +			queue->write_space(sk);
> +
>   		queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue->io_work);
>   	}
>   	read_unlock_bh(&sk->sk_callback_lock);

Minus the whitespace change:

Reviewed-by: Hannes Reinecke <hare@suse.de>

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

Re: [PATCH v2] nvme/tcp: handle tls partially sent records in write_space()

[Wilfred Mallawa]

On Fri, 2025-10-10 at 09:03 +0200, Hannes Reinecke wrote:
> On 10/10/25 06:25, Wilfred Mallawa wrote:
> > From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > 
> > With TLS enabled, records that are encrypted and appended to TLS TX
> > list can fail to see a retry if the underlying TCP socket is busy,
> > for
> > example, hitting an EAGAIN from tcp_sendmsg_locked(). This is not
> > known
> > to the NVMe TCP driver, as the TLS layer successfully generated a
> > record.
> > 
> > Typically, the TLS write_space() callback would ensure such records
> > are
> > retried, but in the NVMe TCP Host driver, write_space() invokes
> > nvme_tcp_write_space(). This causes a partially sent record in the
> > TLS TX
> > list to timeout after not being retried.
> > 
> > This patch fixes the above by calling queue->write_space(), which
> > calls
> > into the TLS layer to retry any pending records.
> > 
> > Fixes: be8e82caa685 ("nvme-tcp: enable TLS handshake upcall")
> > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > ---
> > V2:
> >      - Unconditionally invoke TLS write_space(). This means we
> > don't need
> >        to export tls_is_partially_sent_record()
> > ---
> > drivers/nvme/host/tcp.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > index 1413788ca7d5..6016510577bd 100644
> > --- a/drivers/nvme/host/tcp.c
> > +++ b/drivers/nvme/host/tcp.c
> > @@ -1079,8 +1079,13 @@ static void nvme_tcp_write_space(struct sock
> > *sk)
> >   
> >   	read_lock_bh(&sk->sk_callback_lock);
> >   	queue = sk->sk_user_data;
> > +
> >   	if (likely(queue && sk_stream_is_writeable(sk))) {
> >   		clear_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> > +		/* Ensure pending TLS partial records are retried
> > */
> > +		if (nvme_tcp_queue_tls(queue))
> > +			queue->write_space(sk);
> > +
> >   		queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue-
> > >io_work);
> >   	}
> >   	read_unlock_bh(&sk->sk_callback_lock);
> 
> Minus the whitespace change:
> 
> Reviewed-by: Hannes Reinecke <hare@suse.de>

Ah only just saw this, sent a V3 with the white-space fixed. Thanks for
the review!

Regards,
Wilfred

> 
> Cheers,
> 
> Hannes