Re: [PATCH v4 28/33] netfs: Change the read result collector to only use one work item

[Nathan Chancellor <nathan@kernel.org>]

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);
>  }
>  
>  /*