Hi Harry,
On Mon, 14 Sep 2026 21:36:39 +0100 "Harry Yoo (Meta)" <[email protected]> wrote:
> The zswap_writeback_enabled test fails when a write to memory.reclaim
> returns -EAGAIN, which means less than the requested amount was
> reclaimed. attempt_writeback() propagates the -EAGAIN to the caller,
> and the test case is marked as failed even when zswap writeback did
> happen.
>
> This heavily depends on the performance of the backing swap device.
> Reclaim does not wait for writeback (on cgroup v2), does not count pages
> that are under writeback as reclaimed, and memory.reclaim gives up after
> MAX_RECLAIM_RETRIES passes without making progress. On a slow device
> where reclaim does not make any progress before writeback completes,
> a write to memory.reclaim fails.
>
> On a VM with zswap enabled, where IO delay was injected via dm-delay,
> the success rate of the zswap writeback test drops dramatically once
> the delay reaches 11 ms: 7% failures at 10 ms and 79% failures at 11 ms,
> n = 100.
>
> When zswap writeback is enabled, ignore -EAGAIN from memory.reclaim and
> determine pass/fail based on the zswpwb counter because that is what
> zswap_writeback_enabled actually wants to test.
>
> With this change, the test reliably passes even on a slow swap device
> (tested up to 1000 ms delay). This makes the test resilient against
> the performance of the swap device.
Makes sense to me.
[...]
> --- a/tools/testing/selftests/cgroup/test_zswap.c
> +++ b/tools/testing/selftests/cgroup/test_zswap.c
> @@ -346,7 +346,16 @@ static int attempt_writeback(const char *cgroup, void
> *arg)
> * it can't writeback to swap.
> */
> ret = cg_write_numeric(cgroup, "memory.reclaim", memsize);
> - if (!wb_enabled)
> +
> + /*
> + * When writeback is enabled, memory.reclaim may still fail to reclaim
> + * the requested amount of memory due to a slow swap device.
> + * Ignore -EAGAIN here. The caller determines pass/fail based on the
> + * zswap writeback counter.
> + */
> + if (wb_enabled && ret == -EAGAIN)
> + ret = 0;
> + else if (!wb_enabled)
> ret = (ret == -EAGAIN) ? 0 : -1;
My humble eyes were unable to easily understand the change. Is the change
effectively same to below, and if so, would this be easier to read?
'''
@@ -344,10 +344,11 @@ static int attempt_writeback(const char *cgroup, void
*arg)
* writeback as zswap.max is 1/4 of what was needed when reclaim ran
the first time.
* If writeback is disabled, memory reclaim will fail as zswap is
limited and
* it can't writeback to swap.
+ * Even if writeback is enabled, it could return -EAGAIN due to a slow
+ * swap device.
*/
ret = cg_write_numeric(cgroup, "memory.reclaim", memsize);
- if (!wb_enabled)
- ret = (ret == -EAGAIN) ? 0 : -1;
+ ret = (ret == -EAGAIN) ? 0 : -1;
out:
free(mem);
'''
Someone might hate the second 'ret' assignment. But I was unable to make it
look cleaner without introducing a >80 column line. The file already has
multiple >80 column lines and I don't really mind having a new long line,
though.
Thanks,
SJ
[...]
