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

[Matthew Wilcox <willy@infradead.org>]

On Wed, Aug 17, 2022 at 09:51:57PM +0800, Guixin Liu 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?

I think you've misunderstood the purpose of 'prev_pos'.  But this has
been the source of bugs, so let's go through it in detail.

In general, we want to mark a folio as accessed each time we read from
it.  So if we do this:

	read(fd, buf, 1024 * 1024);

we want to mark each folio as having been accessed.

But if we're doing lots of short reads, we don't want to mark a folio as
being accessed multiple times (if you dive into the implementation,
you'll see the first time, the 'referenced' flag is set and the second
time, the folio is moved to the active list, so it matters how often
we call mark_accessed).  IOW:

	for (i = 0; i < 1024 * 1024; i++)
		read(fd, buf, 1);

should do the same amount of accessed/referenced/activation as the single
read above.

So when we store ki_pos in prev_pos, we don't want to know "Where did
the previous read start?"  We want to know "Where did the previous read
end".  That's why when we test it, we check whether prev_pos - 1 is in
the same folio as the offset we're looking at:

                if (!pos_same_folio(iocb->ki_pos, ra->prev_pos - 1,
                                                        fbatch.folios[0]))
                        folio_mark_accessed(fbatch.folios[0]);

I'm not super-proud of this code, and accept that it's confusing.
But I don't think the patch below is right.  If you could share
your actual test and show what's going wrong, I'm interested.

I think what you're saying is that this loop:

	for (i = 0; i < 1000; i++)
		pread(fd, buf, 4096, 1024 * 1024);

results in the folio at offset 1MB being marked as accessed more than
once.  If so, then I think that's the algorithm behaving as designed.
Whether that's desirable is a different question; when I touched this
code last, I was trying to restore the previous behaviour which was
inadvertently broken.  I'm not taking a position on what the right
behaviour is for such code.

>  mm/filemap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 660490c..68fd987 100644
> --- 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;
> -- 
> 1.8.3.1
>