[bug report] netfs: Speed up buffered reading

[Dan Carpenter <dan.carpenter@linaro.org>]

Hello David Howells,

Commit 5c7822a3013d ("netfs: Speed up buffered reading") from Jul 2,
2024 (linux-next), leads to the following Smatch static checker
warning:

	fs/netfs/read_retry.c:152 netfs_retry_read_subrequests()
	error: we previously assumed 'rreq->netfs_ops->prepare_read' could be null (see line 51)

fs/netfs/read_retry.c
    33 static void netfs_retry_read_subrequests(struct netfs_io_request *rreq)
    34 {
    35         struct netfs_io_subrequest *subreq;
    36         struct netfs_io_stream *stream0 = &rreq->io_streams[0];
    37         LIST_HEAD(sublist);
    38         LIST_HEAD(queue);
    39 
    40         _enter("R=%x", rreq->debug_id);
    41 
    42         if (list_empty(&rreq->subrequests))
    43                 return;
    44 
    45         if (rreq->netfs_ops->retry_request)
    46                 rreq->netfs_ops->retry_request(rreq, NULL);
    47 
    48         /* If there's no renegotiation to do, just resend each retryable subreq
    49          * up to the first permanently failed one.
    50          */
    51         if (!rreq->netfs_ops->prepare_read &&
                                   ^^^^^^^^^^^^^^
Check for NULL

    52             !test_bit(NETFS_RREQ_COPY_TO_CACHE, &rreq->flags)) {
    53                 struct netfs_io_subrequest *subreq;
    54 
    55                 list_for_each_entry(subreq, &rreq->subrequests, rreq_link) {
    56                         if (test_bit(NETFS_SREQ_FAILED, &subreq->flags))
    57                                 break;
    58                         if (__test_and_clear_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags)) {
    59                                 netfs_reset_iter(subreq);
    60                                 netfs_reissue_read(rreq, subreq);
    61                         }
    62                 }
    63                 return;
    64         }
    65 
    66         /* Okay, we need to renegotiate all the download requests and flip any
    67          * failed cache reads over to being download requests and negotiate
    68          * those also.  All fully successful subreqs have been removed from the
    69          * list and any spare data from those has been donated.
    70          *
    71          * What we do is decant the list and rebuild it one subreq at a time so
    72          * that we don't end up with donations jumping over a gap we're busy
    73          * populating with smaller subrequests.  In the event that the subreq
    74          * we just launched finishes before we insert the next subreq, it'll
    75          * fill in rreq->prev_donated instead.
    76 
    77          * Note: Alternatively, we could split the tail subrequest right before
    78          * we reissue it and fix up the donations under lock.
    79          */
    80         list_splice_init(&rreq->subrequests, &queue);
    81 
    82         do {
    83                 struct netfs_io_subrequest *from;
    84                 struct iov_iter source;
    85                 unsigned long long start, len;
    86                 size_t part, deferred_next_donated = 0;
    87                 bool boundary = false;
    88 
    89                 /* Go through the subreqs and find the next span of contiguous
    90                  * buffer that we then rejig (cifs, for example, needs the
    91                  * rsize renegotiating) and reissue.
    92                  */
    93                 from = list_first_entry(&queue, struct netfs_io_subrequest, rreq_link);
    94                 list_move_tail(&from->rreq_link, &sublist);
    95                 start = from->start + from->transferred;
    96                 len   = from->len   - from->transferred;
    97 
    98                 _debug("from R=%08x[%x] s=%llx ctl=%zx/%zx/%zx",
    99                        rreq->debug_id, from->debug_index,
    100                        from->start, from->consumed, from->transferred, from->len);
    101 
    102                 if (test_bit(NETFS_SREQ_FAILED, &from->flags) ||
    103                     !test_bit(NETFS_SREQ_NEED_RETRY, &from->flags))
    104                         goto abandon;
    105 
    106                 deferred_next_donated = from->next_donated;
    107                 while ((subreq = list_first_entry_or_null(
    108                                 &queue, struct netfs_io_subrequest, rreq_link))) {
    109                         if (subreq->start != start + len ||
    110                             subreq->transferred > 0 ||
    111                             !test_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags))
    112                                 break;
    113                         list_move_tail(&subreq->rreq_link, &sublist);
    114                         len += subreq->len;
    115                         deferred_next_donated = subreq->next_donated;
    116                         if (test_bit(NETFS_SREQ_BOUNDARY, &subreq->flags))
    117                                 break;
    118                 }
    119 
    120                 _debug(" - range: %llx-%llx %llx", start, start + len - 1, len);
    121 
    122                 /* Determine the set of buffers we're going to use.  Each
    123                  * subreq gets a subset of a single overall contiguous buffer.
    124                  */
    125                 netfs_reset_iter(from);
    126                 source = from->io_iter;
    127                 source.count = len;
    128 
    129                 /* Work through the sublist. */
    130                 while ((subreq = list_first_entry_or_null(
    131                                 &sublist, struct netfs_io_subrequest, rreq_link))) {
    132                         list_del(&subreq->rreq_link);
    133 
    134                         subreq->source        = NETFS_DOWNLOAD_FROM_SERVER;
    135                         subreq->start        = start - subreq->transferred;
    136                         subreq->len        = len   + subreq->transferred;
    137                         stream0->sreq_max_len = subreq->len;
    138 
    139                         __clear_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
    140                         __set_bit(NETFS_SREQ_RETRYING, &subreq->flags);
    141 
    142                         spin_lock_bh(&rreq->lock);
    143                         list_add_tail(&subreq->rreq_link, &rreq->subrequests);
    144                         subreq->prev_donated += rreq->prev_donated;
    145                         rreq->prev_donated = 0;
    146                         trace_netfs_sreq(subreq, netfs_sreq_trace_retry);
    147                         spin_unlock_bh(&rreq->lock);
    148 
    149                         BUG_ON(!len);
    150 
    151                         /* Renegotiate max_len (rsize) */
--> 152                         if (rreq->netfs_ops->prepare_read(subreq) < 0) {
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Unchecked dereference

    153                                 trace_netfs_sreq(subreq, netfs_sreq_trace_reprep_failed);
    154                                 __set_bit(NETFS_SREQ_FAILED, &subreq->flags);
    155                         }
    156 
    157                         part = umin(len, stream0->sreq_max_len);
    158                         if (unlikely(rreq->io_streams[0].sreq_max_segs))

regards,
dan carpenter