Re: [Devel] [PATCH VZ10 v9 8/8] selftests/ve: Add mount accounting selftest

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

Sashiko review:

On 9/15/26 19:22, Vladimir Riabchun wrote:
> There are 6 test cases, covered in the new test:
> 1. Simple mount accouting correctness, just mount/umount.
> 2. Verification of correct limit hits and changes, including
>     negative values.
> 3. Partial mounts test, when mount limit is hit in the middle
>     of creation.
> 4. Test that enabled pseudosuper allows overuse.
> 5. Test that pseudosuper doesn't affect mount accounting.
> 6. Failcount feature verification.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-135520
> 
> Feature: per-ve failcounters
> Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> ---
>   tools/testing/selftests/ve/.gitignore         |   1 +
>   tools/testing/selftests/ve/Makefile           |   1 +
>   .../selftests/ve/ve_mount_accounting_test.c   | 550 ++++++++++++++++++
>   tools/testing/selftests/ve/ve_selftest.h      |  21 +
>   4 files changed, 573 insertions(+)
>   create mode 100644 tools/testing/selftests/ve/ve_mount_accounting_test.c
> 
> diff --git a/tools/testing/selftests/ve/.gitignore b/tools/testing/selftests/ve/.gitignore
> index afa4c568c2c9..3df4d05888dc 100644
> --- a/tools/testing/selftests/ve/.gitignore
> +++ b/tools/testing/selftests/ve/.gitignore
> @@ -1,2 +1,3 @@
>   ve_ns_owner_test
>   ve_perms_test
> +ve_mount_accounting_test
> diff --git a/tools/testing/selftests/ve/Makefile b/tools/testing/selftests/ve/Makefile
> index ec40cbc7b3a1..c6efe7c4b4fb 100644
> --- a/tools/testing/selftests/ve/Makefile
> +++ b/tools/testing/selftests/ve/Makefile
> @@ -4,5 +4,6 @@ CFLAGS += -g -Wall -O2
>   
>   TEST_GEN_PROGS += ve_ns_owner_test
>   TEST_GEN_PROGS += ve_perms_test
> +TEST_GEN_PROGS += ve_mount_accounting_test
>   
>   include ../lib.mk
> diff --git a/tools/testing/selftests/ve/ve_mount_accounting_test.c b/tools/testing/selftests/ve/ve_mount_accounting_test.c
> new file mode 100644
> index 000000000000..6ba13dcdcc87
> --- /dev/null
> +++ b/tools/testing/selftests/ve/ve_mount_accounting_test.c
> @@ -0,0 +1,550 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ve_mount_accounting selftests
> + *
> + * Tests to check the correctness of mount accounting.
> + */
> +#define _GNU_SOURCE
> +#include <asm/unistd.h>
> +#include <linux/sched.h>
> +#include <linux/limits.h>
> +#include <sys/wait.h>
> +#include <sys/syscall.h>
> +#include <sys/stat.h>
> +#include <sys/mount.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <sched.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <time.h>
> +
> +#include "../kselftest_harness.h"
> +#include "ve_selftest.h"
> +
> +#define TMP_DIR			"/ve-mnt-tmp/"
> +#define VE_MOUNTS_MAX		128
> +
> +/*
> + * These values take a while to calculate, the first test will
> + * do this and store the values for those who come after.
> + */
> +int *need_avail;
> +
> +FIXTURE(ve_mnt_acc)
> +{
> +	int cgv2_fd;
> +	int ctid;
> +	struct namespace_ctx ns_ctx;
> +	int orig_have_mnt;
> +};
> +
> +/*
> + * free_vfsmnt is called via call_rcu, so we need to ensure
> + * that all pending call_rcu tasks are finished.
> + * However, one barrier is not enough:
> + * mntput_no_expire
> + *   init_task_work(&mnt->mnt_rcu, __cleanup_mnt);
> + *   task_work_add(task, &mnt->mnt_rcu, TWA_RESUME)
> + *     OR
> + *   schedule_delayed_work(&delayed_mntput_work, 1)
> + *   # Both later call cleanup_mnt
> + *     cleanup_mnt
> + *       call_rcu(&mnt->mnt_rcu, delayed_free_vfsmnt);
> + *
> + * We can't enforce workqueue flush from userspace, so instead
> + * read several times until the value is the one we expect.
> + */

Sashiko says:
  This segment of the `wait_on_mnt_avail` function can potentially lead
  to an infinite wait if the value `mnt_avail_nr` does not match
  the expected value before timeout.
  While this isn't a bug introduced by this patch, could we include
  some mechanism  to handle timeouts more gracefully, such as an error code
  return or retry limit?

Something is off here.
1. There was no bug before this patch.
2. Retry and error return mechanism is what this code does.

> +static int wait_on_mnt_avail(FIXTURE_DATA(ve_mnt_acc) * self, int expected)
> +{
> +#define WAIT_TIMEOUT_NS		(4096UL * 1000 * 1000)
> +	char path_avail[64];
> +	int mnt_avail_nr = 0;
> +	struct timespec delay = { .tv_nsec = 1000000 };
> +
> +	snprintf(path_avail, sizeof(path_avail), "%d/ve.mnt_avail_nr", self->ctid);
> +
> +	while (delay.tv_nsec <= WAIT_TIMEOUT_NS) {
> +		if (read_s32_at(self->cgv2_fd, path_avail, &mnt_avail_nr)) {
> +			fprintf(stderr, "Failed to read from %s\n", path_avail);
> +			return 1;
> +		}
> +		if (mnt_avail_nr == expected)
> +			return 0;
> +
> +		do_rcu_barrier();
> +		if (nanosleep(&delay, NULL)) {
> +			fprintf(stderr, "nanosleep failed: %s\n", strerror(errno));
> +			return 1;
> +		}
> +		delay.tv_nsec *= 2;
> +	}
> +	fprintf(stderr, "Didn't get expected value at %s\n", path_avail);
> +	fprintf(stderr, "Expected %d, last read %d\n", expected, mnt_avail_nr);
> +	return -1;
> +}
> +
> +static int set_pseudosuper(FIXTURE_DATA(ve_mnt_acc) * self, int value)
> +{
> +	char path[64];
> +
> +	snprintf(path, sizeof(path), "%d/ve.pseudosuper", self->ctid);
> +	return write_u64_at(self->cgv2_fd, path, value);
> +}
> +

Sashiko says:
  In `_create_mount`, can the clean-up logic be more robust to ensure
  that filesystem resources are consistently freed in the event of an error
  or unexpected behavior?

No more cleanup here is possible.

> +static int _create_mount(void *id_ptr)
> +{
> +	char path[PATH_MAX];
> +	int id = *(int *)id_ptr, ret;
> +
> +	snprintf(path, sizeof(path), TMP_DIR "%d", id);
> +
> +	if (mkdir(path, 0755) < 0) {
> +		fprintf(stderr, "Failed to create directory %s: %s\n", path, strerror(errno));
> +		return -1;
> +	}
> +	ret = mount("tmpfs", path, "tmpfs", 0, "size=1M");
> +	if (!ret)
> +		return 0;
> +	fprintf(stderr, "Failed to mount tmpfs to %s: %s\n", path, strerror(errno));
> +
> +	rmdir(path);
> +	return -1;
> +}
> +
> +/* Create a mount and wait until mount counter is valid */
> +static int create_mount(FIXTURE_DATA(ve_mnt_acc) * self, int id)
> +{
> +	int ret, avail_before;
> +	char path[64];
> +
> +	snprintf(path, sizeof(path), "%d/ve.mnt_avail_nr", self->ctid);
> +
> +	if (read_s32_at(self->cgv2_fd, path, &avail_before)) {
> +		fprintf(stderr, "Failed to read mnt_avail_nr in create_mount\n");
> +		return 1;
> +	}
> +

Sashiko says:
  Would it be beneficial to include comprehensive error handling in `create_mount`
  to cover cases when `run_in_ve` fails?

No, it wouldn't. Nothing we can do in this case and nothing
needs to be handled - no resources created.

> +	ret = run_in_ve(self->cgv2_fd, self->ctid, &self->ns_ctx,
> +		CLONE_NEWVE | CLONE_NEWNS, _create_mount, &id);
> +
> +	if (ret)
> +		wait_on_mnt_avail(self, avail_before);
> +	else
> +		ret = wait_on_mnt_avail(self, avail_before - 1);
> +	return ret;
> +}
> +

Sashiko says:
  Does the `_destroy_mount` function correctly handle subsequent errors that may
  occur during the unmount and directory removal stages?

Yes, it does.

> +static int _destroy_mount(void *id_ptr)
> +{
> +	char path[PATH_MAX];
> +	struct stat st;
> +	int id = *(int *)id_ptr;
> +
> +	snprintf(path, sizeof(path), TMP_DIR "%d", id);
> +
> +	if (stat(path, &st))
> +		return 1;
> +	if (umount(path)) {
> +		fprintf(stderr, "failed to umount directory %s: %s\n", path, strerror(errno));
> +		return -1;
> +	}
> +	if (rmdir(path)) {
> +		fprintf(stderr, "failed to remove directory %s: %s\n", path, strerror(errno));
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> ...
> +
> +int main(int argc, char **argv)
> +{
> +	need_avail = mmap(NULL, sizeof(int),
> +		PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> +
> +	if (need_avail == MAP_FAILED) {
> +		fprintf(stderr, "Failed to mmap shared region: %s\n", strerror(errno));
> +		return 1;
> +	}
> +	*need_avail = -1;
> +
> +	return test_harness_run(argc, argv);

Sashiko says:
  Does the `main` function properly handle the memory allocated by `mmap`,
  especially considering that there might not be a corresponding `munmap` to free
  the memory?

Doesn't matter, we can free need_avail only when test_harness_run finishes.
It is easier to just exit.

> +}
> diff --git a/tools/testing/selftests/ve/ve_selftest.h b/tools/testing/selftests/ve/ve_selftest.h
> index bf9b89b59a6c..8cc9b3238aaf 100644
> --- a/tools/testing/selftests/ve/ve_selftest.h
> +++ b/tools/testing/selftests/ve/ve_selftest.h
> @@ -413,4 +413,25 @@ static inline int is_mounted(const char *path)
>   	return errno == EXDEV ? 1 : -1;
>   }
>   

Sashiko says:
  Could write operations within `do_rcu_barrier` be improperly assumed to be atomic
  leading to potential scheduling issues when invoked in contexts that might sleep?

No, they couldn't. This is user-space code.

> +static inline int do_rcu_barrier(void)
> +{
> +	int fd;
> +
> +	errno = 0;
> +	fd = open("/sys/module/rcutree/parameters/do_rcu_barrier", O_WRONLY);
> +
> +	if (fd < 0) {
> +		fprintf(stderr, "Failed to open do_rcu_barrier: %s\n", strerror(errno));
> +		return -1;
> +	}
> +	errno = 0;
> +	if (write(fd, "1", 2) != 2) {
> +		fprintf(stderr, "Failed to issue rcu_barrier: %s\n", strerror(errno));
> +		close(fd);
> +		return -1;
> +	}
> +	close(fd);
> +	return 0;
> +}
> +
>   #endif /* __SELFTESTS_VE_VE_SELFTEST_H */

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel