On 10/06/2024 18:31, Yosry Ahmed wrote:
On Mon, Jun 10, 2024 at 7:31 AM Usama Arif <usamaarif642@xxxxxxxxx> wrote:
start/end writeback combination incorrectly increments NR_WRITTEN
counter, eventhough the pages aren't written to disk. Pages successfully
stored in zswap should just unlock folio and return from writepage.
Signed-off-by: Usama Arif <usamaarif642@xxxxxxxxx>
---
mm/page_io.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/mm/page_io.c b/mm/page_io.c
index a360857cf75d..501784d79977 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -196,9 +196,7 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
return ret;
}
if (zswap_store(folio)) {
- folio_start_writeback(folio);
folio_unlock(folio);
- folio_end_writeback(folio);
Removing these calls will have several effects, I am not really sure it's safe.
1. As you note in the commit log, NR_WRITTEN stats (and apparently
others) will no longer be updated. While this may make sense, it's a
user-visible change. I am not sure if anyone relies on this.
Thanks for the review.
This patch would correct NR_WRITTEN stat, so I think its a good thing?
But yeah as you said for people relying on that stat, suddenly this
number would be lowered if they pick up a kernel with this patch, not
sure how such changes would be dealt with in the kernel.
2. folio_end_writeback() calls folio_rotate_reclaimable() after
writeback completes to put a folio that has been marked with
PG_reclaim at the tail of the LRU, to be reclaimed first next time. Do
we get this call through other paths now?
We could add
if (folio_test_reclaim(folio)) {
folio_clear_reclaim(folio);
folio_rotate_reclaimable(folio);
}
to if zswap_store is successful to fix this?
3. If I remember correctly, there was some sort of state machine where
folios go from dirty to writeback to clean. I am not sure what happens
if we take the writeback phase out of the equation.
Overall, the change seems like it will special case the folios written
to zswap vs. to disk further, and we may end up missing important
things (like folio_rotate_reclaimable()). I would like to see a much
stronger argument for why it is safe and justified tbh :)
The patch came about from zero page swap optimization series
(https://lore.kernel.org/all/ZmcITDhdBzUGEHuY@xxxxxxxxxxxxxxxxxxxx/),
where Matthew pointed out that NR_WRITTEN would be wrong with the way I
was doing it.
Overall, I thought it would be good to have consistency with how
zeropages and zswap pages would be dealt with, and have a more correct
NR_WRITTEN stat.
In the next revision of the zero page patch, I will just add
folio_rotate_reclaimable after folio_unlock if folio is zero filled.
return 0;
}
if (!mem_cgroup_zswap_writeback_enabled(folio_memcg(folio))) {
--
2.43.0
