Hi David,
On Fri, Nov 08, 2024 at 05:32:29PM +0000, David Howells wrote:
...
> diff --git a/fs/netfs/read_retry.c b/fs/netfs/read_retry.c
> index 264f3cb6a7dc..8ca0558570c1 100644
> --- a/fs/netfs/read_retry.c
> +++ b/fs/netfs/read_retry.c
> @@ -12,15 +12,8 @@
> static void netfs_reissue_read(struct netfs_io_request *rreq,
> struct netfs_io_subrequest *subreq)
> {
> - struct iov_iter *io_iter = &subreq->io_iter;
> -
> - if (iov_iter_is_folioq(io_iter)) {
> - subreq->curr_folioq = (struct folio_queue *)io_iter->folioq;
> - subreq->curr_folioq_slot = io_iter->folioq_slot;
> - subreq->curr_folio_order = subreq->curr_folioq->orders[subreq->curr_folioq_slot];
> - }
> -
> - atomic_inc(&rreq->nr_outstanding);
> + __clear_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags);
> + __set_bit(NETFS_SREQ_RETRYING, &subreq->flags);
> __set_bit(NETFS_SREQ_IN_PROGRESS, &subreq->flags);
> netfs_get_subrequest(subreq, netfs_sreq_trace_get_resubmit);
> subreq->rreq->netfs_ops->issue_read(subreq);
> @@ -33,13 +26,12 @@ static void netfs_reissue_read(struct netfs_io_request *rreq,
> static void netfs_retry_read_subrequests(struct netfs_io_request *rreq)
> {
> struct netfs_io_subrequest *subreq;
> - struct netfs_io_stream *stream0 = &rreq->io_streams[0];
> - LIST_HEAD(sublist);
> - LIST_HEAD(queue);
> + struct netfs_io_stream *stream = &rreq->io_streams[0];
> + struct list_head *next;
>
> _enter("R=%x", rreq->debug_id);
>
> - if (list_empty(&rreq->subrequests))
> + if (list_empty(&stream->subrequests))
> return;
>
> if (rreq->netfs_ops->retry_request)
> @@ -52,7 +44,7 @@ static void netfs_retry_read_subrequests(struct netfs_io_request *rreq)
> !test_bit(NETFS_RREQ_COPY_TO_CACHE, &rreq->flags)) {
> struct netfs_io_subrequest *subreq;
>
> - list_for_each_entry(subreq, &rreq->subrequests, rreq_link) {
> + list_for_each_entry(subreq, &stream->subrequests, rreq_link) {
> if (test_bit(NETFS_SREQ_FAILED, &subreq->flags))
> break;
> if (__test_and_clear_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags)) {
> @@ -73,48 +65,44 @@ static void netfs_retry_read_subrequests(struct netfs_io_request *rreq)
> * populating with smaller subrequests. In the event that the subreq
> * we just launched finishes before we insert the next subreq, it'll
> * fill in rreq->prev_donated instead.
> -
> + *
> * Note: Alternatively, we could split the tail subrequest right before
> * we reissue it and fix up the donations under lock.
> */
> - list_splice_init(&rreq->subrequests, &queue);
> + next = stream->subrequests.next;
>
> do {
> - struct netfs_io_subrequest *from;
> + struct netfs_io_subrequest *subreq = NULL, *from, *to, *tmp;
> struct iov_iter source;
> unsigned long long start, len;
> - size_t part, deferred_next_donated = 0;
> + size_t part;
> bool boundary = false;
>
> /* Go through the subreqs and find the next span of contiguous
> * buffer that we then rejig (cifs, for example, needs the
> * rsize renegotiating) and reissue.
> */
> - from = list_first_entry(&queue, struct netfs_io_subrequest, rreq_link);
> - list_move_tail(&from->rreq_link, &sublist);
> + from = list_entry(next, struct netfs_io_subrequest, rreq_link);
> + to = from;
> start = from->start + from->transferred;
> len = from->len - from->transferred;
>
> - _debug("from R=%08x[%x] s=%llx ctl=%zx/%zx/%zx",
> + _debug("from R=%08x[%x] s=%llx ctl=%zx/%zx",
> rreq->debug_id, from->debug_index,
> - from->start, from->consumed, from->transferred, from->len);
> + from->start, from->transferred, from->len);
>
> if (test_bit(NETFS_SREQ_FAILED, &from->flags) ||
> !test_bit(NETFS_SREQ_NEED_RETRY, &from->flags))
> goto abandon;
>
> - deferred_next_donated = from->next_donated;
> - while ((subreq = list_first_entry_or_null(
> - &queue, struct netfs_io_subrequest, rreq_link))) {
> - if (subreq->start != start + len ||
> - subreq->transferred > 0 ||
> + list_for_each_continue(next, &stream->subrequests) {
> + subreq = list_entry(next, struct netfs_io_subrequest, rreq_link);
> + if (subreq->start + subreq->transferred != start + len ||
> + test_bit(NETFS_SREQ_BOUNDARY, &subreq->flags) ||
> !test_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags))
> break;
> - list_move_tail(&subreq->rreq_link, &sublist);
> - len += subreq->len;
> - deferred_next_donated = subreq->next_donated;
> - if (test_bit(NETFS_SREQ_BOUNDARY, &subreq->flags))
> - break;
> + to = subreq;
> + len += to->len;
> }
>
> _debug(" - range: %llx-%llx %llx", start, start + len - 1, len);
> @@ -127,36 +115,28 @@ static void netfs_retry_read_subrequests(struct netfs_io_request *rreq)
> source.count = len;
>
> /* Work through the sublist. */
> - while ((subreq = list_first_entry_or_null(
> - &sublist, struct netfs_io_subrequest, rreq_link))) {
> - list_del(&subreq->rreq_link);
> -
> + subreq = from;
> + list_for_each_entry_from(subreq, &stream->subrequests, rreq_link) {
> + if (!len)
> + break;
> subreq->source = NETFS_DOWNLOAD_FROM_SERVER;
> subreq->start = start - subreq->transferred;
> subreq->len = len + subreq->transferred;
> - stream0->sreq_max_len = subreq->len;
> -
> __clear_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
> __set_bit(NETFS_SREQ_RETRYING, &subreq->flags);
> -
> - spin_lock(&rreq->lock);
> - list_add_tail(&subreq->rreq_link, &rreq->subrequests);
> - subreq->prev_donated += rreq->prev_donated;
> - rreq->prev_donated = 0;
> trace_netfs_sreq(subreq, netfs_sreq_trace_retry);
> - spin_unlock(&rreq->lock);
> -
> - BUG_ON(!len);
>
> /* Renegotiate max_len (rsize) */
> + stream->sreq_max_len = subreq->len;
> if (rreq->netfs_ops->prepare_read(subreq) < 0) {
> trace_netfs_sreq(subreq, netfs_sreq_trace_reprep_failed);
> __set_bit(NETFS_SREQ_FAILED, &subreq->flags);
> + goto abandon;
> }
>
> - part = umin(len, stream0->sreq_max_len);
> - if (unlikely(rreq->io_streams[0].sreq_max_segs))
> - part = netfs_limit_iter(&source, 0, part, stream0->sreq_max_segs);
> + part = umin(len, stream->sreq_max_len);
> + if (unlikely(stream->sreq_max_segs))
> + part = netfs_limit_iter(&source, 0, part, stream->sreq_max_segs);
> subreq->len = subreq->transferred + part;
> subreq->io_iter = source;
> iov_iter_truncate(&subreq->io_iter, part);
> @@ -166,58 +146,106 @@ static void netfs_retry_read_subrequests(struct netfs_io_request *rreq)
> if (!len) {
> if (boundary)
> __set_bit(NETFS_SREQ_BOUNDARY, &subreq->flags);
> - subreq->next_donated = deferred_next_donated;
> } else {
> __clear_bit(NETFS_SREQ_BOUNDARY, &subreq->flags);
> - subreq->next_donated = 0;
> }
>
> + netfs_get_subrequest(subreq, netfs_sreq_trace_get_resubmit);
> netfs_reissue_read(rreq, subreq);
> - if (!len)
> + if (subreq == to)
> break;
> -
> - /* If we ran out of subrequests, allocate another. */
> - if (list_empty(&sublist)) {
> - subreq = netfs_alloc_subrequest(rreq);
> - if (!subreq)
> - goto abandon;
> - subreq->source = NETFS_DOWNLOAD_FROM_SERVER;
> - subreq->start = start;
> -
> - /* We get two refs, but need just one. */
> - netfs_put_subrequest(subreq, false, netfs_sreq_trace_new);
> - trace_netfs_sreq(subreq, netfs_sreq_trace_split);
> - list_add_tail(&subreq->rreq_link, &sublist);
> - }
> }
>
> /* If we managed to use fewer subreqs, we can discard the
> - * excess.
> + * excess; if we used the same number, then we're done.
> */
> - while ((subreq = list_first_entry_or_null(
> - &sublist, struct netfs_io_subrequest, rreq_link))) {
> - trace_netfs_sreq(subreq, netfs_sreq_trace_discard);
> - list_del(&subreq->rreq_link);
> - netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_done);
> + if (!len) {
> + if (subreq == to)
> + continue;
> + list_for_each_entry_safe_from(subreq, tmp,
> + &stream->subrequests, rreq_link) {
> + trace_netfs_sreq(subreq, netfs_sreq_trace_discard);
> + list_del(&subreq->rreq_link);
> + netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_done);
> + if (subreq == to)
> + break;
> + }
> + continue;
> }
>
> - } while (!list_empty(&queue));
> + /* We ran out of subrequests, so we need to allocate some more
> + * and insert them after.
> + */
> + do {
> + subreq = netfs_alloc_subrequest(rreq);
> + if (!subreq) {
> + subreq = to;
> + goto abandon_after;
> + }
> + subreq->source = NETFS_DOWNLOAD_FROM_SERVER;
> + subreq->start = start;
> + subreq->len = len;
> + subreq->debug_index = atomic_inc_return(&rreq->subreq_counter);
> + subreq->stream_nr = stream->stream_nr;
> + __set_bit(NETFS_SREQ_RETRYING, &subreq->flags);
> +
> + trace_netfs_sreq_ref(rreq->debug_id, subreq->debug_index,
> + refcount_read(&subreq->ref),
> + netfs_sreq_trace_new);
> + netfs_get_subrequest(subreq, netfs_sreq_trace_get_resubmit);
> +
> + list_add(&subreq->rreq_link, &to->rreq_link);
> + to = list_next_entry(to, rreq_link);
> + trace_netfs_sreq(subreq, netfs_sreq_trace_retry);
> +
> + stream->sreq_max_len = umin(len, rreq->rsize);
> + stream->sreq_max_segs = 0;
> + if (unlikely(stream->sreq_max_segs))
> + part = netfs_limit_iter(&source, 0, part, stream->sreq_max_segs);
> +
> + netfs_stat(&netfs_n_rh_download);
> + if (rreq->netfs_ops->prepare_read(subreq) < 0) {
> + trace_netfs_sreq(subreq, netfs_sreq_trace_reprep_failed);
> + __set_bit(NETFS_SREQ_FAILED, &subreq->flags);
> + goto abandon;
> + }
> +
> + part = umin(len, stream->sreq_max_len);
> + subreq->len = subreq->transferred + part;
> + subreq->io_iter = source;
> + iov_iter_truncate(&subreq->io_iter, part);
> + iov_iter_advance(&source, part);
> +
> + len -= part;
> + start += part;
> + if (!len && boundary) {
> + __set_bit(NETFS_SREQ_BOUNDARY, &to->flags);
> + boundary = false;
> + }
> +
> + netfs_reissue_read(rreq, subreq);
> + } while (len);
> +
> + } while (!list_is_head(next, &stream->subrequests));
>
> return;
>
> - /* If we hit ENOMEM, fail all remaining subrequests */
> + /* If we hit an error, fail all remaining incomplete subrequests */
> +abandon_after:
> + if (list_is_last(&subreq->rreq_link, &stream->subrequests))
> + return;
This change as commit 1bd9011ee163 ("netfs: Change the read result
collector to only use one work item") in next-20241114 causes a clang
warning:
fs/netfs/read_retry.c:235:20: error: variable 'subreq' is uninitialized when used here [-Werror,-Wuninitialized]
235 | if (list_is_last(&subreq->rreq_link, &stream->subrequests))
| ^~~~~~
fs/netfs/read_retry.c:28:36: note: initialize the variable 'subreq' to silence this warning
28 | struct netfs_io_subrequest *subreq;
| ^
| = NULL
May be a shadowing issue, as adding KCFLAGS=-Wshadow shows:
fs/netfs/read_retry.c:75:31: error: declaration shadows a local variable [-Werror,-Wshadow]
75 | struct netfs_io_subrequest *subreq = NULL, *from, *to, *tmp;
| ^
fs/netfs/read_retry.c:28:30: note: previous declaration is here
28 | struct netfs_io_subrequest *subreq;
| ^
Cheers,
Nathan
> + subreq = list_next_entry(subreq, rreq_link);
> abandon:
> - list_splice_init(&sublist, &queue);
> - list_for_each_entry(subreq, &queue, rreq_link) {
> - if (!subreq->error)
> - subreq->error = -ENOMEM;
> - __clear_bit(NETFS_SREQ_FAILED, &subreq->flags);
> + list_for_each_entry_from(subreq, &stream->subrequests, rreq_link) {
> + if (!subreq->error &&
> + !test_bit(NETFS_SREQ_FAILED, &subreq->flags) &&
> + !test_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags))
> + continue;
> + subreq->error = -ENOMEM;
> + __set_bit(NETFS_SREQ_FAILED, &subreq->flags);
> __clear_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
> __clear_bit(NETFS_SREQ_RETRYING, &subreq->flags);
> }
> - spin_lock(&rreq->lock);
> - list_splice_tail_init(&queue, &rreq->subrequests);
> - spin_unlock(&rreq->lock);
> }
>
> /*
> @@ -225,14 +253,19 @@ static void netfs_retry_read_subrequests(struct netfs_io_request *rreq)
> */
> void netfs_retry_reads(struct netfs_io_request *rreq)
> {
> - trace_netfs_rreq(rreq, netfs_rreq_trace_resubmit);
> + struct netfs_io_subrequest *subreq;
> + struct netfs_io_stream *stream = &rreq->io_streams[0];
>
> - atomic_inc(&rreq->nr_outstanding);
> + /* Wait for all outstanding I/O to quiesce before performing retries as
> + * we may need to renegotiate the I/O sizes.
> + */
> + list_for_each_entry(subreq, &stream->subrequests, rreq_link) {
> + wait_on_bit(&subreq->flags, NETFS_SREQ_IN_PROGRESS,
> + TASK_UNINTERRUPTIBLE);
> + }
>
> + trace_netfs_rreq(rreq, netfs_rreq_trace_resubmit);
> netfs_retry_read_subrequests(rreq);
> -
> - if (atomic_dec_and_test(&rreq->nr_outstanding))
> - netfs_rreq_terminated(rreq);
> }
>
> /*
