Re: [Devel] [PATCH VZ10 v9 3/8] selftests/ve: Update ve_ns_owner_test

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

Sashiko review:

On 9/15/26 19:22, Vladimir Riabchun wrote:
> Mount accounting interface has changed, now it shows the number
> of available mounts.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-135520
> 
> Feature: per-ve failcounters
> Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> ---
>   tools/testing/selftests/ve/ve_ns_owner_test.c | 33 ++++++++++---------
>   1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/testing/selftests/ve/ve_ns_owner_test.c b/tools/testing/selftests/ve/ve_ns_owner_test.c
> index 82c31dff4b1b..c3fa7d7b7f9c 100644
> --- a/tools/testing/selftests/ve/ve_ns_owner_test.c
> +++ b/tools/testing/selftests/ve/ve_ns_owner_test.c
> @@ -18,7 +18,7 @@
>    * with a new netns and mntns, the parent reads the new ve's counters
>    * via cgroupfs and asserts they reflect the just-created namespaces:
>    *   - ve.netns_avail_nr drops by exactly one (the new netns);
> - *   - ve.mnt_nr is strictly greater than zero (mounts copied into the
> + *   - ve.mnt_avail_nr drops by some value >= 1 (mounts copied into the
>    *     new mntns are accounted to the new ve).
>    *
>    * We never assert against the parent ve's counters: those are shared
> @@ -50,6 +50,7 @@
>    * any spurious accounting against the parent ve would overflow it.
>    */
>   #define VE_NETNS_MAX		3
> +#define VE_MOUNTS_MAX		4096
>   
>   /*
>    * Synchronisation across the clone() boundary: child does its setup,
> @@ -106,7 +107,7 @@ static int clone_child_func(void *arg)
>   /*
>    * Before fix:
>    *   - clone path: ve.netns_avail_nr stays at VE_NETNS_MAX and
> - *     ve.mnt_nr stays at 0 because copy_net_ns()/copy_mnt_ns()
> + *     ve.mnt_avail_nr stays at VE_MOUNTS_MAX because copy_net_ns()/copy_mnt_ns()
>    *     charged the parent ve via get_exec_env().
>    *   - unshare path: the syscall itself returned -EINVAL, so this
>    *     check was unreachable.
> @@ -117,16 +118,16 @@ static int clone_child_func(void *arg)
>   static void check_new_ve_owner(struct __test_metadata *_metadata,
>   			       int cgv2_fd, int ctid)
>   {
> -	unsigned long long avail, mnt;
> -	char path[PATH_MAX];
> +	unsigned long long avail_netns, avail_mnt;
> +	char path[64];
>   
>   	snprintf(path, sizeof(path), "%d/ve.netns_avail_nr", ctid);
> -	ASSERT_EQ(read_u64_at(cgv2_fd, path, &avail), 0);
> -	EXPECT_EQ(avail, VE_NETNS_MAX - 1);
> +	ASSERT_EQ(read_u64_at(cgv2_fd, path, &avail_netns), 0);
> +	EXPECT_EQ(avail_netns, VE_NETNS_MAX - 1);
>   
> -	snprintf(path, sizeof(path), "%d/ve.mnt_nr", ctid);
> -	ASSERT_EQ(read_u64_at(cgv2_fd, path, &mnt), 0);
> -	EXPECT_GT(mnt, 0);
> +	snprintf(path, sizeof(path), "%d/ve.mnt_avail_nr", ctid);
> +	ASSERT_EQ(read_u64_at(cgv2_fd, path, &avail_mnt), 0);
> +	EXPECT_LT(avail_mnt, VE_MOUNTS_MAX);

Sashiko says:
  This is a pre-existing issue, but the logic in check_new_ve_owner to verify that
  've.mnt_avail_nr' strictly decreases seems unsound, potentially causing
  intermittent test failures.
  Could the test environment allow for external factors that might not ensure
  this decrement happens logically?
  Is there a possibility that concurrent operations could influence the
  've.mnt_avail_nr' in a way that disrupts this test's reliability?

No, this is our namespace and nobody should touch it. If value is different, a bug
is somewhere.

>   }
>   
>   FIXTURE(ve_ns_owner)
> @@ -137,7 +138,7 @@ FIXTURE(ve_ns_owner)
>   
>   FIXTURE_SETUP(ve_ns_owner)
>   {
> -	unsigned long long initial_mnt_nr;
> +	unsigned long long initial_mnt_avail_nr;
>   	char val[16];
>   	char path[PATH_MAX];
>   
> @@ -174,13 +175,13 @@ FIXTURE_SETUP(ve_ns_owner)
>   
>   	/*
>   	 * The new ve cgroup has not been entered by anything yet, so its
> -	 * mnt_nr counter must start at 0. Each test below verifies that
> -	 * the clone/unshare populates the new mntns under this ve, i.e.
> -	 * mnt_nr rises strictly above zero.
> +	 * mnt_avail_nr counter should be VE_MOUNTS_MAX. Each test below
> +	 * verifies that the clone/unshare populates the new mntns
> +	 * under this ve, i.e. mnt_avail_nr strictly decreases.
>   	 */
> -	snprintf(path, sizeof(path), "%d/ve.mnt_nr", self->ctid);
> -	ASSERT_EQ(read_u64_at(self->cgv2_fd, path, &initial_mnt_nr), 0);
> -	ASSERT_EQ(initial_mnt_nr, 0);
> +	snprintf(path, sizeof(path), "%d/ve.mnt_avail_nr", self->ctid);
> +	ASSERT_EQ(read_u64_at(self->cgv2_fd, path, &initial_mnt_avail_nr), 0);
> +	ASSERT_EQ(initial_mnt_avail_nr, VE_MOUNTS_MAX);
>   };
>   
>   FIXTURE_TEARDOWN(ve_ns_owner)

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

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