Re: [RFC PATCH] mm/filemap.c: fix the timing of asignment of prev_pos

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 17 Aug 2022 21:51:57 +0800 Guixin Liu <kanie@linux.alibaba.com> 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@linux.alibaba.com>
> 
> ---
> 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.