Re: [Devel] [PATCH hci-8.1 1/4] ve: Remove STOPPED state

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] ve: renumbering VE_STATE_DEAD from 4 to 3 changes the state value logged by setns
- [Low] ve: commit message claims STOPPED removal blocks VE reuse, but it is only a cleanup
--

commit afa07824d11d984ebc29eb501787f9b9b4ecea47
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

ve: Remove STOPPED state

This patch merges VE_STATE_STOPPED into VE_STATE_DEAD. The stop path in
ve_exit_ns() and the error path in ve_start_container() now set DEAD
directly. It also drops the ve_set_state(DEAD) call in ve_destroy() and
renumbers VE_STATE_DEAD from 4 to 3.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-142405

> diff --git a/include/linux/ve.h b/include/linux/ve.h
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h
> @@ -40,8 +40,7 @@ struct vfsmount;
>  #define VE_STATE_STARTING	0
>  #define VE_STATE_RUNNING	1
>  #define VE_STATE_STOPPING	2
> -#define VE_STATE_STOPPED	3
> -#define VE_STATE_DEAD		4
> +#define VE_STATE_DEAD		3

[Severity: Low]
Is the renumbering of VE_STATE_DEAD from 4 to 3 intended to be visible in
logs?

The raw state integer is printed in the setns check in
kernel/ve/ve_namespace.c:

    ve_name(ve_ns->ve), ve_ns->ve->state,

and in the ve_set_state() WARN_ONCE messages. Anything that parses the
integer in the "setns to the VE namespace of a non-running CT (state %d)"
message now sees 3 for a stopped CT, where it could previously see 3 or 4.

The cgroup state file is unaffected, since ve_state_show() still prints
"STOPPED" for VE_STATE_DEAD.

[ ... ]

> @@ -680,7 +680,7 @@ void ve_exit_ns(struct pid_namespace *pid_ns)
>  	ve_hook_iterate_fini(VE_SS_CHAIN, ve);
>  	ve_list_del(ve);
>  	ve_drop_context(ve);
> -	ve_set_state(ve, VE_STATE_STOPPED);
> +	ve_set_state(ve, VE_STATE_DEAD);
>  	up_write(&ve->op_sem);

[Severity: Low]
This isn't a bug, but the commit message doesn't seem to match what the
patch does. It says:

    We really don't want to reuse VE after its stop.
    Some resources from the old run can reference this CT, so new
    start may see leftovers.

    Fix this by transitioning from STOPPING to DEAD directly.

Is that description accurate?

ve_start_container() already rejected a restart for any state other than
STARTING:

    if (ve->state != VE_STATE_STARTING || ve_nsproxy)
        return -EBUSY;

STARTING is only assigned in ve_create(). A VE in STOPPED could therefore
not be restarted before this patch, and a VE in DEAD cannot be restarted
now.

The ve_set_state(DEAD) call removed from ve_destroy() wrote to an object
that kmem_cache_free() releases immediately afterwards:

    ve_set_state(ve, VE_STATE_DEAD);
    kmem_cache_free(ve_cachep, ve);

So DEAD was never observable there.

The later patches in the series (override_ve/revert_ve and the overlayfs
creator VE changes) don't check VE_STATE_DEAD or VE_IS_RUNNING. They take
and drop references with get_ve() and put_ve(). It looks like the
"leftovers" problem is addressed by those reference changes and not by
this patch.

Could the commit message be reworded to say that this is a state machine
cleanup that merges STOPPED into DEAD and removes a dead store in
ve_destroy()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261022-ovl-use-orig-ve-v1-0-da84c00c8065@virtuozzo.com?part=1

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