Re: [PATCH net-next 1/9] dlm: fix send buffer backpressure handling

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] fs/dlm/lowcomms.c:send_to_sock() — making the CF_APP_LIMITED
  branch live for the first time in ten years introduces a permanent…
- [Low] fs/dlm/lowcomms.c:send_to_sock(): after switching the guard to
  SOCK_NOSPACE, the guard and the "Notify TCP" action now operate on the…

--

dlm: fix send buffer backpressure handling

The send path in fs/dlm/lowcomms.c tested SOCKWQ_ASYNC_NOSPACE in
con->sock->flags, a bit that has not lived there since commit ceb5d58b2170,
so the test was always false and send_to_sock() always returned
DLM_IO_RESCHED on -EAGAIN. The patch switches the test to SOCK_NOSPACE and
drops the matching clear_bit() in lowcomms_write_space().

> diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> index 2aff1c7c17de4..abe9ae4c643fa 100644
> --- a/fs/dlm/lowcomms.c
> +++ b/fs/dlm/lowcomms.c
> @@ -522,10 +522,8 @@ static void lowcomms_write_space(struct sock *sk)
>  	clear_bit(SOCK_NOSPACE, &con->sock->flags);
>  
>  	spin_lock_bh(&con->writequeue_lock);
> -	if (test_and_clear_bit(CF_APP_LIMITED, &con->flags)) {
> +	if (test_and_clear_bit(CF_APP_LIMITED, &con->flags))
>  		con->sock->sk->sk_write_pending--;
> -		clear_bit(SOCKWQ_ASYNC_NOSPACE, &con->sock->flags);
> -	}
>  
>  	lowcomms_queue_swork(con);
>  	spin_unlock_bh(&con->writequeue_lock);
> @@ -1391,7 +1389,7 @@ static int send_to_sock(struct connection *con)
>  	if (ret == -EAGAIN || ret == 0) {
>  		lock_sock(con->sock->sk);
>  		spin_lock_bh(&con->writequeue_lock);
> -		if (test_bit(SOCKWQ_ASYNC_NOSPACE, &con->sock->flags) &&
> +		if (test_bit(SOCK_NOSPACE, &con->sock->flags) &&
>  		    !test_and_set_bit(CF_APP_LIMITED, &con->flags)) {

[Severity: High]
This makes the CF_APP_LIMITED branch reachable for the first time in ten
years.  Once it is taken, is there anything left that can restart this
connection if the TCP connection then dies?

send_to_sock() parks the send worker:

	if (test_bit(SOCK_NOSPACE, &con->sock->flags) &&
	    !test_and_set_bit(CF_APP_LIMITED, &con->flags)) {
		...
		con->sock->sk->sk_write_pending++;
		clear_bit(CF_SEND_PENDING, &con->flags);
		...
		/* wait for write_space() event */
		return DLM_IO_END;
	}

process_send_sockets() just breaks on DLM_IO_END, so nothing is queued on
io_workqueue for this connection any more.

The only place that clears CF_APP_LIMITED on a live socket is
lowcomms_write_space(), and that is only reached from TCP ACK processing
(tcp_check_space() -> tcp_new_space() -> sk->sk_write_space()).

If the peer stops acking, tcp_reset() or tcp_write_err() end up in
tcp_done_with_error(), which purges the write queue, calls tcp_done() and
sk_error_report().  None of those call sk->sk_write_space(), and an RST
does not produce an sk_data_ready() either, so neither worker is woken.

Every remaining restart point goes through lowcomms_queue_swork(), which
refuses to queue while CF_APP_LIMITED is set:

	if (!test_bit(CF_IO_STOP, &con->flags) &&
	    !test_bit(CF_APP_LIMITED, &con->flags) &&
	    !test_and_set_bit(CF_SEND_PENDING, &con->flags))
		queue_work(io_workqueue, &con->swork);

That gate covers _dlm_lowcomms_commit_msg() for new messages, the
retransmit path lowcomms_error_report() -> dlm_midcomms_unack_msg_resend()
-> dlm_lowcomms_resend_msg() -> _dlm_lowcomms_commit_msg(), and the
receive-side error recovery in process_recv_sockets():

		if (ret < 0) {
			if (test_bit(CF_IS_OTHERCON, &con->flags)) {
				close_connection(con, false);
			} else {
				spin_lock_bh(&con->writequeue_lock);
				lowcomms_queue_swork(con);
				spin_unlock_bh(&con->writequeue_lock);
			}

lowcomms_error_report() only logs, asks for a retransmit and forwards to
the default error report; it neither clears CF_APP_LIMITED nor closes the
connection.  dlm_lowcomms_connect_node() only queues swork when
con->sock == NULL, and the dead socket is never closed, so con->sock stays
non-NULL.

Before this change the (dead) test meant send_to_sock() returned
DLM_IO_RESCHED, swork was requeued, the next sock_sendmsg() picked up the
socket error and the ret < 0 path did close_connection() plus
lowcomms_queue_swork().  So the CPU-burning loop the patch removes was
also the error detection and reconnect mechanism.

Would it make sense to add a fallback that clears CF_APP_LIMITED or closes
the connection when sk_err is set, so a peer RST or retransmission timeout
during the app-limited state does not leave the connection permanently
silent?

>  			/* Notify TCP that we're limited by the
>  			 * application window size.

[Severity: Low]
With the test switched to SOCK_NOSPACE, does the body of this branch still
do anything?  The guard and the notification now touch the same bit in the
same word:

	if (test_bit(SOCK_NOSPACE, &con->sock->flags) &&
	    !test_and_set_bit(CF_APP_LIMITED, &con->flags)) {
		/* Notify TCP that we're limited by the
		 * application window size.
		 */
		set_bit(SOCK_NOSPACE, &con->sock->sk->sk_socket->flags);

For the kernel sockets dlm creates, sk->sk_socket == con->sock, which is
also how lowcomms_write_space() spells it:

	clear_bit(SOCK_NOSPACE, &con->sock->flags);

lock_sock(con->sock->sk) is held across the test and the set, so the bit
cannot be cleared in between, and the set_bit() can never change state.
The comment about notifying TCP then describes something the code no
longer does.

The two different spellings of one flags word inside a single if statement
are the same kind of trap that produced the dead test this patch fixes, so
perhaps the set_bit() and its comment could be dropped, or at least
spelled consistently as con->sock->flags.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922122721.3568295-1-edumazet%40google.com