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

[netdev-bot+sashiko@kernel.org]

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

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

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] This patch turns on the CF_APP_LIMITED / DLM_IO_END wait in
  send_to_sock() (fs/dlm/lowcomms.c).

--

dlm: fix send buffer backpressure handling

This patch changes send_to_sock() to test SOCK_NOSPACE instead of
SOCKWQ_ASYNC_NOSPACE. That bit has not lived in socket->flags since it
moved to socket_wq->flags.

With the patch, a TCP -EAGAIN sets CF_APP_LIMITED and returns
DLM_IO_END to wait for lowcomms_write_space(). Before, the send work was
requeued right away.

The patch also removes the stale clear_bit() of SOCKWQ_ASYNC_NOSPACE from
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

[ ... ]

> @@ -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)) {
>  			/* Notify TCP that we're limited by the
>  			 * application window size.

[Severity: High]
If the TCP connection is aborted while CF_APP_LIMITED is set, can the
send worker for that node stay stuck for good?

With this change the CF_APP_LIMITED / DLM_IO_END path is now live for
TCP. Once the flag is set, only lowcomms_write_space() or
close_connection() clear it.

A connection can be aborted by an RST from the peer (tcp_reset()), or by
tcp_write_err() after a retransmit, zero window probe or keepalive
timeout. All of these end up in tcp_done_with_error():

net/ipv4/tcp_input.c:tcp_done_with_error() {
    ...
	WRITE_ONCE(sk->sk_err, err);
	smp_wmb();
	tcp_write_queue_purge(sk);
	tcp_done(sk);
	if (!sock_flag(sk, SOCK_DEAD))
		sk_error_report(sk);
    ...
}

That path never calls sk->sk_write_space(), so lowcomms_write_space()
does not run.

lowcomms_error_report() does run, and it resends like this:

lowcomms_error_report()
  dlm_midcomms_unack_msg_resend()
    dlm_lowcomms_resend_msg()
      _dlm_lowcomms_commit_msg()
        lowcomms_queue_swork()

However, lowcomms_queue_swork() does nothing while the flag 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);

An RST does not call sk_data_ready, so process_recv_sockets() usually
does not run. When it does, its ret < 0 path for the main connection also
goes through lowcomms_queue_swork(), and is blocked in the same way.

So send_to_sock() never calls sock_sendmsg() again and never sees the
socket error. As a result, process_send_sockets() never reaches:

	default:
		if (ret < 0) {
			close_connection(con, false);

After that, every dlm_lowcomms_commit_msg() to that node is queued but
never sent.

If the peer reconnects, accept_from_sock() still finds con->sock set.
It attaches the new socket to othercon, which only receives. The send
side stays stuck until membership recovery calls dlm_lowcomms_close().

There is also a race here. Suppose the RST arrives after sock_sendmsg()
returns -EAGAIN but before lock_sock(). The error_report kick is lost,
because CF_SEND_PENDING is still set.

This code then sees SOCK_NOSPACE still set, and arms CF_APP_LIMITED on a
socket that is already dead. Neither sk->sk_err nor sk_state is checked
before it arms the flag.

Before this patch, send_to_sock() always returned DLM_IO_RESCHED. The
next sendmsg() on the dead socket failed with -ECONNRESET or -EPIPE, and
process_send_sockets() then closed the connection and reconnected.

The commit message says:

    This restores the semantics dlm had before the bit moved.

The CF_APP_LIMITED check in lowcomms_queue_swork() was added in 2022 by
commit dbb751ffab0b ("fs: dlm: parallelize lowcomms socket handling").
That was long after the bit stopped working.

Has that check ever been exercised with a working CF_APP_LIMITED path?

The later patches in this series do not seem to change this. They only
replace the bit operations with sk_set_nospace() / sk_clear_nospace().
lowcomms_error_report(), lowcomms_queue_swork() and this arming check
stay the same.

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