On Wed, 17 Aug 2022 21:51:57 +0800 Guixin Liu <kanie@xxxxxxxxxxxxxxxxx> wrote:
> The prev_pos should be assigned before the iocb->ki_pos is incremented,
> so that the prev_pos is the exact location of the last visit.
>
> Fixes: 06c0444290cec ("mm/filemap.c: generic_file_buffered_read() now
> uses find_get_pages_contig")
> Signed-off-by: Guixin Liu <kanie@xxxxxxxxxxxxxxxxx>
>
> ---
> Hi guys,
> When I`m running repetitive 4k read io which has same offset,
> I find that access to folio_mark_accessed is inevitable in the
> read process, the reason is that the prev_pos is assigned after the
> iocb->ki_pos is incremented, so that the prev_pos is always not equal
> to the position currently visited.
> Is this a bug that needs fixing?
It looks wrong to me and it does appear that 06c0444290cecf0 did this
unintentionally.
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2703,8 +2703,8 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
> copied = copy_folio_to_iter(folio, offset, bytes, iter);
>
> already_read += copied;
> - iocb->ki_pos += copied;
> ra->prev_pos = iocb->ki_pos;
> + iocb->ki_pos += copied;
>
> if (copied < bytes) {
> error = -EFAULT;
So we significantly messed up pagecache page aging and nobody noticed
for nearly two years. What does this tell us :(
I'd be interested if anyone can demonstrate runtime effects from this
change. If yes then I'll add cc:stable. If no then I'll ask why we
even bothered.
