Re: [PATCH net v11 04/17] rxrpc: Fix lack of short-send handling in rxrpc_kernel_send_data()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch adds this sentence to
  Documentation/networking/rxrpc.rst: rxrpc_kernel_send_data() "returns…

Pre-existing issues:
- [Low] Documentation/networking/rxrpc.rst says the msg passed to
  rxrpc_kernel_send_data() must not carry "any flags other than…

--

rxrpc: Fix lack of short-send handling in rxrpc_kernel_send_data()

This makes rxrpc_kernel_send_data() keep looping after a short send from
rxrpc_send_data() until the iterator is drained. It also drops the len
argument and uses the iterator count instead. On success the function now
returns 0 rather than the amount copied.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com

> diff --git a/Documentation/networking/rxrpc.rst b/Documentation/networking/rxrpc.rst
> index 8926dab8e2e60..01ad12fdf3058 100644
> --- a/Documentation/networking/rxrpc.rst
> +++ b/Documentation/networking/rxrpc.rst

[ ... ]

> @@ -880,13 +879,16 @@ The kernel interface functions are as follows:
>       MSG_MORE if there will be subsequent data sends for this call.
>  
>       The msg must not specify a destination address, control data or any flags
> -     other than MSG_MORE.  len is the total amount of data to transmit.
> +     other than MSG_MORE.

[Severity: Low]
This is a pre-existing issue, and this patch did not introduce it. Is the
"any flags other than MSG_MORE" restriction still correct? The in-tree
caller afs_make_call() passes MSG_WAITALL:

fs/afs/rxrpc.c:afs_make_call() {
    ...
	msg.msg_flags		= MSG_WAITALL | (call->write_iter ? MSG_MORE : 0);
    ...
}

rxrpc_send_data() also acts on both MSG_DONTWAIT and MSG_WAITALL:

net/rxrpc/sendmsg.c:rxrpc_send_data() {
    ...
	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
    ...
wait_for_space:
	ret = -EAGAIN;
	if (msg->msg_flags & MSG_DONTWAIT)
		goto maybe_error;
	...
	ret = rxrpc_wait_for_tx_window(rx, call, &timeo,
				       msg->msg_flags & MSG_WAITALL);
    ...
}

This sentence is already being edited here. Could it be changed to list
the flags that are actually honoured? The same wording is still present
at the end of the series.

>  
>       notify_end_rx can be NULL or it can be used to specify a function to be
>       called when the call changes state to end the Tx phase.  This function is
>       called with a spinlock held to prevent the last DATA packet from being
>       transmitted until the function returns.
>  
> +     It returns 0 if all the data is queued and a negative error code on
> +     failure.

[Severity: Low]
Is "queued" the right word here? Suppose MSG_MORE is set and the data
does not exactly fill a txbuf. In that case rxrpc_send_data() leaves the
partly filled txbuf in call->tx_pending and does not call
rxrpc_queue_packet():

net/rxrpc/sendmsg.c:rxrpc_send_data() {
    ...
		/* add the packet to the send queue if it's now full */
		if (!txb->space ||
		    (msg_data_left(msg) == 0 && !more)) {
			...
			rxrpc_queue_packet(rx, call, txb, notify_end_tx);
			call->tx_pending = NULL;
		}
    ...
}

At that point the iterator is empty, so the new loop in
rxrpc_kernel_send_data() returns 0:

		if (msg_data_left(msg) == 0) {
			ret = 0;
			break;
		}

Two examples of this path are rxperf_process_call() sending ZERO_PAGE
chunks, and afs_make_call() sending the request header before
write_iter.

So a return of 0 seems to mean the data was taken from the iterator and
buffered, not queued in the rxrpc_queue_packet() sense. A later patch in
the series, "rxrpc: Fix sendmsg length", uses the word "buffered" for
this same state in this document.

A negative return can also now follow a partial transfer. An earlier pass
of the loop may consume part of the iterator before a later pass fails.
Should the documentation say so as well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923133706.1496540-1-dhowells%40redhat.com