mm/filemap.c: fix the timing of asignment of prev_pos

RFC PATCH In review Guixin Liu 3 participants 5 messages Aug 17, 2022 – Aug 18, 2022

Subsystems (3)

Memory management VFS Page cache

lore

Share & tools

lore

Guixin Liu

author

2022-08-17 13:52 UTC #0 lore raw

RFC PATCH 1/1 mm/filemap.c: fix the timing of asignment of prev_pos

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 <[email protected]>

---
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?

 mm/filemap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/filemap.c +1 -1

index 660490c..68fd987 100644
@@ -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;

Signature · 1 line

1.8.3.1

Andrew Morton

maintainer

2022-08-17 15:17 UTC #1 lore raw

Quoted 7 lines · On Wed, 17 Aug 2022 21:51:57 +0800 Guixin Liu <[email protected]> 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 <[email protected]>

Quoted 8 lines

---
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.

mm/filemap.c ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter, · line 2710

Quoted 12 lines

mm/filemap.c +1 -1

@@ -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.

Matthew Wilcox

maintainer

2022-08-17 15:31 UTC #2 lore raw

Quoted 18 lines · Andrew Morton

Quoted 7 lines · On Wed, 17 Aug 2022 21:51:57 +0800 Guixin Liu <[email protected]> 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 <[email protected]>
Quoted 8 lines
---
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?

Quoted 2 lines

It looks wrong to me and it does appear that 06c0444290cecf0 did this unintentionally.

That commit was the start of a problem, but I think I restored the original behaviour in 5ccc944dce3d. You were part of that discussion back in June:
https://lore.kernel.org/linux-mm/[email protected]/

Matthew Wilcox

maintainer

2022-08-17 15:25 UTC #3 lore raw

In reply to Guixin Liu

Quoted 7 lines · Guixin Liu

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 <[email protected]>

Quoted 8 lines

---
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.

Quoted 20 lines

 mm/filemap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/filemap.c +1 -1

index 660490c..68fd987 100644
@@ -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;

Signature · 1 line

1.8.3.1

Guixin Liu

author

2022-08-18 03:13 UTC #4 lore raw

Quoted 56 lines · Matthew Wilcox

Quoted 7 lines · Guixin Liu
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 <[email protected]>
Quoted 8 lines
---
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);

Quoted 7 lines

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.

My thanks for your detailed description, I am wrong about this, I test not on the newest code, My fault.

The 5ccc944dce3d actually solved this problem.

Best regards,

Guixin Liu

Quoted 20 lines

  mm/filemap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

mm/filemap.c +1 -1

index 660490c..68fd987 100644
@@ -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;

Signature · 1 line

1.8.3.1