[PATCH 06/23] nvmet: add context annotation for nvmet_ns::nsid

[Nilay Shroff]

On 11/10/26 3:26 AM, Bart Van Assche wrote:
> On 11/8/26 7:28 PM, Nilay Shroff wrote:
>  > [ ... ]
> 
> 1. Bisectability:
>     This patch causes Clang analyzer errors in nvmet_format_ana_group(),
>     nvmet_execute_identify_endgrp_list(), and nvmet_execute_identify_nslist(),
>     which are only resolved in patch 07. Please reorder or squash patch 07.
> 
As mentioned in your next comment, I agree that the nsid annotation is unnecessary
and will drop patch 06/23. Since patch 07/23 only addresses access violations
introduced by annotation added in patch 06/23, I'll drop patch 07/23 as well.

That said, patch 07/23 also addresses a separate locking issue: we should hold
subsys->lock while iterating over subsys->namespaces. However, I'll address this
when annotating subsys->namespaces with __guarded_by(&subsys->lock) in a
subsequent series.

> 2. Design:
>     ns->nsid is set once during nvmet_ns_alloc() and is immutable throughout
>     the lifetime of the namespace. It is never modified under subsys->lock or
>     any other lock. Marking it __guarded_by(&subsys->lock) and creating
>     nvmet_ns_nsid() to bypass the annotation throughout the driver adds
>     unnecessary churn. Please drop the annotation on nsid.

Agreed. Although ns->nsid is initialized under subsys->lock in nvmet_ns_alloc(),
this happens before the namespace is enabled or published. Since ns->nsid is never
modified afterwards in any other code path throughout the namespace's lifetime,
annotating it with __guarded_by(&subsys->lock) is unnecessary. I'll drop this
annotation.

>  > @@ -746,6 +755,12 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
>  >
>  >      init_completion(&ns->disable_done);
>  >
>  > +    __assume_ctx_lock(&ns->subsys->lock);
>  >      ns->nsid = nsid;
>  >      ns->subsys = subsys;
> 
> Here and elsewhere, lockdep_assert_held() should be used instead of
> __assume_ctx_lock(). Additionally, the lock state should only be
> verified after the ns->subsys assignment.
Yes correct, but if we drop this patch then above change would not be needed.

Thanks,
--Nilay