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

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

On 10/22/26 17:11, no-reply@sashiko.virtuozzo.com wrote:
> 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.
> 

STOPPED left intentionally, vzctl checks it.
Numbers in warns - don't care, these are debug messages.

> [ ... ]
> 
>> @@ -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()?

Huh, true. So it just drops useless state. I'll change commit message
in v2.

> 
> [ ... ]
> 

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

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