On Fri, 2022-11-04 at 16:38 +0000, David Howells wrote:
> Fix the dodgy maths in netfs_rreq_unlock_folios(). start_page could be
> inside the folio, in which case the calculation of pgpos will be come up
> with a negative number (though for the moment rreq->start is rounded down
> earlier and folios would have to get merged whilst locked)
>
> Alter how this works to just frame the tracking in terms of absolute file
> positions, rather than offsets from the start of the I/O request. This
> simplifies the maths and makes it easier to follow.
>
> Fix the issue by using folio_pos() and folio_size() to calculate the end
> position of the page.
>
> Fixes: 3d3c95046742 ("netfs: Provide readahead and readpage netfs helpers")
> Reported-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> cc: Jeff Layton <jlayton@xxxxxxxxxx>
> cc: linux-cachefs@xxxxxxxxxx
> cc: linux-fsdevel@xxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/r/Y2SJw7w1IsIik3nb@xxxxxxxxxxxxxxxxxxxx/
> ---
>
> fs/netfs/buffered_read.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
> index baf668fb4315..7679a68e8193 100644
> --- a/fs/netfs/buffered_read.c
> +++ b/fs/netfs/buffered_read.c
> @@ -17,9 +17,9 @@ void netfs_rreq_unlock_folios(struct netfs_io_request *rreq)
> {
> struct netfs_io_subrequest *subreq;
> struct folio *folio;
> - unsigned int iopos, account = 0;
> pgoff_t start_page = rreq->start / PAGE_SIZE;
> pgoff_t last_page = ((rreq->start + rreq->len) / PAGE_SIZE) - 1;
> + size_t account = 0;
> bool subreq_failed = false;
>
> XA_STATE(xas, &rreq->mapping->i_pages, start_page);
> @@ -39,23 +39,23 @@ void netfs_rreq_unlock_folios(struct netfs_io_request *rreq)
> */
> subreq = list_first_entry(&rreq->subrequests,
> struct netfs_io_subrequest, rreq_link);
> - iopos = 0;
> subreq_failed = (subreq->error < 0);
>
> trace_netfs_rreq(rreq, netfs_rreq_trace_unlock);
>
> rcu_read_lock();
> xas_for_each(&xas, folio, last_page) {
> - unsigned int pgpos, pgend;
> + loff_t pg_end;
> bool pg_failed = false;
>
> if (xas_retry(&xas, folio))
> continue;
>
> - pgpos = (folio_index(folio) - start_page) * PAGE_SIZE;
> - pgend = pgpos + folio_size(folio);
> + pg_end = folio_pos(folio) + folio_size(folio) - 1;
>
> for (;;) {
> + loff_t sreq_end;
> +
> if (!subreq) {
> pg_failed = true;
> break;
> @@ -63,11 +63,11 @@ void netfs_rreq_unlock_folios(struct netfs_io_request *rreq)
> if (test_bit(NETFS_SREQ_COPY_TO_CACHE, &subreq->flags))
> folio_start_fscache(folio);
> pg_failed |= subreq_failed;
> - if (pgend < iopos + subreq->len)
> + sreq_end = subreq->start + subreq->len - 1;
> + if (pg_end < sreq_end)
> break;
>
> account += subreq->transferred;
> - iopos += subreq->len;
> if (!list_is_last(&subreq->rreq_link, &rreq->subrequests)) {
> subreq = list_next_entry(subreq, rreq_link);
> subreq_failed = (subreq->error < 0);
> @@ -75,7 +75,8 @@ void netfs_rreq_unlock_folios(struct netfs_io_request *rreq)
> subreq = NULL;
> subreq_failed = false;
> }
> - if (pgend == iopos)
> +
> + if (pg_end == sreq_end)
> break;
> }
>
>
>
> --
> Linux-cachefs mailing list
> Linux-cachefs@xxxxxxxxxx
> https://listman.redhat.com/mailman/listinfo/linux-cachefs
>
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
